GNOME Bugzilla – Bug 615128
gdk monitor order does not seem to be correct
Last modified: 2010-04-23 21:43:03 UTC
gtk+2.20.0 xrandr --version: xrandr program version 1.3.2 Server reports RandR version 1.3 xrandr -q: Screen 0: minimum 320 x 200, current 2560 x 1024, maximum 8192 x 8192 DVI-0 connected 1280x1024+1280+0 (normal left inverted right x axis y axis) 376m ... DIN disconnected (normal left inverted right x axis y axis) DVI-1 connected 1280x1024+0+0 (normal left inverted right x axis y axis) 360mm x ... xdpyinfo -ext XINERAMA: XINERAMA version 1.1 opcode: 149 head #0: 1280x1024 @ 1280,0 head #1: 1280x1024 @ 0,0 test.c: monitor 0, name: DVI-1, 1280x1024+0+0 monitor 1, name: DVI-0, 1280x1024+1280+0 primary: monitor 0, name: DVI-1, 1280x1024+0+0
Created attachment 158158 [details] test program
Created attachment 158159 [details] xorg.conf
i just had a look into gdkscreen-x11.c and now i'm really confused. the method init_randr13 first retrives the data from randr (in the correct order!) and remembers the index of the primary screen (again from randr) in the list. now the confusing part: the list gets sortet from top left to bottom right, outright ignoring the spezified order (and therefor invalidating the remembered index of the primary screen).
Created attachment 158245 [details] [review] find index after sorting if the sorting of the monitors is needed, this should fix the index of the primary monitor. compiles, but not tested.
Doh, that is a stupid bug. But I think we want to still compare the output ids, instead of doing string comparisons. We can just iterate over the monitors array after sorting and compare monitor.output
Created attachment 158288 [details] [review] now compares xids oh, didn't know rroutput's where xid's ...
(In reply to comment #6) > Created an attachment (id=158288) [details] [review] > now compares xids > > oh, didn't know rroutput's where xid's ... Matthias already pushed a fix http://git.gnome.org/browse/gtk+/commit/?id=b4adea747780fb05a7923707fe431d924c2ada9d
Created attachment 158295 [details] [review] v3 it seems that many implementations of randr 1.3 (eg radeon) are buggy, primary is not set but it is the first output returned by randr. this patch fixes the sorting and sets the primary monitor to the first monitor returned by randr if primary is not set and no lvds is found.
Created attachment 158299 [details] [review] add new heuristic, applies to master Use the first monitor returned by RANDR as primary, if primary is not set and no LVDS is found.
i'm (trying) to reopen this, because my original bug is not fixed by this. the xserver, as it is now, does not set the primary monitor but just returns it as the first in the list ( see https://bugs.freedesktop.org/show_bug.cgi?id=23952 ). this may be no gdk bug, but my submitted patch would work around this in a clean way.
(In reply to comment #10) > i'm (trying) to reopen this, because my original bug is not fixed by this. > the xserver, as it is now, does not set the primary monitor but just returns it > as the first in the list ( see > https://bugs.freedesktop.org/show_bug.cgi?id=23952 ). this may be no gdk bug, > but my submitted patch would work around this in a clean way. How so? In case there is no primary set we do fallback to the first one anyway unless it is a laptop in then we fall back to LVDS.
> > How so? > > In case there is no primary set we do fallback to the first one anyway unless > it is a laptop in then we fall back to LVDS. we fall back to the first monitor _after_ sorting them (eg _always_ top left). my patch tries to fix this.
is there a problem with this patch?
Review of attachment 158299 [details] [review]: This looks good to me, but I'd rather let Matthias review/approve it.
Review of attachment 158299 [details] [review]: Looks good to commit, if you split that declaration into two lines. ::: gdk/x11/gdkscreen-x11.c @@ +748,3 @@ Display *dpy = GDK_SCREEN_XDISPLAY (screen); XRRScreenResources *resources; + RROutput primary_output, first_output = None; I'd rather have these on two separate lines @@ +834,3 @@ + { + screen_x11->primary_monitor = i; + break; If you want to add that break here, you should add it in the other if as well, for consistency. @@ +842,3 @@ + { + screen_x11->primary_monitor = i; + break; Hah, you already did. @@ +847,3 @@ + /* No primary specified and no LVDS found */ + if (screen_x11->monitors[i].output == first_output) + screen_x11->primary_monitor = i; This way of doing this is a little sneaky, since it makes the breaks you added further up more than just an optimization. But I guess it is the easiest way to do things.
Created attachment 159446 [details] [review] fixed declaration ok, fixed the variable declaration. i have no git account, so i cannot commit this myself.
Pushed to master.