GNOME Bugzilla – Bug 620377
Relayout on monitor layout changes
Last modified: 2010-06-04 13:21:13 UTC
See patch.
Created attachment 162556 [details] [review] Relayout on monitor layout changes Currently we only relayout when the screen size changes, this gets the cases where a monitor gets added/removed but not when the primary monitor changes. We need to relayout on all monitor layout changes.
Created attachment 162562 [details] [review] Relayout on monitor layout changes *) We need to do this for the messageTray too.
Review of attachment 162562 [details] [review]: Hmm. - If we remove the only two connections to ShellGlobal::screen-size-changed, then we should remove the signal as well. - I don't like scattering the assumption that the screen is the default GDK screen over the code - I'd rather see an extra property on ShellGlobal to get the screen (even if it internally just does gdk_screen_get_default() for now) - Reading the GDK code (wanted to figure out if monitors-changed was emitted once per change), I was very confused by the snippet: #ifdef HAVE_RANDR display_x11 = GDK_DISPLAY_X11 (gdk_screen_get_display (screen)); if (display_x11->have_randr13 && event->type == ConfigureNotify) { g_signal_emit_by_name (screen, "monitors-changed"); return; } XRRUpdateConfiguration (event); #else Do you have any idea what that top branch is about? I also don't see any handling for primary monitor changing in the GDK code - it seems to be initialized once at startup and then not changed again. ::: js/ui/main.js @@ +160,3 @@ background.lower_bottom(); + let GdkScreen = Gdk.Screen.get_default(); a variable should have a lower-case g - 'gdkScreen'
(In reply to comment #3) > Review of attachment 162562 [details] [review]: > > Hmm. > > - If we remove the only two connections to ShellGlobal::screen-size-changed, > then we should remove the signal as well. Yeah I thought about that too but OTOH just because we have no users now does not imply that one might not want to use this signal. But as we can simply use gdk anyway it does indeed make sense to remove it. > - I don't like scattering the assumption that the screen is the default GDK > screen over the code - I'd rather see an extra property on ShellGlobal to get > the screen (even if it internally just does gdk_screen_get_default() for now) OK this makes sense. > - Reading the GDK code (wanted to figure out if monitors-changed was emitted > once per change), I was very confused by the snippet: > > > #ifdef HAVE_RANDR > display_x11 = GDK_DISPLAY_X11 (gdk_screen_get_display (screen)); > > if (display_x11->have_randr13 && event->type == ConfigureNotify) > { > g_signal_emit_by_name (screen, "monitors-changed"); > return; > } > > XRRUpdateConfiguration (event); > #else > > Do you have any idea what that top branch is about? Not quite sure but this looks odd ... It might be bug in GDK which results into GdkScreen::size-changed never gets emitted when the driver supports xrandr >= 1.2 (this is what have_randr13 means). This might have supposed to mean "size-changed" and not "monitors-changed" ... RRSetOutputPrimary sends both a ConfigureNotify and a RROutputChangeNotify event, which both gets passed to _gdk_x11_screen_size_changed (the later only if HAVE_RANDR is defined). So it might be an optimization to avoid calling process_monitors_change twice ... will check with Matthias what this is about. > I also don't see any handling for primary monitor changing in the GDK code - it > seems to be initialized once at startup and then not changed again. process_monitors_change() calls init_multihead() which either sets the primary monitor by calling init_randr13() or set it to 0 in the fallback path. > ::: js/ui/main.js > @@ +160,3 @@ > background.lower_bottom(); > > + let GdkScreen = Gdk.Screen.get_default(); > > a variable should have a lower-case g - 'gdkScreen' OK
Created attachment 162658 [details] [review] Relayout on monitor layout changes Currently we only relayout when the screen size changes, this gets the cases where a monitor gets added/removed but not when the primary monitor changes. We need to relayout on all monitor layout changes. Remove ShellGlobal::screen-size-changed signal as it is no longer used, Gdk is used to track changes now. A ShellGlobal::gdk-screen property is added for this purpose.
Review of attachment 162658 [details] [review]: ::: js/ui/main.js @@ +161,3 @@ + let gdkScreen = global.get_gdk_screen(); + gdkScreen.connect('monitors-changed', _relayout); The point of suggesting adding the property was to use it from JS! global.gdk_screen.connect(...) ::: src/shell-global.c @@ +424,3 @@ + * shell_global_get_gdk_screen: + * + * Return value: (transfer none): The default #GdkScreen I wouldn't document it this way. (Yeah, the docs for shell_global_get_screen() are wrong too.) It's the shell's GdkScreen. Property docs are fine. @@ +451,2 @@ static gboolean +update_screen_size (gpointer data) Danger! two different boolean return value meanings; as it was previously: update_screen_size: boolean is whether it was changed on_screen_size_changed_cb: boolean is whether to keep calling the MetaLater or to stop calling it - so always should return false. ::: src/shell-global.h @@ +39,3 @@ MetaScreen *shell_global_get_screen (ShellGlobal *global); +GdkScreen *shell_global_get_gdk_screen (ShellGlobal *global); Duplicate getter here is currently necessary, but I think it's better to leave it rather than to have the duplication for MetaScreen and not GdkScreen. (We added the duplication for MetaScreen because the GObject property is a real pain to use from C.) So it's good this way. shell_global_get_primary_monitor() should use the getter.
(In reply to comment #6) > Review of attachment 162658 [details] [review]: > > ::: js/ui/main.js > @@ +161,3 @@ > > + let gdkScreen = global.get_gdk_screen(); > + gdkScreen.connect('monitors-changed', _relayout); > > The point of suggesting adding the property was to use it from JS! > global.gdk_screen.connect(...) OK > ::: src/shell-global.c > @@ +424,3 @@ > + * shell_global_get_gdk_screen: > + * > + * Return value: (transfer none): The default #GdkScreen > > I wouldn't document it this way. (Yeah, the docs for shell_global_get_screen() > are wrong too.) It's the shell's GdkScreen. Property docs are fine. OK > @@ +451,2 @@ > static gboolean > +update_screen_size (gpointer data) > > Danger! two different boolean return value meanings; as it was previously: > > update_screen_size: boolean is whether it was changed > on_screen_size_changed_cb: boolean is whether to keep calling the MetaLater or > to stop calling it - so always should return false. Huh? I did change it to always return false for exactly this reason. > ::: src/shell-global.h > @@ +39,3 @@ > MetaScreen *shell_global_get_screen (ShellGlobal *global); > > +GdkScreen *shell_global_get_gdk_screen (ShellGlobal *global); > > Duplicate getter here is currently necessary, but I think it's better to leave > it rather than to have the duplication for MetaScreen and not GdkScreen. (We > added the duplication for MetaScreen because the GObject property is a real > pain to use from C.) So it's good this way. > > shell_global_get_primary_monitor() should use the getter. OK
> > @@ +451,2 @@ > > static gboolean > > +update_screen_size (gpointer data) > > > > Danger! two different boolean return value meanings; as it was previously: > > > > update_screen_size: boolean is whether it was changed > > on_screen_size_changed_cb: boolean is whether to keep calling the MetaLater or > > to stop calling it - so always should return false. > > Huh? > I did change it to always return false for exactly this reason. Sorry, my mistake. Missed that in the patch.
Created attachment 162660 [details] [review] Relayout on monitor layout changes *) Use the property from JS *) Fix doc *) Use the getter in shell_global_get_primary_monitor()
Review of attachment 162660 [details] [review]: Looks good to me
Attachment 162660 [details] pushed as ccbf247 - Relayout on monitor layout changes