On Wed, 8 Apr 2026 14:38:07 GMT, Martin Fox <[email protected]> wrote:
>> 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?
>
>> Not sure what you mean about the system menu being canceled?
>
> Sorry, I should have been clearer. The call to `[_menu
> cancelTrackingWithoutAnimation]` will cause the menu to not actually open but
> this PR will still send the jMenuOpeningMethod event. Since the menu never
> opened it will never close so there won't be a matching jMenuClosedMethod
> event.
>
>> Should I go ahead and change it removing the notification in favour of the
>> grab?
>
> Yes, I think you should remove it. There might be some strange case where
> s_grabWindow isn't set to nil when it should be and we'll cancel the opening
> of a system menu unexpectedly. But the chance of that is very small and the
> problem would correct itself in a single click.
Done!
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2102#discussion_r3053022471
