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 745406 - [PATCH] Make "size-changed" signal of GdkScreen more robust
[PATCH] Make "size-changed" signal of GdkScreen more robust
Status: RESOLVED NOTABUG
Product: gtk+
Classification: Platform
Component: Backend: X11
2.24.x
Other Linux
: Normal major
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2015-03-02 05:22 UTC by John Lindgren
Modified: 2015-03-14 17:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Make "size-changed" signal of GdkScreen more robust (2.58 KB, patch)
2015-03-02 05:22 UTC, John Lindgren
none Details | Review

Description John Lindgren 2015-03-02 05:22:18 UTC
Created attachment 298252 [details] [review]
Make "size-changed" signal of GdkScreen more robust

Changes in xfwm4 4.12 uncovered a bug in GDK where the "size-changed" signal for GdkScreen can be defeated if XRRUpdateConfiguration() is called outside of GDK.  Since xfwm4 relies on this signal to detect changes to the screen configuration, missing the signal does bad things for window placement (most notably, maximized windows either partially off-screen or not filling the whole screen).
Comment 1 Matthias Clasen 2015-03-02 11:19:06 UTC
Doesn't look right to me. What does 'defeated' mean here, exactly ? And what events does gdk see that do not cause it to emit a ::size-changed signal ?
Comment 2 John Lindgren 2015-03-02 13:20:11 UTC
What exactly doesn't look right?  The event in question is XRRScreenChangeNotifyEvent.  xfwm4 also listens to this event, via gdk_window_add_filter(), and calls XRRUpdateConfiguration(), which updates Xlib's idea of the screen size before GDK gets to process the event.  This means that the "old" screen size fetched by GDK on lines 1175-1176 is actually the new size.  GDK therefore assumes that the size hasn't changed and doesn't send the "size-changed" event.

IMHO, it isn't safe for GDK to assume that it is the only one calling XRRUpdateConfiguration().  To reliably detect a change in screen size, it's necessary to keep track of what GDK last thought the screen size was, and when that doesn't match the current size, then "size-changed" should be emitted.
Comment 3 Matthias Clasen 2015-03-02 13:24:00 UTC
(In reply to John Lindgren from comment #2)
> What exactly doesn't look right?  The event in question is
> XRRScreenChangeNotifyEvent.  xfwm4 also listens to this event, via
> gdk_window_add_filter(), and calls XRRUpdateConfiguration(), which updates
> Xlib's idea of the screen size before GDK gets to process the event.  This
> means that the "old" screen size fetched by GDK on lines 1175-1176 is
> actually the new size.  GDK therefore assumes that the size hasn't changed
> and doesn't send the "size-changed" event.

It doesn't look right to keep yet another window size in yet another place, when we already keep our idea of the window size in GdkWindow.

> IMHO, it isn't safe for GDK to assume that it is the only one calling
> XRRUpdateConfiguration().  To reliably detect a change in screen size, it's
> necessary to keep track of what GDK last thought the screen size was, and
> when that doesn't match the current size, then "size-changed" should be
> emitted.

No, I think it is perfectly safe to assume that. GTK+ is an application toolkit, not a wm toolkit. If you are using event filters, it is your responsibility to not mess up GDK's internal state...
Comment 4 John Lindgren 2015-03-02 13:34:52 UTC
(In reply to Matthias Clasen from comment #3)
> It doesn't look right to keep yet another window size in yet another place,
> when we already keep our idea of the window size in GdkWindow.

Seems appropriate to me to track the *screen* size in GdkScreen.  Unless you want to move the handling of XRRScreenChangeNotifyEvent to GdkWindow ...

> No, I think it is perfectly safe to assume that. GTK+ is an application
> toolkit, not a wm toolkit. If you are using event filters, it is your
> responsibility to not mess up GDK's internal state...

I would argue that Xlib state != GDK's internal state.  However, what exactly do you suggest that xfwm4 do differently?  Not use GDK at all and draw with Xlib directly?  Or split window decorations into a separate process from the compositor?
Comment 5 John Lindgren 2015-03-02 13:40:13 UTC
(In reply to Matthias Clasen from comment #3)
> It doesn't look right to keep yet another window size in yet another place,
> when we already keep our idea of the window size in GdkWindow.

FWIW, the Quartz backend already does exactly what I'm suggesting the X11 backend should do as well: track the screen size in GdkScreen[Quartz/X11].
Comment 6 John Lindgren 2015-03-02 13:46:30 UTC
(In reply to Matthias Clasen from comment #3)
> It doesn't look right to keep yet another window size in yet another place,
> when we already keep our idea of the window size in GdkWindow.

And GTK 3 tracks the screen size in GdkScreen as well.
Comment 7 John Lindgren 2015-03-02 13:59:47 UTC
(In reply to Matthias Clasen from comment #3)
> GTK+ is an application toolkit, not a wm toolkit.

What exactly do you mean by this, anyway?  GNOME's own WM, Metacity, used GDK/GTK for ages.  (Not sure what Mutter does nowadays.)
Comment 8 Matthias Clasen 2015-03-02 14:17:52 UTC
(In reply to John Lindgren from comment #7)
> (In reply to Matthias Clasen from comment #3)
> > GTK+ is an application toolkit, not a wm toolkit.
> 
> What exactly do you mean by this, anyway?  GNOME's own WM, Metacity, used
> GDK/GTK for ages.  (Not sure what Mutter does nowadays.)

That doesn't mean I have to be happy with it.
Comment 9 Emmanuele Bassi (:ebassi) 2015-03-02 14:46:51 UTC
(In reply to John Lindgren from comment #7)
> (In reply to Matthias Clasen from comment #3)
> > GTK+ is an application toolkit, not a wm toolkit.
> 
> What exactly do you mean by this, anyway?  GNOME's own WM, Metacity, used
> GDK/GTK for ages.  (Not sure what Mutter does nowadays.)

That's not entirely correct. Metacity used GDK for some things, but in general it used Xlib for handling the whole . It did use GTK, but only for drawing menus, and even that has been removed to avoid collisions with Wayland.

In general, doing things with X11 API behind GDK's back is frowned upon, when not actively unsupported. If you are already using XRR* API, why not keep track of the screen geometry inside XFWM? Metacity/Mutter already does that as well: https://git.gnome.org/browse/mutter/tree/src/backends/x11/meta-monitor-manager-xrandr.c
Comment 10 John Lindgren 2015-03-02 15:42:23 UTC
(In reply to Emmanuele Bassi (:ebassi) from comment #9)
> In general, doing things with X11 API behind GDK's back is frowned upon,
> when not actively unsupported. If you are already using XRR* API, why not
> keep track of the screen geometry inside XFWM?

You seem to be saying two conflicting things: using the X11 API together with GDK is frowned upon because it can break GDK, therefore the fix is to use more of the X11 API and ignore the bad effects on GDK?

Unless there are serious side effects to doing so, I would rather make GDK less fragile than debate about what clients need to do to avoid breaking it.  I guess this is a design question which should be discussed with the xfwm4 developers, not with me.  I came at this issue looking for a simple solution, and fixing the "size-changed" logic still appears to me to be the cleanest, least intrusive fix.

That's all from me ... xfwm4 4.12 has been released and is broken due to this bug, so I expect that either Olivier will implement a workaround on his side, or you will be hearing more from him about the issue.
Comment 11 Emmanuele Bassi (:ebassi) 2015-03-02 15:49:56 UTC
(In reply to John Lindgren from comment #10)
> (In reply to Emmanuele Bassi (:ebassi) from comment #9)
> > In general, doing things with X11 API behind GDK's back is frowned upon,
> > when not actively unsupported. If you are already using XRR* API, why not
> > keep track of the screen geometry inside XFWM?
> 
> You seem to be saying two conflicting things: using the X11 API together
> with GDK is frowned upon because it can break GDK, therefore the fix is to
> use more of the X11 API and ignore the bad effects on GDK?

No: I'm saying you should not mix GDK and X11 usage; either use GDK only, or use X11 only. See, not conflicting at all.

GDK maintains its own internal state; if you use X11 API behind its back, the internal GDK state *may* go out of sync. This is undefined behaviour, as far as the GTK developers are concerned. We *may* fix bugs that are caused by using X11 API alongside GDK, but we'd rather people fix their own code. Especially because GDK is meant to be used in applications, not inside system components like a window manager.

> Unless there are serious side effects to doing so, I would rather make GDK
> less fragile than debate about what clients need to do to avoid breaking it.

Perfectly fine attitude. The GTK developers are, on the other hand, perfectly entitled to tell you that you're using their API in the wrong way.

> That's all from me ... xfwm4 4.12 has been released and is broken due to
> this bug, so I expect that either Olivier will implement a workaround on his
> side, or you will be hearing more from him about the issue.

We are happy to discuss long term solutions with the XFWM maintainers.
Comment 12 Matthias Clasen 2015-03-02 15:55:19 UTC
Can we modify the patch to keep the last known config timestamp, instead of the size ? That would make it a lot more palatable to me.
Comment 13 Olivier Fourdan 2015-03-11 08:22:04 UTC
If that helps, I think I've pushed a workaround in xfwm4, I agree it doesn't need to update RR config (I don't see why it did).
Comment 14 John Lindgren 2015-03-14 17:51:45 UTC
The xfwm4 fix makes this a non-issue.