Implementing IFormattable on KeyGesture #15828
Conversation
…rings when performing KeyGesture.ToString()
|
Oh damn. Didn't spot the Converter's ToPlatformString() method was public, and so removing it is a Breaking Change which is upsetting the tests obviously. I'll put it back. |
|
|
@cla-avalonia agree |
|
You can test this PR using the following package version. |
| Key.Left => "←", | ||
| Key.Return => "↩", | ||
| Key.PageDown => "⇞", | ||
| Key.PageUp => "⇟", |
There was a problem hiding this comment.
I don't use macOS/iOS, but I think PageDown/PageUp were swapped here (I found this image on SO), so it should be fixed on the new locations:
There was a problem hiding this comment.
I did think that looked weird, and doing a bit of searching have found other references to the same. I'll fix those up.
|
You can test this PR using the following package version. |
| public string ToString(string? format, IFormatProvider? formatProvider) | ||
| { | ||
| var formatInfo = format switch | ||
| { | ||
| null or "" or "g" => KeyGestureFormatInfo.Invariant, | ||
| "p" => KeyGestureFormatInfo.GetInstance(formatProvider), | ||
| _ => throw new FormatException("Unknown format specifier") | ||
| }; |
There was a problem hiding this comment.
We probably should consider using CultureInfo passed as IFormatProvider to be used for localization here. And pass it down from IValueConverter. But that's a bigger issue for another day.
There was a problem hiding this comment.
True, I imagine "Up Arrow" is not going to be universally understood by every language on Earth. I'll take a look into how that sort of thing is done, but as you say it's a bigger problem for another day. I'll just take steps to try and make sure I'm not making that other day any harder than it needs to be.
…adOnlyDictionary along with moving Arrow key and Backspace key overrides to the common dictionary. Only Apple platforms are now overriding Keys explicitly.
…valonia_Fixes into KeyFormatInfo_branch
|
You can test this PR using the following package version. |
| if (platformKeyOverrides == null) | ||
| return key.ToString(); | ||
|
|
||
| return platformKeyOverrides.TryGetValue(key, out string? result) ? result : |
There was a problem hiding this comment.
Nit: can be simplified to platformKeyOverrides.TryGetValue(key, out var result) || commonKeyOverrides.TryGetValue(key, out result) ? result : key.ToString(), avoiding the nested ternary operators.
|
You can test this PR using the following package version. |
What does the pull request do?
Implements IFormattable on KeyGesture, and the associated KeyGestureFormatInfo class for platform specific KeyGesture formatting information. Follows the general outline provided by @MrJul here: #15393 (comment)
What is the current behavior?
Currently KeyGestures, when printed in user facing scenarios such as menus, are only nicely formatted on Windows, Linux, and MacOS/iOS. Other platforms like Android are left with basic Enum values. As seen in #15392
What is the updated/expected behavior with this PR?
All platforms have more appropriate results displayed in Menus, so as not to confuse users.
How was the solution implemented (if it's not obvious)?
The code for making KeyGesture strings more user-friendly overlapped a lot with the basic KeyGesture.ToString(), so as per the discussion in #15393 IFormattable has been implemented on KeyGesture, and an IFormatProvider implemented to provide the platform specific strings for specific Keys/Modifiers where they do not match the Enum string. For example "." instead of "OemPeriod" or "1" instead of "D1".
A FormatProvider was implemented (KeyGestureFormatInfo), and registered on every platform where PlatformHotkeyConfiguration was also registered using my best guess at the appropriate overrides. These platform registrations will need review, as I don't have or use any iOS/MacOS devices and can't comment on whether these choices were all appropriate. Similarly I wasn't sure what platform AvaloniaNativePlatform represented, but found hints it was an Apple platform so copied from the iOS registrations.
Checklist
Breaking changes
Formatting of menus may have changed on platforms that I don't have access to to test. I have only been able to check Windows, Android, and Browser platforms, all others will need to be checked by someone with familiarity and access.
Obsoletions / Deprecations
Fixed issues
Fixes #15392