On Tue, 7 Apr 2026 16:20:58 GMT, Martin Fox <[email protected]> wrote:
>> Jose Pereda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rework menuWillOpenHandler to apply it only to auto-hide popups
>
> modules/javafx.graphics/src/main/native-glass/mac/GlassMenu.m line 187:
>
>> 185: object:_menu
>> 186: userInfo:nil];
>> 187:
>
> If the system menu is canceled this code will still send out a
> jMenuOpeningMethod call for it. The matching jMenuClosedMethod will never be
> sent.
>
> I don't think you need the notification (?). In menuWillOpen: you can just
> check to see if there's a grab window. If there's a grab window cancel
> tracking on the menu, clear the grab, and return. But I sort of like the
> notification, it helps verify that there really is a popup open and visible
> to the user.
Not sure what you mean about the system menu being canceled? The menu delegate
only has `menuWillOpen:` and `menuDidClose:`, and it would be hard to catch a
"cancel event" that prevented the menu from opening or when it was already
opened.
Worse case, if for whatever reason the system menu gets canceled, the popups
get closed.
As for the `jmenuClosedMenu` not being sent, that would have been already the
case before this PR, wouldn't it?
About notification vs grab: You are right! I could simply use the grab check
instead of using the more complex notification pattern.
This PR initially didn't have the static `hasGrab:` check, and it was simply
using the notification. After your feedback, I added it, and now, as you say,
we could benefit from having this quick access :
- (void)menuWillOpen: (NSMenu *)_menu
{
- [[NSNotificationCenter defaultCenter]
postNotificationName:GlassMenuBarWillOpenNotification ... ]
+ if ([GlassWindow _hasGrab]) {
+ [_menu cancelTrackingWithoutAnimation];
+ [GlassWindow _resetGrab];
+ return;
+ }
...
This will prevent managing the observer lifecycle, broadcasting to multiple
popup windows (just to check which one has the grab and then close it), and it
implies less code changes.
On the other hand, as you say, the notification pattern seems more clear, not
only for separation of concerns, and maybe for other future cases where the
system menu opening event is needed. But for now seems a little bit overkill.
So what do you think? Should I go ahead and change it removing the notification
in favour of the grab?
> modules/javafx.graphics/src/main/native-glass/mac/GlassWindow+Overrides.m
> line 327:
>
>> 325: // or hiding the popup.
>> 326: // Note that only auto-hide popups have an active focus grab.
>> 327: if (![self->nsWindow isVisible] || ![GlassWindow _hasGrab]) {
>
> I'm not sure you need the isVisible check though there's no harm in leaving
> it in. As far as I know there should never be an invisible popup; when JavaFX
> hides the popup the NSWindow is destroyed, not hidden.
If we remove the notification (as discussed in the other comment), this code
would be removed.
If we keep it, the visible check is indeed not needed, since there can be only
one auto-hide popup with an active grab (and it will be visible), which is the
one that should cancel tracking and reset the grab.
Note that I had only the visible check before I added the grab one.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2102#discussion_r3050461492
PR Review Comment: https://git.openjdk.org/jfx/pull/2102#discussion_r3050594974
