GNOME Bugzilla – Bug 756496
wayland: Make it possible to use a window type hint to map as a subsurface
Last modified: 2015-10-18 13:34:22 UTC
This is needed if one wants to be able to create specialized windows like ones own tooltip window using GtkWindow, without having to have per winsys conditions in the application code. An example of where this is useful is Firefox.
Created attachment 313179 [details] [review] wayland: Clean up code determining how to map a window Restructure the mapping procedure so that its known up front what the expected way mapping is to be done (subsurface, popup or stand alone), and warn if it fails to actually map in such a way (for example a popup without a parent or device grab, a tooltip without a parent).
Created attachment 313180 [details] [review] wayland: Map windows with tooltip hint as subsurfaces Tooltips tend to be placed on top of a parent surface with a given relative coordinate, and without any input focus. So lets map them as subsurfaces.
Created attachment 313181 [details] [review] GtkWindow: Enlarge the type hint private field Make it what it is - the enum - so that that it is sure that the hint will fit in the field. Without this, any hint that doesn't fit in 3 bits will be truncated to the 3 least significant bits, causing unexpected behaviour.
Comment on attachment 313181 [details] [review] GtkWindow: Enlarge the type hint private field Embarrasing. Please push :)
Review of attachment 313179 [details] [review]: Nice refactor overall, the warnings will be handy for future cases, and it's clearer about which window gets which wayland roles. ::: gdk/wayland/gdkwindow-wayland.c @@ +1329,3 @@ + window); + + create_fallback = TRUE; It indeed makes sense to fall back to xdg_surface. I wonder if this function overall wouldn't be clearer if we had a window_has_role() that returns false if a GdkWindow has no xdg_surface, xdg_popup or wl_subsurface. That way we could defer the fallback create_xdg_surface() call to a common place at the end of the function, instead of being called from several places. @@ +1903,3 @@ gdk_wayland_window_sync_parent (window); + if (should_map_as_subsurface (window)) here you dropped the visibility/parent checks, aren't they necessary anymore? it looks to me like we could trigger untimely wl_subsurface creation (eg. when the window is not visible yet)
Review of attachment 313180 [details] [review]: This one makes sense. You can also drop now the gtk_window_set_use_subsurface() call in gtktooltip.c. Unfortunately we can't remove yet the last one use of that private function in gtkentrycompletion.c, the window there has type hint COMBO, but xdg_popup makes no sense there (we don't want the grab to take away keyboard focus from the parent for starters)
Comment on attachment 313181 [details] [review] GtkWindow: Enlarge the type hint private field Attachment 313181 [details] pushed as 364732f - GtkWindow: Enlarge the type hint private field
Review of attachment 313179 [details] [review]: ::: gdk/wayland/gdkwindow-wayland.c @@ +1329,3 @@ + window); + + create_fallback = TRUE; Hmm, Good that you pointed this out, because it made me see that we sometimes shouldn't actually create a fallback (for example like above, for subsurfaces, we also sometimes create it when the transiency relationship is made). We do the "create_fallback" bool path function-wide though.
Review of attachment 313179 [details] [review]: ::: gdk/wayland/gdkwindow-wayland.c @@ +1329,3 @@ + window); + + create_fallback = TRUE; Or, actually, not really because we only use a fallback for the popup case.
Created attachment 313236 [details] [review] wayland: Clean up code determining how to map a window Restructure the mapping procedure so that its known up front what the expected way mapping is to be done (subsurface, popup or stand alone), and warn if it fails to actually map in such a way (for example a popup without a parent or device grab, a tooltip without a parent).
Comment on attachment 313236 [details] [review] wayland: Clean up code determining how to map a window Looks good! You're right about subsurfaces not getting a fallback xdg_surface, and my point is slighly moot if xdg_surface doesn't end in the else{} branch of each conditional.
Attachment 313180 [details] pushed as f838743 - wayland: Map windows with tooltip hint as subsurfaces Attachment 313236 [details] pushed as 9fe40f9 - wayland: Clean up code determining how to map a window