After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 759229 - wayland: zoom window not working on Wayland
wayland: zoom window not working on Wayland
Status: RESOLVED FIXED
Product: gucharmap
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gucharmap maintainers
gucharmap maintainers
Depends on:
Blocks:
 
 
Reported: 2015-12-09 08:43 UTC by Olivier Fourdan
Modified: 2016-01-08 17:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
chartable: fix zoom window on Wayland (1.53 KB, patch)
2015-12-09 08:47 UTC, Olivier Fourdan
none Details | Review
Updated patch to use the gdk API for creating subsurface (5.18 KB, patch)
2015-12-18 08:48 UTC, Olivier Fourdan
none Details | Review
[PATCH v3] chartable: fix zoom window on Wayland (1.44 KB, patch)
2016-01-08 12:18 UTC, Olivier Fourdan
committed Details | Review

Description Olivier Fourdan 2015-12-09 08:43:27 UTC
Description:

On Wayland, popup windows are for menu type widgets and thus the protocol requires a device grab, which gucharmap doesn't do.

As a result, the zoom window is dismissed instantly on Wayland.

Steps to reproduce:

1. Run gucharmap on Waylamd
2. Right click on a character

Actual result:

The zoom window appears for a fraction of a second and then disappears.

Expected result:

The zoom window works on Wayland like it does on X11.

Additional data:

Wayland has two different concepts that can be used for that, one is xdg-popup which requires a device grab, the other one is subsurface.

From the use case hre, I reckon best would be to use a subsurface for the zoom-window.

Bad news is that gtk+/gdk does not expose an API to force the backend to use a subsurface on Wayland. 

Good news is we can easily trick it to use a subsurface by using the tooltip window hint, all it requires to to have aprent which we can obtain with the transient property.

Patch to follow.
Comment 1 Olivier Fourdan 2015-12-09 08:47:17 UTC
Created attachment 317007 [details] [review]
chartable: fix zoom window on Wayland
Comment 2 Christian Persch 2015-12-10 17:33:27 UTC
> Bad news is that gtk+/gdk does not expose an API to force the
> backend to use a subsurface on Wayland. 

Wouldn't it be better to add that API then instead of depending on iternal details like that tooltip window hint => subsurface?
Comment 3 Olivier Fourdan 2015-12-11 07:10:05 UTC
The concept of subsurface is backend (Wayland) specific, I am not sure this would belong to a higher layer such as gdk (generic) or even less gtk+.

If we were to add an API for that, it would be only for Wayland, thus forcing applications to check for the actual windowing backend in use, which is not ideal either.
Comment 4 Olivier Fourdan 2015-12-11 16:42:43 UTC
Oh, btw, there is already an API (well, a window type, rather) for that, but as I said, it's only for Wayland so that means the application needs to be aware of the windowing system in use, which is equally suboptimal.

Beside, it adds more complexity to the code (for this to work you have to subclass GtkWindow and implement your own realize function that will override the gdk window type), but if this is really what you want, I can provide you with a patch that does this...
Comment 5 Olivier Fourdan 2015-12-18 08:48:01 UTC
Created attachment 317610 [details] [review]
Updated patch to use the gdk API for creating subsurface

I still think this is overly complex for little benefit, but that uses the existing gdk API for subsurface as requested (it also means it needs to check for the underlying windowing system in use, which I think is not well suited in high level applications).

But that's either that or attachment 317007 [details] [review]

Both work equally well here, on Wayland and X11 as far as I can tell.
Comment 6 Christian Persch 2015-12-20 19:51:49 UTC
Comment on attachment 317610 [details] [review]
Updated patch to use the gdk API for creating subsurface

A rather larger patch for something that with just the tiniest of gtk+ API could be a simple one-liner like

  priv->zoom_window = gtk_window_new (GTK_WINDOW_POPUP);
+ gtk_window_set_use_subsurface (GTK_WINDOW (priv->zoom_window), TRUE);

or

-  priv->zoom_window = gtk_window_new (GTK_WINDOW_POPUP);
+  priv->zoom_window = gtk_window_new (GTK_WINDOW_POPUP_WITH_SUBSURFACE);

but given the lack of such API, this is ok to commit. Thanks!
Comment 7 Olivier Fourdan 2015-12-21 07:39:57 UTC
Yes, I agree, this will become more and more common, let's put this patch on hold just the time I submit a patch for gtk+

I don't think we should expose the subsurface to gtk+ (as this is a Wayland thing, thus found in the gdk backend, far far deeper that gtk+ ^_~) *but* we could try to be smarter in that backend and use a subsurface when the popup is attached to a toplevel, for example.

I am not entirely sure yet how this is doable and what would be the potential side effects, so let's see how this goes on the gtk side of things first and I'll get back to this patch shortly.
Comment 8 Olivier Fourdan 2015-12-21 13:36:32 UTC
Something like what is discussed in bug 759738 would allow the fix to be a one-liner, we would just have to set the transient_for to the toplevel to get a subsurface that can be positioned at will, just like on X11.
Comment 9 Olivier Fourdan 2016-01-08 12:18:28 UTC
Created attachment 318481 [details] [review]
[PATCH v3] chartable: fix zoom window on Wayland

I have pushed a patch in gtk+ that makes gdk wayland backend select either a subsurface or an xdg-ppup for popup windows depending  if the popup is a transient for the main window (subsurface require this) or if a grab is active (xdg-popup requires a grab).

That's bug 759738 and attachment 318247 [details] [review] which was pushed as commit 28f011e in gtk+

Anyway, long story short, it means that to fix the chartable popup in gucharmap, we just need a one line patch with this, to attach the popup to the parent and gtk+/gdk wayland backend will do the "right thing".
Comment 10 Christian Persch 2016-01-08 16:03:21 UTC
Comment on attachment 318481 [details] [review]
[PATCH v3] chartable: fix zoom window on Wayland

Thanks!

This is harmless on X, and on gtk+ < 3.19, right?
Comment 11 Olivier Fourdan 2016-01-08 16:10:06 UTC
Yeap, right, harmless as in it won't do anything (basically, you'll need gtk+ 3.20)
Comment 12 Olivier Fourdan 2016-01-08 17:38:45 UTC
Comment on attachment 318481 [details] [review]
[PATCH v3] chartable: fix zoom window on Wayland

attachment 318481 [details] [review] pushed as commit cda5ee2 chartable: fix zoom window on Wayland