GNOME Bugzilla – Bug 729215
Please implement Wayland subsurfaces
Last modified: 2014-08-26 11:03:12 UTC
Created attachment 275441 [details] [review] gdkwayland-subsurfaces.diff In order to provide the same level of functionalities with GDK-Wayland that we have with GDK-X11/Win32, we need a way to draw in a GTK+ window using an external backend (such as OpenGL, SDL...) and keep the drawing area consistent in terms of location, size... as we move the GTK+ window around. Real-world examples : GL application (GtkGLgears), media players, video games. Wayland subsurfaces allow that. Please consider this patch, which implements this workflow : gdk_wayland_window_set_use_custom_subsurface (window, <x>, <y>); struct wl_surface *surf = gdk_wayland_window_get_subsurface (window). *and draw to the wl_surface (PS : it has been said it would be possible to achieve that functionality by other means ; that needs more details, though).
Currently in gtk, there is only one wl_surface per toplevel GtkWidget. This patch only adds the possibility to glue a wl_surface to the gtk one. Then the gtk toolkit does not know how to deal with it so that when the layout changes (ex: resizing), it will not work properly. I think the subsurface should be created when calling gdk_window_ensure_native (gdkwindow), unless this is already the GdkWindow of the toplevel GtkWidget. Then make positioning and resizing managed internally by the gtk toolkit, accordling to its parent widget. This way the user would not even know that gtk has created a subsurface. And if really needed he would still be able to retrieve the wl_surface pointer thanks to "gdk_wayland_window_get_wl_surface". But your patch it's a start, thx for that :)
void gdk_wayland_window_set_use_custom_subsurface (GdkWindow *window, gint x, gint y); struct wl_surface *gdk_wayland_window_get_subsurface (GdkWindow *window); Can a surface only have a single subsurface ? And what is 'custom' about the subsurface ?
I don't like this API here. Really, subsurfaces match child windows, so I'd like to see this match gdk_window_move. get_wl_subsurface should retrieve the wl_subsurface; we already have a gdk_wayland_window_get_wl_surface API that we can use. We also need to look at making it work with pointer focus. The compositor will send a leave/enter pair when the pointer enters a subsurface, so we need to make sure that the surface is properly tracked with the toplevel. That means that the user_data is set properly, so we should reuse the existing code that creates the wl_surface.
I'm attaching a few alternative patches that implement this as a separate GdkWindowType. Given timing/schedule restrictions, and with an eye into fixing bug #695504, I've taken a couple of stop-gap solutions in the patches: - GDK_WINDOW_SUBSURFACE windows are only honored in wayland so far, other backends will chicken out. So far every usage should be surrounded by GDK_IS_WAYLAND_DISPLAY checks. - these windows (ab)use the transient_for window's surface as the parent when creating the subsurface, instead of gdk_window_get_parent(), this is purely convenient for the time being as the most glaring cases in need for subsurfaces in the gtk/ side are currently implemented through GTK_WINDOW_POPUP windows, and this allows us to do the quick change by changing the window type and setting a transient.
Created attachment 284189 [details] [review] wayland: Make toplevels' X/Y coordinates be 0 To all effects each window has its own "root" coordinates system, so set toplevels at 0,0 in that coordinate system, so root coordinate calculations are locally right.
Created attachment 284190 [details] [review] gdk: Add GDK_WINDOW_SUBSURFACE window type This window type can only be used on wayland so far, so NULL is returned if it's attempted to be used on any other windowing backend.
Created attachment 284191 [details] [review] wayland: Acquire wl_subcompositor interface This will be needed for GDK_WINDOW_SUBSURFACE windows.
Created attachment 284192 [details] [review] wayland: create a wl_subsurface interface for GDK_WINDOW_SUBSURFACE windows This subsurface is currently dependent on the transient_for parent, so the subsurface is repositioned relative to it.
Review of attachment 284189 [details] [review]: Yep.
Review of attachment 284190 [details] [review]: OK.
Review of attachment 284191 [details] [review]: Sure.
Review of attachment 284192 [details] [review]: ::: gdk/wayland/gdkwindow-wayland.c @@ +1167,3 @@ } + g_clear_pointer (&impl->subsurface, wl_subsurface_destroy); I don't like doing protocol operations in g_clear_pointer. Can you write this as an explicit statement? @@ +1254,3 @@ + + if (impl->subsurface) + wl_subsurface_set_position (impl->subsurface, x, y); Can you move this into the TOPLEVEL check below for clarity? We shouldn't ever hit this on a toplevel. @@ +1554,3 @@ gdk_wayland_window_sync_parent (window); + + g_clear_pointer (&impl->subsurface, wl_subsurface_destroy); Same comment as above.
Review of attachment 284190 [details] [review]: can you add some documentation for the new window type ? Could just say something like "Subsurface-based window. Currently only supported on Wayland."
I've applied the suggestions and added some docs for GDK_WINDOW_SUBSURFACE. Attachment 284189 [details] pushed as e206b72 - wayland: Make toplevels' X/Y coordinates be 0 Attachment 284190 [details] pushed as 84414f0 - gdk: Add GDK_WINDOW_SUBSURFACE window type Attachment 284191 [details] pushed as ad9da99 - wayland: Acquire wl_subcompositor interface Attachment 284192 [details] pushed as ab6f771 - wayland: create a wl_subsurface interface for GDK_WINDOW_SUBSURFACE windows