GNOME Bugzilla – Bug 608647
picks the wrong monitor to start on
Last modified: 2010-05-12 07:36:22 UTC
Ideally we'd have a way to pick the primary monitor but until we do we should probably make sure that the top menu bar starts on the same one that the gnome-panel would have used. I have my laptop connected to my TV. gnome-panel starts on the internal and shell starts on the TV.
ok, gtk master has gdk_screen_get_primary_monitor(), which returns the XRandR "primary monitor", which you could set with the display capplet, if you had the patch that Matthias forgot to attach to bug 588041. the panel also uses some additional heuristics (like looking for an output named "LVDS") if you haven't set the primary with xrandr. gdk_screen_get_primary_monitor() does not do this, but we should probably fix it to. we probably want to just import the code from gnome-panel for now anyway though since it will be a while before most people have a new enough gtk.
FWIW, this isn't just at initial startup. If I connect my TV while the shell is already running on the internal LCD then the top bar moves over to the TV.
Created attachment 157889 [details] [review] Rework shell_global_get_primary_monitor Currently shell_global_get_primary_monitor just returns the first screen, in the list as primary. This is not always correct as the first screen reported by mutter isn't always, the first one listed by RANDR. Use gdk_screen_* to query the monitor information and add a heuristic to prefer LVDS displays (similar like in done for gnome-panel) to prefer the laptop's internal screen over external displays.
Created attachment 157897 [details] [review] Rework shell_global_get_primary_monitor *) Fix case where the driver does not support XRANDR => 1.2
Review of attachment 157897 [details] [review]: ::: src/shell-global.c @@ +1170,1 @@ + for (i = 0; i < gdk_screen_get_n_monitors (screen); i++) This is a really minor thing, but I'd hoist the call to gdk_screen_get_n_monitors (screen) outside of the loop by creating a local variable; this is what the GTK+ uses of it do. It's currently just a field lookup but possibly in the future it could be more expensive. @@ -1170,3 @@ - G_STRUCT_OFFSET (MetaRectangle, width) == G_STRUCT_OFFSET (GdkRectangle, width) && - G_STRUCT_OFFSET (MetaRectangle, x) == G_STRUCT_OFFSET (GdkRectangle, x) && - g_assert (sizeof (MetaRectangle) == sizeof (GdkRectangle) && Why did you remove this assertion? @@ +1180,3 @@ + output_name = gdk_screen_get_monitor_plug_name (screen, i); + { + for (i = 0; i < gdk_screen_get_n_monitors (screen); i++) You need to free output_name in each loop iteration, not just once at the end.
(In reply to comment #5) > Review of attachment 157897 [details] [review]: > > ::: src/shell-global.c > @@ +1170,1 @@ > + for (i = 0; i < gdk_screen_get_n_monitors (screen); i++) > > This is a really minor thing, but I'd hoist the call to > gdk_screen_get_n_monitors (screen) outside of the loop by creating a local > variable; this is what the GTK+ uses of it do. It's currently just a field > lookup but possibly in the future it could be more expensive. OK > @@ -1170,3 @@ > - G_STRUCT_OFFSET (MetaRectangle, width) == G_STRUCT_OFFSET > (GdkRectangle, width) && > - G_STRUCT_OFFSET (MetaRectangle, x) == G_STRUCT_OFFSET > (GdkRectangle, x) && > - g_assert (sizeof (MetaRectangle) == sizeof (GdkRectangle) && > > Why did you remove this assertion? Because the function no longer uses MetaRectangle, so it shouldn't be needed. (unless I am missing something). > @@ +1180,3 @@ > + output_name = gdk_screen_get_monitor_plug_name (screen, i); > + { > + for (i = 0; i < gdk_screen_get_n_monitors (screen); i++) > > You need to free output_name in each loop iteration, not just once at the end. D'oh ... yeah will fix that.
(In reply to comment #6) > > Because the function no longer uses MetaRectangle, so it shouldn't be needed. > (unless I am missing something). Ah, true. > > @@ +1180,3 @@ > > + output_name = gdk_screen_get_monitor_plug_name (screen, i); > > + { > > + for (i = 0; i < gdk_screen_get_n_monitors (screen); i++) > > > > You need to free output_name in each loop iteration, not just once at the end. > > D'oh ... yeah will fix that. Feel free to commit after fixing that.
Attachment 157897 [details] pushed as ca13cec - Rework shell_global_get_primary_monitor with a fix for the free issue.
I LOVE YOU. :) Thanks.
why not just use gdk_screen_get_primary_monitor() ? it supports primary monitor specified by randr and otherwise checks for a LVDS connector. it's a little buggy atm, but i'm working on it :) .. https://bugzilla.gnome.org/show_bug.cgi?id=615128
ah, sorry didn't read comment 1
(In reply to comment #10) > why not just use gdk_screen_get_primary_monitor() ? it supports primary monitor > specified by randr and otherwise checks for a LVDS connector. I know, that is why I have added the LVDS check to it ;) Currently using it requires gtk master (as the version for 2.20.0 does not have the LVDS check yet), so I am waiting for 2.20.1 to be released also we will have to maintain the current code path as fallback for a while (unless we want to depend on gtk 2.20.1+ which is imo not a good idea at this point).
i hope they will release it soon then, because as it is now, the panel always appears on the top left monitor on non laptops as gdk sorts the monitors this way (first monitor is always top left). this change broke the shell panel for me. don't know why it worked with the old code .. maybe mutter respects the order from randr or xinerama.
Created attachment 159448 [details] [review] use gdk_get_primary_monitor this can be used as soon as gtk 2.20.1 is out (and a minimum dependency). i use this myself atm.
Review of attachment 159448 [details] [review]: Thanks, I was wanted to do this for a while but was waiting for gtk 2.20.1 to be released. I am not sure a hard dependency is right though. ::: src/shell-global.c @@ +1109,1 @@ + gdk_screen_get_monitor_geometry (screen, gdk_screen_get_primary_monitor(screen), &rect); I'd rather have this in a #if GTK_CHECK_VERSION(2, 20, 1) ... #else ... old code .. #endif
hmm, doesn't the next version of gnome (and therefore gnome-shell) depend on the next gtk version anyway?
Sounds like this is expected, but I have a dual monitor setup where the right monitor is my primary, but with latest code gnome-shell has begun assuming the left monitor is primary regardless how I connect them to the video card. Any suggested workaround? I've got it working by reverting the three commits related to the above: cbcbd11ba07e8ebe93aa6e83e5211bf62cb096be bae07700a18db0a462d37f0558063aaa9040ed3d ca13cec01c44f65f4d17863aa78741cbe14a156b Just wanted to make sure the issue was raised, I have GTK 2.20.1 which from the above sounds like it should have fixed the issue, but this is not the case. Cheers!
(In reply to comment #17) > Sounds like this is expected, but I have a dual monitor setup where the right > monitor is my primary, but with latest code gnome-shell has begun assuming the > left monitor is primary regardless how I connect them to the video card. > you can use my patch (https://bugzilla.gnome.org/attachment.cgi?id=159448)
Works great, thanks Florian.
Created attachment 160743 [details] [review] Use gdk_screen_get_primary_monitor on gtk-2.20.1+ Starting with gtk-2.20.0 there is a gdk_screen_get_primary_monitor, which supports querying the primary monitor from xrandr. But due to a sorting bug and lack of heuristics in the fallback path, it isn't really useable. Those bugs are fixed in gtk-2.20.1, so use it when building with gtk-2.20.1+.
Review of attachment 160743 [details] [review]: You obviously didn't try to build with GTK 2.20.1 ;) Feel free to ignore the second comment. ::: src/shell-global.c @@ +1109,1 @@ GdkRectangle rect; rect is used outside the cpp condition, so you'll get a build error when building with gtk >= 2.20.1 @@ +1130,3 @@ + gdk_screen_get_monitor_geometry (screen, gdk_screen_get_primary_monitor (screen), &rect); +#else OK, this one is purely stylistic and probably just a matter of personal taste, but I'd organize this along the lines of: GdkRect rect; gint primary = 0; /* your comment */ #if !GTK_CHECK_VERION(...) /* remaining variables */ /* all the stuff we currently do */ #else primary = gdk_screen_get_primary_monitor (screen); #endif gdk_screen_get_monitor_geometry (screen, primary, &rect); I think that way it's slightly clearer that the first part is nothing more than a poor man's gdk_screen_get_primary_monitor() implementation ...
(In reply to comment #21) > Review of attachment 160743 [details] [review]: > > You obviously didn't try to build with GTK 2.20.1 ;) D'oh > Feel free to ignore the second comment. > > ::: src/shell-global.c > @@ +1109,1 @@ > GdkRectangle rect; > > rect is used outside the cpp condition, so you'll get a build error when > building with gtk >= 2.20.1 Indeed ... > @@ +1130,3 @@ > > + gdk_screen_get_monitor_geometry (screen, gdk_screen_get_primary_monitor > (screen), &rect); > +#else > > OK, this one is purely stylistic and probably just a matter of personal taste, > but I'd organize this along the lines of: > > GdkRect rect; > gint primary = 0; > /* your comment */ > #if !GTK_CHECK_VERION(...) > /* remaining variables */ > > /* all the stuff we currently do */ > > #else > primary = gdk_screen_get_primary_monitor (screen); > #endif > > gdk_screen_get_monitor_geometry (screen, primary, &rect); > > I think that way it's slightly clearer that the first part is nothing more than > a poor man's gdk_screen_get_primary_monitor() implementation ... This is probably better (code looks cleaner this way) so I changed it.
Created attachment 160873 [details] [review] Use gdk_screen_get_primary_monitor on gtk-2.20.1+ *) Fix build bug *) Clean up as per review
Review of attachment 160873 [details] [review]: Looks good - please fix the style comment below before committing. ::: src/shell-global.c @@ +1124,3 @@ + /* gdk_screen_get_primary_monitor is only present in gtk-2.20+ + + gint primary = 0; Space before parenthesis
Attachment 160873 [details] pushed as 410dfb2 - Use gdk_screen_get_primary_monitor on gtk-2.20.1+