-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changing fallback behaviour in PlatformKeyGestureConverter.ToPlatformString() to be more user friendly. #15393
Changing fallback behaviour in PlatformKeyGestureConverter.ToPlatformString() to be more user friendly. #15393
Conversation
… ToString(gesture, meta) method rather than gesture.ToString(). This will ensure Keys where the enum value doesn't match the KeySymbol are printed in a more readily understood format for users on platforms that don't have a specific implementation, e.g. Android.
You can test this PR using the following package version. |
@@ -57,7 +57,7 @@ public static string ToPlatformString(KeyGesture gesture) | |||
} | |||
else | |||
{ | |||
return gesture.ToString(); | |||
return ToString(gesture, "Cmd"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Win" key would make more sense since it's likely an external keyboard we are dealing with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with Cmd, because that's what the gesture.ToString() prints for Meta. I agree it's almost certainly an external keyboard, they don't all say Win. The logitech K480 I've been using for testing is "Start | alt opt".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing to keep in mind is that this isn't Android specific, it's any runtime that's not nominally Windows, Linux, or OSX. Edit: Just confirmed this code path is also hit by Browser apps.
Actually looking deeper, it seems using RuntimeInformation.IsPlatform() might no longer be the appropriate way to do this, as a lot of the tests have been added to System.OperatingSystem with more granularity (Browser, Android, iOS, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with Cmd, because that's what the gesture.ToString() prints for Meta
That's a good point: https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Base/Input/KeyGesture.cs#L130
Possibly it's worth reusing code between PlatformKeyGestureConverter and KeyGesture. Their ToString() and Parse() combination should be compatible and unit tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually looking deeper, it seems using RuntimeInformation.IsPlatform() might no longer be the appropriate way to do this
In general, yes. We should use OperatingSystem API (or OperatingSystemEx in Avalonia repo). But you probably won't get much out of it, unless you need to add cases for mobile and browser as well. It also won't tell you on which platform browser is running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part the ToString functions are the same. The biggest difference is the Meta variations in the converter, and using a variety of symbols for the OSXString. These could probably be moved to KeyGesture.ToString() in some way, and then the converter simply checks the platform and calls as appropriate. Would it be desirable for the default KeyGesture.ToString() to print user friendly characters rather than enum strings, or would that be considered a breaking change? I suspect it would.
From what I understand of Key parsing, the default ToString should probably use the Enum value strings, or digits will cause problems.
So maybe something like?:
KeyGesture.ToString(string metaOverride = "cmd", bool prettyPrint = false)
KeyGesture.ToString() => ToString("Cmd", false)
KeyGesture.ToOSXString()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a second look at this issue. You are right, OSX differences in formatted strings make it more difficult.
For the most part, our differences between platform hotkeys are defined by the PlatformHotkeyConfiguration class.
Which also can be redefined by the platform OS as well. Which allows us to avoid RuntimeInformation.IsOSPlatform checks in shared code.
What do you think about:
- KeyGesture.ToString() to always use enum values.
- Add KeyDisplayName properties to PlatformHotkeyConfiguration or something like this. Can be a dictionary, I guess.
- KeyGesture.ToFormattedString() that internally uses PlatformHotkeyConfiguration configuration for special key display names. No need for ToOSXString.
- KeyGesture.Parse/TryParse expects enum values, as it's normally defined in XAML as well. Need to make sure that XAML compiler also uses that.
- PlatformKeyGestureConverter to use KeyGesture.ToFormattedString internally.
Now, ToFormattedString
is not as common in .NET BCL. There are methods named like this in .NET in general, but I couldn't find any in BCL itself. Normally CultureInfo parameter makes all difference for ToString formatting strategy of different types, but we can't use it here. https://www.google.com/search?q=ToFormattedString%20site:learn.microsoft.com
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to follow the BCL, I think we would need to implement IFormattable
/ISpanFormattable
in KeyGesture
. Then we can use the format string + the format provider to output the gesture as desired.
A new type would provide the modifier key strings (or PlatformHotkeyConfiguration
as you mentioned, but I think that isn't its responsability) and implement IFormatProvider
.
Then KeyGesture
can look for the needed format in IFormatProvider
, falling back to the current platform and/or invariant values as needed. People are able to provide the format they need.
Sample, using n
as the invariant format and p
as platform specific ones:
public sealed class KeyModifierFormatInfo(string meta) : IFormatProvider
{
public string Meta { get; } = meta;
// TODO: Ctrl, Shift, etc.
public object? GetFormat(Type? formatType)
=> formatType == typeof(KeyModifierFormatInfo) ? this : null;
public static KeyModifierFormatInfo Invariant { get; } = new("Meta");
public static KeyModifierFormatInfo GetInstance(IFormatProvider? formatProvider)
=> formatProvider?.GetFormat(typeof(KeyModifierFormatInfo)) as KeyModifierFormatInfo
?? AvaloniaLocator.Current.GetService<KeyModifierFormatInfo>()
?? Invariant;
}
// In KeyGesture:
public override string ToString()
=> ToString(null, null);
public string ToString(string? format, IFormatProvider? formatProvider)
{
var formatInfo = format switch
{
null or "" or "n" => KeyModifierFormatInfo.Invariant,
"p" => KeyModifierFormatInfo.GetInstance(formatProvider),
_ => throw new FormatException("Unknown format specifier");
};
// output gesture using formatInfo here
// ...
}
Then it's the matter of documenting the available formats, and its link to KeyModifierFormatInfo
in the rare event where an user wants to completely customize the output. it might feel a bit over-engineered, but that's the pattern used everywhere by the BCL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I have a bit of time to work on this now. Where would the KeyModifierFormatInfo class (or equivalent) go in the Avalonia projects? Avalonia.Controls or Avalonia.Base are where I'm digging around at the moment, but if there's a better spot please let me know.
You can test this PR using the following package version. |
I've done the implementation of IFormattable and opened a new pull request, #15828 using the guidelines provided by MrJul. Please check that pull request and let me know what changes still need to be made. |
Please read the following Contributor License Agreement (CLA). If you agree with the CLA, please reply with the following:
Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by AvaloniaUI OÜ and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant AvaloniaUI OÜ, and those who receive the Submission directly b. Patent License. You grant AvaloniaUI OÜ, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to AvaloniaUI OÜ. You agree to notify AvaloniaUI OÜ in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the Republic of Estonia, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and AvaloniaUI OÜ dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. |
PlatformKeyGestureConverter.ToPlatformString() falls back to KeyGesture.ToString() rather than it's own user friendly string creation method ToString(KeyGesture gesture, string meta). This will print Key enum values such as "OemPeriod" or "D1" rather than more userfriendly strings like "." or "1"
What does the pull request do?
Change the fallback case to use ToString(gesture, "Cmd") rather than gesture.ToString(). This will print user friendly key values for when platform specific cases aren't implemented. Specifically Android.
What is the current behavior?
Key enum strings are printed.
What is the updated/expected behavior with this PR?
More user friendly values are printed.
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
None I can think of.
Fixed issues
Fixes #15392
Other Notes:
I considered adding an Android specific path as well, but couldn't come to a decision about what the appropriate "Meta" string would be. The 2 Android Devices I checked use 🔍 (U+1F50D) for the OS displayed shortcuts list, but in practice it depends on the actual physical keyboard. The Logitech K480 I have for testing triggers those shortcuts with a key labeled "start | alt opt", while the actual key with a magnifying glass on it does not trigger them. Another option I found was using "OS" for the meta label, but that seems to be ROM specific. I'm not sure there is a good universal "Meta" string, so left it at as the fallback case with "Cmd", which is a minimal change from current behaviour.