Created attachment 277638 [details] [review]
frames: Center menu coordinates horizontally to the menu button

Aligning the window menu with the left/right side of the titlebar
button made sense with the old GTK-based popups, but nowadays the
menu coordinates define the spot the menu's arrow points to - in
the shell, all other menus point to the center of their source, so
do the same for the window menu.

Created attachment 277639 [details] [review]
frames: Fix pressed state of window menu buttons

Since window menus have been moved to the compositor, the pressed
state of the corresponding window buttons is messed up, as it is
reset immediately when getting a LeaveNotify event due to the
compositor taking a grab. Fix this by ignoring that particular
event.

Created attachment 277640 [details] [review]
frames: Sync hover after compositor grab has been released

During compositor grabs, frames don't receive events to update the
prelit/pressed state of frame decorations. This is particularly
visible now that compositor grabs can happen as direct result of
interaction with frame decorations, so sync decorations' hover
state after the grab is released.

Created attachment 277642 [details] [review]
windowMenu: Do a better job with faking the source actor

The 0x0 dummyCursor works well when the menu pops up directly underneath
the pointer (e.g. when triggered by right-clicking the titlebar) or by
keyboard, but not when triggered by the menu button -  unless the user
keeps holding the mouse button while moving into the menu, the menu will
be dismissed immediately on button release. Address this by overlaying
the menu button with an appropriately sized actor, to make the window
menu behavior consistent with other shell menus.

Review of attachment 277638 [details] [review]:

OK.

Created attachment 277643 [details] [review]
windowMenu: Ignore button release when opening the menu

When the menu is triggered by clicking a titlebar button, it won't
pop up directly underneath the pointer and close immediately on
button release if the user does not move into the menu while
holding the button. Ignoring the first button release fixes the
common case, though it does not quite work as expected when
the pointer is neither on the menu nor the window button.

This patch is a much simpler alternative to the previous one, though as noted, the behavior is not quite correct in all cases.

Review of attachment 277639 [details] [review]:

Can we just ignore any event with GDK_NOTIFY_GRAB instead?

Review of attachment 277640 [details] [review]:

Hm, the X server should send us an EnterNotify with NotifyUngrab and the correct X/Y position. Is that not happening?

Review of attachment 277642 [details] [review]:

Not too happy about this, especially because the keynav is still sort of broken. I can't think of anything better, though.

::: js/ui/windowMenu.js
@@ +190,3 @@
+        // Pick the height of the frame decoration as size
+        let rect = window.get_frame_rect();
+        let size = y - rect.y;

I recently added meta_window_get_titlebar_rect here. Could that be used?

Review of attachment 277643 [details] [review]:

OK.

Created attachment 277741 [details] [review]
frames: Only skip updating prelight when both control and state match

(In reply to comment #7)
> Can we just ignore any event with GDK_NOTIFY_GRAB instead?

I don't think that is correct - for instance when hovering the close button and hitting alt-f2, the current behavior of removing the prelight makes sense to me. It's just in case of the window menus where there's a direct relation between button state and grab where keeping the state is better IMHO.
Regarding the NotifyGrab, I did initially try using event->mode, but for some reason it is always NORMAL in case of a compositor grab.


(In reply to comment #8)
> Hm, the X server should send us an EnterNotify with NotifyUngrab and the
> correct X/Y position. Is that not happening?

Yeah, this can be addressed a lot simpler, see updated patch.


(In reply to comment #9)
> +        let rect = window.get_frame_rect();
> +        let size = y - rect.y;
> 
> I recently added meta_window_get_titlebar_rect here. Could that be used?

I did see that, but not sure if it would be better. Ideally we'd match the actual visible button, but we don't have that. Next best thing would be to use the corresponding button_rect - I've considered adding width/height or replacing the coordinates with a rectangle, but both options make for rather confusing API. Now the above assumes that 'y' is below the button (e.g. it would still include the button when pointing 20px below the titlebar for some reason), while get_titlebar_rect() also assumes that it points to the bottom edge or inside the titlebar. This doesn't look like a terrible assumption, so if you feel it'd be more readable, I'll make get_titlebar_rect() introspecable and switch to that.

Review of attachment 277741 [details] [review]:

I'm not sure what this is fixing. There are three callers for update_prelit_control: motion, enter, and leave. Motion only calls it in prelit, so that doesn't change anything here. The other two always change the control, so we don't hit the early return.

Any hints?

(In reply to comment #11)
> I did see that, but not sure if it would be better. Ideally we'd match the
> actual visible button, but we don't have that. Next best thing would be to use
> the corresponding button_rect - I've considered adding width/height or
> replacing the coordinates with a rectangle, but both options make for rather
> confusing API.

I would be fine with proper button rect geometry in show_window_menu -- it would make it so that if the window menu button was near-offscreen, the menu would be flipped properly to be on top. We could have two APIs -- meta_window_show_window_menu for the common case and meta_window_show_window_menu_for_rect otherwise.

(In reply to comment #12)
> I'm not sure what this is fixing. There are three callers for
> update_prelit_control: motion, enter, and leave. Motion only calls it in
> prelit, so that doesn't change anything here. The other two always change the
> control, so we don't hit the early return.

It may be hit in the enter case after applying attachment 277639 [details] [review]. when a leave event is ignored and the control is stilled hovered after dismissing the menu.

(In reply to comment #13)
> I would be fine with proper button rect geometry in show_window_menu -- it
> would make it so that if the window menu button was near-offscreen, the menu
> would be flipped properly to be on top.

Seems to be working fine already, but sure, I'll add API.


> We could have two APIs -- meta_window_show_window_menu for the common case and
> meta_window_show_window_menu_for_rect otherwise.

Sigh, adding API like that would be much nicer if it didn't involve going through 1000 layers between UI and compositor :-(

I've been meaning to clean that up for a while, and just make meta_compositor_* call the MetaPlugin vfuncs directly. That would remove a few of the layers. Really, the majority of code should stay as-is, with only the plugin interface changing, and a new piece of code to access it.

(If you want to push these commits now and then fix up and add the API later, that's fine by me...)

Review of attachment 277741 [details] [review]:

Ah, right, that makes sense then.

Created attachment 277767 [details] [review]
Pass button_rect when opening window menu from button

When opening the window menu without an associated control - e.g.
by right-clicking the titlebar or by keyboard - using coordinates
for the menu position is appropriate. However when the menu is
associated with a window button, the expected behavior in the
shell can be implemented much easier with the full button geometry:
the menu will point to the center of the button's bottom edge
rather than align to the left/right side of the titlebar as it
does now, and the clickable area where a release event does not
dismiss the menu will match the actual clickable area in mutter.

So add an additional show_window_menu_for_rect() function and
use it when opening the menu from a button.

Created attachment 277768 [details] [review]
windowMenu: Implement new show_menu_for_rect() hook

Having the full geometry of the menu's source button (if any) will
allow us to address several misbehaviors of window menus, so use
that instead of show_menu().

Created attachment 277769 [details] [review]
windowMenu: Do a better job with faking the source actor

The 0x0 dummyCursor works well when the menu pops up directly underneath
the pointer (e.g. when triggered by right-clicking the titlebar) or by
keyboard, but not when triggered by the menu button - the menu does not
point to the center of the button's bottom edge, and unless the user
keeps holding the mouse button while moving into the menu, the menu will
be dismissed immediately on button release.
Address these issues by using the button geometry to overlay the window
button with an appropriately sized actor that acts as a proper sourceActor,
to make the window menu behavior consistent with other shell menus.

Review of attachment 277768 [details] [review]:

Oh, I was imagining that we'd just fake a 1x1 rectangle in mutter and pass that through its hooks, but I guess this works too.

Review of attachment 277767 [details] [review]:

::: src/ui/frames.c
@@ +1228,3 @@
 
+          /* relies on both structs being identical */
+          root_rect = *((MetaRectangle *)rect);

Yuck. I'd just do:

    int win_x = event->x_root - event->x;

    root_rect.x = win_x + rect.x;
    ...
    root_rect.width = rect.width;

Review of attachment 277769 [details] [review]:

OK.

Attachment 277639 [details] pushed as e2105dc - frames: Fix pressed state of window menu buttons
Attachment 277741 [details] pushed as a7f0838 - frames: Only skip updating prelight when both control and state match
Attachment 277767 [details] pushed as b64548e - Pass button_rect when opening window menu from button

Attachment 277768 [details] pushed as 5b61f2d - windowMenu: Implement new show_menu_for_rect() hook
Attachment 277769 [details] pushed as e51eb72 - windowMenu: Do a better job with faking the source actor