Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Windows/macOS] Implement menu key accelerators #14740

Merged
merged 58 commits into from
Jul 3, 2023
Merged

[Windows/macOS] Implement menu key accelerators #14740

merged 58 commits into from
Jul 3, 2023

Conversation

jsuarezruiz
Copy link
Contributor

@jsuarezruiz jsuarezruiz commented Apr 24, 2023 •
edited by mattleibow
Loading

Description of Change

Implement menu key accelerators.

Menu Bar Context
fix-5211-2 fix-5211

Issues Fixed

Fixes #5211
Related with #14021

@jsuarezruiz jsuarezruiz added t/bug Something isn't working platform/windows 🪟 legacy-area-desktop Windows / WinUI / Project Reunion & Mac Catalyst / macOS specifics (Menus & other Controls)) area-controls-menubar Desktop MenuBarItems labels Apr 24, 2023
@jsuarezruiz jsuarezruiz marked this pull request as ready for review April 25, 2023 10:45
@jsuarezruiz
Copy link
Contributor Author

We need to do the same on macOS. It will be in a separate PR.

rachelkang
rachelkang reviewed Apr 25, 2023
View reviewed changes
Copy link
Member

@rachelkang rachelkang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let's hold off on merging until we have a better sense of the Mac implementation though, to ensure this API looks the way we want it to.

Things we might consider/look more into:

  • Separating the modifier and key into separate paramaters, and not using strings (especially with parsing around +)
  • Create a property to enable implementation via XAML
  • Tests

PureWeen
PureWeen requested changes May 3, 2023
View reviewed changes
src/Core/src/Core/IAccelerator.cs Outdated Show resolved Hide resolved
@jsuarezruiz
Copy link
Contributor Author

Updated the FromString method from the Accelerator class to fix some tests, also added more to cover more scenarios.

@jsuarezruiz
Copy link
Contributor Author

I really like this! But to keep up with the documentation efforts, I'd be very thankful if you add docstrings to the new public APIs! Here's a quick list of the things that I found.

  • Public string Key --- Accelerator.cs
  • Public interface IAccelerator --- IAccelerator.cs
  • Public static void MapAccelerator --- MenuFlyoutSubItemHandler.Windows.cs
  • KeyBoardAcceleratorExtensions.cs --- there a couple of public symbols here.

Bonus points if you add docs to other public methods and properties in the modified files 😊

@jknaudt21 will do, thanks for the reminder JD! :)

Added pending docstrings to the public APIs.

mandel-macaque
mandel-macaque approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking due to my comments, they are just food for thought.

src/Controls/src/Core/Accelerator.cs Outdated Show resolved Hide resolved
src/Controls/src/Core/Accelerator.cs Show resolved Hide resolved
src/Controls/src/Core/Menu/MenuItem.cs
@@ -145,6 +145,9 @@ static object CoerceIsEnabledProperty(BindableObject bindable, object value)
return false;
}

IReadOnlyList<IAccelerator> IMenuElement.Accelerators =>
GetAccelerator(this) is Accelerator acc ? new[] { acc } : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment, should we one returning null? Or an empty list of IAccelerator. Why the need of a IReadOnlyList? Are users going to access the by index?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order matters I think because you want the accelerators to be in a specific order. The null is not my favorite but some people like it - not sure it matters. We can always not-null it later, but making it null later is breaking.

src/Core/src/Platform/Windows/KeyboardAcceleratorExtensions.cs

var keyboardAccelerator = new KeyboardAccelerator();
keyboardAccelerator.Key = key.ToVirtualKey();
if (modifiers is not null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making modifiers to be an empty array rather than nullable will remove this type of checks.

src/Core/src/Platform/Windows/KeyboardAcceleratorExtensions.cs
Comment on lines +90 to +96
if (int.TryParse(key, out int numberKey))
{
if (numberKey >= 0 && numberKey <= 9)
{
key = $"Number{key}";
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, so the setup is wrong, yet we are not throwing an exception or anything, meaning that we will have an issue at run time (key is revamped maybe?). If the number is out of range, we should throw and out of range exception.

rachelkang and others added 2 commits June 27, 2023 19:25
hartez
hartez suggested changes Jun 30, 2023
View reviewed changes
src/Controls/src/Core/Accelerator.cs Outdated Show resolved Hide resolved
@jsuarezruiz jsuarezruiz requested a review from hartez July 3, 2023 12:17
mattleibow
mattleibow approved these changes Jul 3, 2023
View reviewed changes
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work team!

@rachelkang rachelkang merged commit 752c724 into main Jul 3, 2023
@rachelkang rachelkang deleted the fix-5211 branch July 3, 2023 18:16
@davidbritch davidbritch mentioned this pull request Aug 9, 2023
Closed
@PureWeen PureWeen mentioned this pull request Aug 16, 2023
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2023
@Eilon Eilon added t/desktop The issue relates to desktop scenarios (MacOS/MacCatalyst/Windows/WinUI/WinAppSDK) and removed legacy-area-desktop Windows / WinUI / Project Reunion & Mac Catalyst / macOS specifics (Menus & other Controls)) labels May 10, 2024
@samhouts samhouts added the fixed-in-8.0.0-preview.7.8842 Look for this fix in 8.0.0-preview.7.8842! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jknaudt21 jknaudt21 jknaudt21 approved these changes

@rmarinho rmarinho rmarinho approved these changes

@MartyIX MartyIX MartyIX left review comments

@rachelkang rachelkang rachelkang approved these changes

@PureWeen PureWeen PureWeen approved these changes

@mandel-macaque mandel-macaque mandel-macaque approved these changes

@mattleibow mattleibow mattleibow approved these changes

@hartez hartez hartez approved these changes

Assignees
No one assigned
Labels
area-controls-menubar Desktop MenuBarItems fixed-in-8.0.0-preview.7.8842 Look for this fix in 8.0.0-preview.7.8842! platform/windows 🪟 t/bug Something isn't working t/desktop The issue relates to desktop scenarios (MacOS/MacCatalyst/Windows/WinUI/WinAppSDK)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Menu missing accelerators