GNOME Bugzilla – Bug 744452
Update mutters xdg-shell implementation to unstable v5
Last modified: 2015-02-17 14:18:04 UTC
These patches updaes mutters xdg-shell implementation to the unstable version 5. One of the patches introduces the enum MetaWaylandSurfaceRole. In weston this is a string but I went with an enum simply because we can, and it allows us to rely on the compiler to check whether all roles are dealt with when necessary. I have also been considering making "roles" an vfunc API, and never rely on a checking a role except when assigning one, but that I have yet to start. Another patch moves most of the popup logic to its own file. This is because some popup logic is needed to be exposed to outside of meta-wayland-pointer.c. We probably want it outside there anyway to be able to support touch based popups or keyboard based popups, but that I also have yet to get started on.
Created attachment 296736 [details] [review] wayland: Use custom dispatcher until use_unstable_version is requested If any other request is made, reply with an error.
Created attachment 296737 [details] [review] wayland: Update to xdg-shell unstable version 5 Updates the function type signatures and version number. The rest will come as separate commits.
Created attachment 296738 [details] [review] wayland: Introduce MetaWaylandSurfaceRole Introduce surface roles and use it to ensure a surface never changes role.
Created attachment 296739 [details] [review] wayland: Fail when popup parent does not have an allowed role An xdg_popup may only have another xdg_popup or xdg_surface as a parent, so send an error if it provides an invalid parent.
Created attachment 296740 [details] [review] wayland: Check the serial when creating popups Send popup_done immediately if the serial is incorrect so the client can destroy its resources.
Created attachment 296741 [details] [review] wayland: Move out popup logic to its own file We'll want to expose popup logic outside of meta-wayland-pointer.c and one day we'll also probably want to add touch support for popups, so lets move it to its own file. There are no significant semantical changes, only refactoring.
Created attachment 296742 [details] [review] wayland: Unmap popup windows when a popup chain is dismissed When dismissing a popup grab, always unmap every popup window in the chain, instead of relying on the surfaces and xdg_popups being destroyed.
Created attachment 296743 [details] [review] wayland: Fail clients who try to create or destroy a not-top-most popup If a client creates an xdg_popup given a parent that is a xdg_popup that is not the most top one in the grab chain, send the not_the_topmost_popup error. Also fail a client who destroys a popup that is not the top most one.
Have we submitted xdg-shell v5 upstream yet?
Yes, I sent it yesterday: http://lists.freedesktop.org/archives/wayland-devel/2015-February/019943.html
Review of attachment 296736 [details] [review]: I don't see any reason to do this if xdg-shell is going to be solidified before Weston 1.8.
Review of attachment 296737 [details] [review]: Sure.
Review of attachment 296738 [details] [review]: OK. ::: src/wayland/meta-wayland-surface.c @@ +85,3 @@ + else + { + wl_resource_post_error(error_resource, error_code, Missing space. @@ +718,2 @@ surface->compositor = compositor; + surface->role = META_WAYLAND_SURFACE_ROLE_NONE; No need to explicitly initialize to 0.
Review of attachment 296739 [details] [review]: OK.
Review of attachment 296740 [details] [review]: ::: src/wayland/meta-wayland-pointer.c @@ +846,3 @@ } + +guint32 Don't use guint32 in newly written code. @@ +847,3 @@ + +guint32 +meta_wayland_pointer_get_grab_serial (MetaWaylandPointer *pointer) I'd much prefer a gboolean meta_wayland_pointer_can_popup (MetaWaylandPointer *pointer, uint32_t serial);
Review of attachment 296741 [details] [review]: Can we go without this for now? My long-term wish is actually to demolish the grab system and use the event route to do a similar thing.
Review of attachment 296742 [details] [review]: Looks OK.
Review of attachment 296743 [details] [review]: Sure.
(In reply to Jasper St. Pierre from comment #16) > Review of attachment 296741 [details] [review] [review]: > > Can we go without this for now? My long-term wish is actually to demolish > the grab system and use the event route to do a similar thing. Wouldn't we still need something grab like, except that it will use something else than the existing grab system underneath? I don't see how this patch changes that and we'll need a controller object for a popup "grab" anyway as I assume we don't want to poke manually at routes all over the place. I don't think it makes sense to cram such logic into meta-wayland-pointer.c, which is why I moved the popup part out of it, so I'd argue that we should not go without this one.
What I meant was MetaWaylandPointerGrab. I want that to die at some point.
(In reply to Jasper St. Pierre from comment #20) > What I meant was MetaWaylandPointerGrab. I want that to die at some point. Yes I understand that, but this patch doesn't expose it anywhere new, it just exposes the MetaWaylandPopupGrab which just happens to have a MetaWaylandPointerGrab. We'd still need something like MetaWaylandPopupGrab (or MetaWaylandPopupChain or whatever) which is the intention. The option here (to be able to do what the following of the patches do) is to just move all of this into meta-wayland-pointer.c which I don't think makes any sense.
Why can't MetaWaylandPopupGrab be on the surface itself? Anyway, I don't really care, we'll clean this up later in any case. Consider it ACN.
(In reply to Jasper St. Pierre from comment #22) > Why can't MetaWaylandPopupGrab be on the surface itself? Because a MetaWaylandPopupGrab spans over multiple surfaces. I suppose it could be owned by the top level surface or something. > > Anyway, I don't really care, we'll clean this up later in any case. Consider > it ACN. Ok.
Attachment 296737 [details] pushed as f6869bb - wayland: Update to xdg-shell unstable version 5 Attachment 296738 [details] pushed as 945bf62 - wayland: Introduce MetaWaylandSurfaceRole Attachment 296739 [details] pushed as f328890 - wayland: Fail when popup parent does not have an allowed role Attachment 296740 [details] pushed as f5c65d9 - wayland: Check the serial when creating popups Attachment 296742 [details] pushed as be77874 - wayland: Unmap popup windows when a popup chain is dismissed Attachment 296743 [details] pushed as 9a99a80 - wayland: Fail clients who try to create or destroy a not-top-most popup