GNOME Bugzilla – Bug 769936
wayland: Port xdg_shell implementation to unstable v6
Last modified: 2016-08-26 08:57:14 UTC
The to be attached patches ports the xdg-shell implementation in mutter from xdg-shell unstable v5 to xdg-shell unstable v6. This is a backward incompatible change, thus for clients that use xdg-shell unstable v5 to work, they need to port as well. The clients distributed via weston 1.12 which is scheduled to be released next month will use xdg-shell unstable v6. Apart from mega patch that does the porting, gtk-shell is also changed to extend xdg-shell for the tiled state, as the ability to have arbitrary values passed via the state array is no longer allowed.
Created attachment 333341 [details] [review] src/Makefile.am: Pass protocol file to wayland-scanner as an argument Instead of piping the protocol file content to wayland-scanner, pass the file name as an argument. This enables a new enough wayland-scanner to print more meaningful error messages.
Created attachment 333342 [details] [review] MetaWaylandSurfaceRole: Set the surface instance on construction Set the MetaWaylandSurface instance pointer on construction so that all surface role relevant parameters are initialized when constructed.
Created attachment 333343 [details] [review] MetaWaylandSurface: Allow passing parameters when assigning role Allow passing parameters (only GObject parameters supported for now) so that role assignment can affect the paremeters set during construction. If a role was already assigned when assigning, the passed parameters are set using g_object_set_valist().
Created attachment 333344 [details] [review] window: Make meta_window_has_pointer() per protocol implemented
Created attachment 333345 [details] [review] core: Add support for custom window placement rules Add support for assigning a window a custom window placement rule used for calculating the initial window position as well as defining how a window is constrained. The custom rule is a declarative rule which defines a set of parameters which the placing algorithm and constrain algorithm uses for calculating the position of a window. It is meant to be used to implement positioning of menus and other popup windows created via Wayland. A custom placement rule replaces any other placement or constraint rule.
Created attachment 333346 [details] [review] wayland: Move out gtk_shell from meta-wayland-surface.c
Created attachment 333347 [details] [review] MetaWaylandSurface: Move 'destroying' field It is not related to the gtk_surface extension, so move it to other generic fields.
Created attachment 333348 [details] [review] wayland: Move gtk_surface fields out of MetaWaylandSurface Let the gtk_shell extension unit handle its own state.
Created attachment 333349 [details] [review] MetaWaylandSurface: Add 'configure' signal Emit a 'configure' signal before configuring the role. This will enable extensions to send its own configure events before the role is configured.
Created attachment 333350 [details] [review] wayland: Make gtk_shell handle our private window states Instead of using the "allocated" state ranges of xdg_shell, lets just use our own gtk_shell by adding a state enum and a configure event.
Created attachment 333351 [details] [review] wayland: Keep track of configured position Besides the configured dimension, also keep track of the configured position. This will later be used by popup position feedback.
Created attachment 333352 [details] [review] wayland: Call assigned() surface role vfunc when re-assigned This will later be used by xdg-shell to ensure no buffer is attached on assignment.
Created attachment 333353 [details] [review] wayland: Support passing gpointer's on role assignment This will later be used to pass non GObject instance pointers.
Created attachment 333354 [details] [review] wayland: Let shell surface role sync generic window state Instead of having each final role do the same call, lets just make the common role object deal with synchronizing window buffer size.
Created attachment 333355 [details] [review] wayland/xdg-shell: Make keyboard focus follow grabbing popup This is the explicitly intended keyboard focus symantics of xdg-shell unstable v6. The semantics are undefined in unstable v5.
Created attachment 333356 [details] [review] wayland/xdg-shell: Port to unstable v6 Port the xdg_shell implementation to use the unstable v6 protocol. This includes: - making xdg_surface a generic base interface for xdg_shell surface roles - create a xdg_toplevel role replacing the old xdg_surface - change the xdg_opup role to be based on xdg_surface - make xdg_popup not grab by default - add support for xdg_positioner
Created attachment 333357 [details] [review] xdg-shell: Don't early out of role commit before calling parent impl Make sure to always call the parent role commit vfunc, so that they can handle updating their state properly.
Related GTK+ patches are available at bug 769937. The patches are also available at https://github.com/jadahl/mutter/commits/wip/xdg-shell-v6 .
Comment on attachment 333341 [details] [review] src/Makefile.am: Pass protocol file to wayland-scanner as an argument LGTM, good to see that these (stdin) messages will go :)
Comment on attachment 333342 [details] [review] MetaWaylandSurfaceRole: Set the surface instance on construction Makes sense indeed
Review of attachment 333343 [details] [review]: The overall change makes sense to me. some comments though. ::: src/wayland/meta-wayland-surface.c @@ +179,3 @@ + object_class = g_type_class_ref (role_type); + + params = g_array_new (FALSE, FALSE, sizeof (GParameter)); Might be nicer to use g_array_set_clear_func() here so you don't need unset_params_values() later. That way you just need g_array_unref (params) after you're done with it. @@ +197,3 @@ + g_value_init (¶m.value, ptype); + + if (g_type_is_a (ptype, G_TYPE_OBJECT)) Looks like a place to use G_VALUE_COLLECT & co, that way you don't have to deal that much with mapping varargs to GValue content.
Comment on attachment 333344 [details] [review] window: Make meta_window_has_pointer() per protocol implemented Indeed!
Comment on attachment 333345 [details] [review] core: Add support for custom window placement rules LGTM
Comment on attachment 333346 [details] [review] wayland: Move out gtk_shell from meta-wayland-surface.c Makes sense to split this
Comment on attachment 333347 [details] [review] MetaWaylandSurface: Move 'destroying' field Uncontroversial :)
Comment on attachment 333348 [details] [review] wayland: Move gtk_surface fields out of MetaWaylandSurface Makes sense
Review of attachment 333349 [details] [review]: The change makes sense. A few comments though ::: src/wayland/meta-wayland-surface.c @@ +1593,3 @@ META_WAYLAND_SURFACE_ROLE_SHELL_SURFACE (surface->role); + g_signal_emit (surface, SURFACE_CONFIGURE, 0); I think you already fixed this locally :). @@ +1792,3 @@ + G_TYPE_FROM_CLASS (object_class), + G_SIGNAL_RUN_LAST, + 0, NULL, NULL, NULL, I think I'd prefer to use g_cclosure_marshal_VOID__VOID over NULL GSignalCMarshaller here.
Comment on attachment 333350 [details] [review] wayland: Make gtk_shell handle our private window states Makes sense to me.
Comment on attachment 333351 [details] [review] wayland: Keep track of configured position Looks good.
Comment on attachment 333352 [details] [review] wayland: Call assigned() surface role vfunc when re-assigned Actually seems like a leftover from the earlier patch adding vararg support, wouldn't mind this being merged with that one. a-c-n nonetheless
Comment on attachment 333353 [details] [review] wayland: Support passing gpointer's on role assignment I guess this one is moot if you use G_VALUE_COLLECT as advised earlier :)
Comment on attachment 333354 [details] [review] wayland: Let shell surface role sync generic window state Indeed
Comment on attachment 333355 [details] [review] wayland/xdg-shell: Make keyboard focus follow grabbing popup /me must work on the grabbing interface :)
Comment on attachment 333356 [details] [review] wayland/xdg-shell: Port to unstable v6 It looks correct to me.
Comment on attachment 333357 [details] [review] xdg-shell: Don't early out of role commit before calling parent impl I think there's a missing verb in the commit summary :). LGTM anyway.
Attachment 333341 [details] pushed as 3ee16a2 - src/Makefile.am: Pass protocol file to wayland-scanner as an argument Attachment 333342 [details] pushed as 2114f2e - MetaWaylandSurfaceRole: Set the surface instance on construction Attachment 333343 [details] pushed as 3b3f40a - MetaWaylandSurface: Allow passing parameters when assigning role Attachment 333344 [details] pushed as 9fb891d - window: Make meta_window_has_pointer() per protocol implemented Attachment 333345 [details] pushed as 8833991 - core: Add support for custom window placement rules Attachment 333346 [details] pushed as c73d3d9 - wayland: Move out gtk_shell from meta-wayland-surface.c Attachment 333347 [details] pushed as 335cd8e - MetaWaylandSurface: Move 'destroying' field Attachment 333348 [details] pushed as 3a75e55 - wayland: Move gtk_surface fields out of MetaWaylandSurface Attachment 333349 [details] pushed as cfb3d10 - MetaWaylandSurface: Add 'configure' signal Attachment 333350 [details] pushed as a8d86b4 - wayland: Make gtk_shell handle our private window states Attachment 333351 [details] pushed as 817911d - wayland: Keep track of configured position Attachment 333352 [details] pushed as a5efa30 - wayland: Call assigned() surface role vfunc when re-assigned Attachment 333354 [details] pushed as f21df37 - wayland: Let shell surface role sync generic window state Attachment 333355 [details] pushed as 24c3844 - wayland/xdg-shell: Make keyboard focus follow grabbing popup Attachment 333356 [details] pushed as ef3e036 - wayland/xdg-shell: Port to unstable v6 Attachment 333357 [details] pushed as dc03b3a - xdg-shell: Don't early out of role commit before calling parent impl
(In reply to Jonas Ådahl from comment #4) > Created attachment 333344 [details] [review] [review] > window: Make meta_window_has_pointer() per protocol implemented This is not correct: when running as a wayland compositor, you need to use the Clutter path even for X11 clients, because the X server does not know where the pointer is.
(In reply to Giovanni Campagna from comment #37) > (In reply to Jonas Ådahl from comment #4) > > Created attachment 333344 [details] [review] [review] [review] > > window: Make meta_window_has_pointer() per protocol implemented > > This is not correct: when running as a wayland compositor, you need to use > the Clutter path even for X11 clients, because the X server does not know > where the pointer is. You're right. The MetaWindow(Wayland|X11) represents the type of the client, not the mode the compositor runs in. Ugh, the lack of abstraction still in that direction still annoys me some times. I'll revert it.
Now I'm puzzled, because I explicitly checked that during, and saw a MetaWindowWayland when launching GDK_BACKEND=x11 gedit on a nested mutter.
(In reply to Carlos Garnacho from comment #39) > Now I'm puzzled, because I explicitly checked that during, and saw a > MetaWindowWayland when launching GDK_BACKEND=x11 gedit on a nested mutter. How did you test that? Could it be that you happened to test the gedit "service" that is D-Bus activated and doesn't see the environment you set? Just did this anyhow just to verify: $ gdb mutter --nested (gdb) b meta_window_set_title (gdb) run $ GDK_BACKEND=x11 gtk3-demo then when gdb breaks: (gdb) print g_type_name_from_instance(window) $2 = (const gchar *) 0x7ffff7b9609c "MetaWindowX11"
Nah, ignore me, I indeed see that now, I maybe typoed the GDK_BACKEND envvar or something, that's the only explanation I have.