GNOME Bugzilla – Bug 715029
Ensure GdkScreen::monitors-changed gets emitted after the Screen dimensions are updated
Last modified: 2013-11-25 16:58:32 UTC
In the x11 code, _gdk_x11_screen_size_changed() is called when receiving Configure events on the root window, or XRandR events, so when plugging/unplugging a monitor, it is called multiple times, so process_monitors_change() does quite some filtering to ensure the signal is just emitted once. But this seems to bring in problems wrt gdk_screen_get_width/height() when unplugging a monitor, as the signal is emitted early in the event burst, early enough for Width/HeightOfScreen (used underneath those functions) to still be unaware of the final size, so these functions return the previous size if called within a callback. This has been seen on the wacom plugin in gnome-settings-daemon. When it receives that signal, it tries to remap tablets without a monitor mapping to the whole screen, so dimensions come out wrong. As there is no clear beginning or end in that bunch of events, in the patch I'm attaching I resorted to just caching width/height, and emitting the signal again when that changes, even if it's emitted twice for a single physical event...
Created attachment 261248 [details] [review] x11: ensure GdkScreen::monitors-changed is emitted after the screen size changes When a monitor is connected or disconnected, a bunch of configure events on the root window and XRandR events will notify GDK that the Screen changed its configuration, which the GdkX11Screen code filters to great extent, just trying to spawn that signal once. However, when a monitor is disconnected, the first events that GDK gets trigger the signal before Width/HeightOfScreen() are aware of the final dimensions. So just trigger the signal again when those values change.
We should definitively emit a ::size-changed after the new size is in place. Its not clear to me why that doesn't work with the current code. Can you list the events we actually get in this case, and the signals we emit ?
Ok... so I noticed there's some race condition when plugging a monitor too, just less frequent When plugging a monitor, I most usually get this: RRScreenChangeNotify (size: 1366,768) RRScreenChangeNotify (size: 3046,1050) <-- Emits the signal (n_monitors:2, size:3046,1050) Configure (coords: 0,0,3046,1050) RRScreenChangeNotify (size: 3046,1050) RRNotify (subtype: RRNotify_CrtcChange) Configure (coords: 0,0,3046,1050) RRScreenChangeNotify (size: 3046,1050) RRNotify, (subtype: RRNotify_CrtcChange) Configure (coords: 0,0,3046,1050) RRNotify (subtype: RRNotify_OutputProperty) RRNotify (subtype: RRNotify_OutputProperty) But in the race condition case, plugging a monitor looks like: RRScreenChangeNotify (size: 1366,768) <-- Emits the signal (n_monitors:2, size:1366,768) RRScreenChangeNotify (size: 3046,1050) Configure (coords: 0,0,3046,1050) RRScreenChangeNotify (size: 3046,1050) RRNotify (subtype: RRNotify_CrtcChange) Configure (coords: 0 0 3046 1050) RRScreenChangeNotify (size: 3046,1050) RRNotify (subtype: RRNotify_CrtcChange) Configure (coords: 0,0,3046,1050) RRNotify (subtype: RRNotify_OutputProperty) RRNotify (subtype: RRNotify_OutputProperty) And unplugging a monitor seems to be quite invariably: RRNotify (subtype: RRNotify_OutputProperty) <-- Emits the signal (n_monitors:1, size:3046,1050) RRScreenChangeNotify (size: 3046,1050) RRScreenChangeNotify (size: 3046,1050) RRNotify (subtype: RRNotify_CrtcChange) Configure (coords: 0,0,3046,1050) RRScreenChangeNotify (size: 3046,1050) RRNotify (subtype: RRNotify_CrtcChange) Configure (coords: 0,0,3046,1050) RRScreenChangeNotify (size: 1366,768) Configure (coords: 0,0,1366,768) RRScreenChangeNotify (size: 1366,768) RRNotify (subtype: RRNotify_CrtcChange) Configure (coords: 0,0,1366,768) RRNotify (subtype: RRNotify_OutputProperty) It is worth noting too that process_monitors_change() in GdkX11Screen dumps all previous monitor information and rebuilds the monitor list from randr, so there's a brief lapse when randr and core functions are in disagreement.
one idea I had: when reconstructing the monitor information, we could update the screen size to be the bounding box of the monitors - that should almost always be correct, and would probably cover up the temporary disagreement. what do you think ?
That doesn't sound like a bad idea, it wouldn't the first place at all where we ditch Xlib.h info in favor of extensions' :). I'm attaching a patch that does that, from my testing it works all fine. Even for the race condition when plugging monitors that I mentioned in comment #3, I realized both monitors are first reported at a 0,0 offset, but right afterwards one is rearranged, which changes the bounding box, and the signal is re-emitted again in that case. One thing that could still be off though under the same circumstances is the millimeters API, and this approach wouldn't work as well with that API as the support for mm sizes varies across the supported multi* implementations, but it's not like it's the only brokenness underlying there... and it should be more of an strange case too.
Created attachment 261450 [details] [review] x11: keep track of the screen pixel size by calculating the bounding box of monitors This is so we always have the latest information given by XRandR (or other), and not rely on Core protocol information that might not have been updated yet. This is specially visible when a monitor is connected (less frequent) or disconnected (much more frequent), callbacks on GdkScreen::monitors-changed that call gdk_screen_get_width/height() could get the screen size previous to the monitor rearrangement. So in order to fix this, keep track of the latest monitors information, and calculate the bounding box in order to know the screen size. Also, make that a factor to re-emit GdkScreen::monitors-changed if necessary.
Review of attachment 261450 [details] [review]: ::: gdk/x11/gdkscreen-x11.c @@ +1219,3 @@ x11_screen->primary_monitor != primary_monitor; + changed |= update_bounding_box (screen); I disagree with this line. The changed boolean is about whether the monitor information changed, and we should emit ::monitors-changed. If the bounding box update changes the screen dimension, we should instead emit ::size-changed. We already have a check for the screen dimensions in the caller of process_monitors_change, so it should be fine to just make update_bounding_box return void. I think maybe this should instead be if (changed) update_bounding_box (screen); since there is no point in doing this extra work if the monitors haven't actually changed.
Created attachment 261464 [details] [review] x11: keep track of the screen pixel size by calculating the bounding box of monitors This is so we always have the latest information given by XRandR (or other), and not rely on Core protocol information that might not have been updated yet. This is specially visible when a monitor is connected (less frequent) or disconnected (much more frequent), callbacks on GdkScreen::monitors-changed that call gdk_screen_get_width/height() could get the screen size previous to the monitor rearrangement. So in order to fix this, keep track of the latest monitors information, and calculate the bounding box in order to know the screen size.
Review of attachment 261464 [details] [review]: looks good now, thanks
Great :), the patch is now on master, 3.10, and 3.8.