Refactor Menu and ContextMenu events, allow for getting pointer info on open #6012
Conversation
|
There are usually always 4 events related to popups: Opening, Opened, Closing and Closed. I do not think it is a good idea to get rid of the Closing event. In terms of a menu, the use cases for Closing are more rare. I'm sure there are cases where clicking on a menu item will cause a confirmation of some sort first which will delay closing of the menu. The Closing is really there to provide the option of manually controlling how the menu closes. Aside from the obvious API symmetry keeping this event, it seems like a bad idea to handicap future applications by removing it. |
|
Also, don't forget Avalonia/src/Avalonia.Controls/Flyouts/FlyoutBase.cs Lines 95 to 98 in 8f83939 |
| /// <summary> | ||
| /// Defines the <see cref="Opened"/> event. | ||
| /// </summary> | ||
| public static readonly RoutedEvent<RoutedEventArgs> OpenedEvent = |
There was a problem hiding this comment.
If we have Menu breaking changes, should we follow WPF/UWP APIs then?
In WPF there is no Menu.Opened/Closed event, which makes sense for me (do we really need them?).
But there is MenuItem.SubmenuOpenedEvent and MenuItem.SubmenuClosedEvent.
In case of UWP I don't see anything like that neither in MenuBar nor MenuBarItem nor MenuFlyoutSubItem.
There was a problem hiding this comment.
Yeah not sure about Menu.Opened as the menu itself doesn't really "open" - it's its submenus that open. Having said that NativeMenu has an Opening event which maps to https://developer.apple.com/documentation/appkit/nsmenudelegate/1518156-menuwillopen?language=objc.
| /// <summary> | ||
| /// Occurs when a <see cref="ContextMenu"/> is opened. | ||
| /// </summary> | ||
| public event EventHandler<ContextMenuEventArgs> Opened |
There was a problem hiding this comment.
Do we really need to have this breaking change from ContextMenuOpening to Opened? Similarly to other events in this class.
I though we keep ContextMenu in Avalonia only because of historical reasons and compatibility with WPF?
There was a problem hiding this comment.
That´s a good point. There is no reason to keep ContextMenu in my opinion if there is a breaking change from WPF anyway. It´s a good time to switch to MenuFlyout´s.
There was a problem hiding this comment.
I think we should keep ContextMenu, and migrate it to use native menus on relevant platforms as part of #3855. With that in mind, unifying the events is a good idea.
There was a problem hiding this comment.
@grokys ContextFlyout API is part of future native menu API.
It was discussed to be able tool write somethinf similar to:
<Control.ContextFlyout>
<NativeMenuFlyout>
<NativeMenuItem />
</NativeMenuFlyout>
</Context.ContextFlyout>Or
<Control.ContextFlyout>
<MenuFlyout ShouldUseNativeMenu="True" >
<MenuItem />
</MenuFlyout>
</Context.ContextFlyout>In any way existing Flyout API is more flexible comparing with ContextMenu.
Also it's not limited to right-click, and can be involed with public API on any control by any event.
| /// Initializes a new instance of the ContextMenuEventArgs | ||
| /// class. | ||
| /// </summary> | ||
| public Point? GetPosition(IVisual? relativeTo) |
There was a problem hiding this comment.
I believe it should be mentioned in the docs summary, that GetPosition returns null, when context menu was opened without pointer interaction?
There was a problem hiding this comment.
UWP uses TryGet pattern in ContextRequestedEventArgs that I am going to port as well. Does it make sense to apply it here?
https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.input.contextrequestedeventargs.trygetposition?view=winrt-20348#Windows_UI_Xaml_Input_ContextRequestedEventArgs_TryGetPosition_Windows_UI_Xaml_UIElement_Windows_Foundation_Point__
|
Would this allow one to programmatically tell whether a (totally not referring to how some context menus in Windows 10 enlarge their items if opened via tap-and-hold, but keep them smaller if opened via right-click) |
|
@Splitwirez Lots of other info could be added to ContextMenuEventArgs in addition to GetPosition |
|
Another thing to bear in mind is that WPF has This sounds like it might be a useful place to raise opening/closing events for context menus, particularly for menus which are shared between multiple controls. Does |
With the event below developer can show anything when context flyout was requested. IMO it makes more sense. |
Agreed. Would it be worth including this event in this PR? |
|
Closing this PR for now. Would be great if anybody wants to pick it up. |
What does the pull request do?
Refactors ContextMenus and Menus based on #4443, and embeds position info for the Opened and Opening events on the ContextMenu for #5793.
What is the current behavior?
Open/close menu events are confusing. See #4443 for details why. It is also currently hard to tell where mouse cursor was when ContextMenu was opened.
What is the updated/expected behavior with this PR?
This PR drops the Closing event, and unifies naming across other events. On the ContextMenu, Opening and Opened events now have a
GetPosition(IVisual? relativeTo)method which can be used to retrieve the position of the pointer at the time of the event opening.How was the solution implemented (if it's not obvious)?
These were implemented after some discussion with @grokys, this does change the API in meaningful ways so I'm obviously still looking for feedback on whether this API change is justified (particularly, the dropping of the Closing event).
Checklist
Breaking changes
In addition to the removal of the redundant Closing event, Menu and ContextMenu event names were slightly changed.
Obsoletions / Deprecations
Fixed issues
Fixes #4443
Fixes #5793