GNOME Bugzilla – Bug 707863
wayland: implement support for popup surfaces
Last modified: 2013-09-16 12:54:02 UTC
Popup surfaces are mapped into override_redirect surfaces of a DROPDOWN_MENU type, with the addition of a special pointer grab.
Created attachment 254605 [details] [review] wayland: implement support for popup surfaces
Review of attachment 254605 [details] [review]: ::: src/core/window-private.h @@ +682,2 @@ void meta_window_recalc_window_type (MetaWindow *window); +void meta_window_type_changed (MetaWindow *window); I'm not too happy with this name. ::: src/core/window.c @@ +8682,1 @@ window->border_only = window->mwm_border_only; These should be FALSE as well for Wayland clients; we should never stare at MWM hints for Wayland. ::: src/wayland/meta-wayland-pointer.c @@ +562,3 @@ + MetaWaylandPointerGrab *grab; + + if (pointer->grab != &pointer->default_grab) Does Wayland not support stacked popups?
(In reply to comment #2) > Review of attachment 254605 [details] [review]: > > ::: src/core/window-private.h > @@ +682,2 @@ > void meta_window_recalc_window_type (MetaWindow *window); > +void meta_window_type_changed (MetaWindow *window); > > I'm not too happy with this name. Proposals? > ::: src/wayland/meta-wayland-pointer.c > @@ +562,3 @@ > + MetaWaylandPointerGrab *grab; > + > + if (pointer->grab != &pointer->default_grab) > > Does Wayland not support stacked popups? It does, but I need to implement it.
Created attachment 254675 [details] [review] wayland: heavily refactor pointer grabs Grabs are now slice allocated structures that are handled by whoever starts the grab. They contain a generic grab structure with the interface and a backpointer to the MetaWaylandPointer. The grab interface has been changed to pass full clutter events, which allowed to remove the confusion between grab->focus and pointer->focus. Invidual grabs are now required to keep their focus, and choose whoever gets the events.
Created attachment 254676 [details] [review] wayland: implement stacked popups Allow multiple popups from the same clients to be stacked under the same pointer grab. This is necessary to implement submenus.
Created attachment 254677 [details] [review] wayland: remove some wl_signal usage It was a left-over from the initial code import from weston.
Review of attachment 254675 [details] [review]: Yes.
Review of attachment 254676 [details] [review]: ::: src/wayland/meta-wayland-pointer.c @@ +500,3 @@ MetaWaylandPointerGrab generic; + MetaWaylandSurface *first_popup; From what I can tell, "first_popup" means "the popup at the top that currently has the grab", which isn't necessarily the first popup to start the grab. I'd remove the GList, and simply add a next_popup pointer right in MetaWaylandPopup, and change this to "current_popup".
Review of attachment 254677 [details] [review]: Yes.
(In reply to comment #8) > Review of attachment 254676 [details] [review]: > > ::: src/wayland/meta-wayland-pointer.c > @@ +500,3 @@ > MetaWaylandPointerGrab generic; > > + MetaWaylandSurface *first_popup; > > From what I can tell, "first_popup" means "the popup at the top that currently > has the grab", which isn't necessarily the first popup to start the grab. > > I'd remove the GList, and simply add a next_popup pointer right in > MetaWaylandPopup, and change this to "current_popup". No, first_popup is just a handy way to know which client holds the grab (which is in owner-events mode, so there isn't a single window which gets all the events). I could replace it with a wl_client pointer, I guess. Also, the next_popup idea is not bad.
Created attachment 254717 [details] [review] wayland: implement stacked popups Allow multiple popups from the same clients to be stacked under the same pointer grab. This is necessary to implement submenus.
Review of attachment 254717 [details] [review]: This is a lot clearer, yeah. ::: src/wayland/meta-wayland-pointer.c @@ +569,3 @@ g_assert (popup_grab->generic.interface == &popup_grab_interface); + wl_list_for_each_safe (popup, tmp, &popup_grab->all_popups, link) If wl_list_insert appends to the list, this means that we'll be sending popup dones in the order that they were added in. So, if client adds A, B, C, then we'll send popup_done to A, then B, then C. I'm not sure if the spec (or Weston) says anything about the ordering, but I think we should pop them off the stack, so we'll send done to C, then B, then A.
I did nothing about the ordering because wl_list_insert prepends to the list, not appends (so this is effectively a popup stack). Also, wl_list_insert + wl_list_for_each is exactly how weston implements it, so we're guaranteed maximum compatibility. Attachment 254605 [details] pushed as 76e2455 - wayland: implement support for popup surfaces Attachment 254675 [details] pushed as 776a86a - wayland: heavily refactor pointer grabs Attachment 254677 [details] pushed as c0e7f6d - wayland: remove some wl_signal usage Attachment 254717 [details] pushed as 9a13b85 - wayland: implement stacked popups