GNOME Bugzilla – Bug 745406
[PATCH] Make "size-changed" signal of GdkScreen more robust
Last modified: 2015-03-14 17:51:45 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).
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 ?
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.
(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...
(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?
(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].
(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.
(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.)
(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.
(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
(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.
(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.
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.
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).
The xfwm4 fix makes this a non-issue.