GNOME Bugzilla – Bug 645408
switching between single and multi monitor puts all windows in one desktop
Last modified: 2011-09-14 13:39:43 UTC
When switching between single and multiple monitors using TwinView (nvidia's proprietary extension), the shell forgets about all desktops and puts all windows in the same desktop. This setup worked fine in GNOME 2.
I can reproduce that on Intel.
I guess having it also happen on Intel increases the severity.
Created attachment 184100 [details] [review] Don't move all window to active workspace if monitors change
So, say you plug in a monitor, and it's set to appear to the left of the primary. What happens with Metacity/Mutter is that all the windows stick on the left-most monitor, which which is secondary, so primary ends up empty. Since primary is empty, all workspaces other than workspace 1 are deleted. When the monitor is plugged back in, and we restore the windows, their workspaces don't exist, so we put them back on 1. Though this patch looks right, I'd like to have fixes for the above problem and any residual problem with windows getting left sticky (which I can no longer reproduce with this patch, for whatever reason but occurred in the above scenario without it) before we land it instead of a trickle of fixes. We should see if the best fix for the above is to actually fix the real problem - that is, if another output is added and primary stays on an existing output, keep the windows where they were. The trouble with trying to fix that is that I don't think we have any output identification (other than resolution and pray it's unique) using Xinerama. In fact, it's hard to imagine a fix without the real fix - what if the user did a bunch of workspace manipulation to add back workspaces and move things around before unplugging - where do the old windows go then?
Created attachment 184186 [details] [review] When monitors change, keep windows on same output. If XRANDR is availible, we track the first (or primary) output per crtc (== xinerama monitor) so when the monitors change we can try to find the same output and move windows there. If we can't find the original monitor in the new set (or XRANDR is not supported) we move the window to the primary monitor.
Review of attachment 184186 [details] [review]: Only small style issues with the code here, and testing it out, this is definitely a huge improvement if you have the primary monitor on the right. On the other hand, the basic problem of windows hopping between displays has been there for years, so it can't be that urgent, can it? It's *worse* now because when windows hop of the primary display you lose workspace assignment, but maybe it wasn't that bad before? This basically smells like 3.0.1 to me, not 3.0.0; like we should go with your first patch which is really simple and fixes the more common primary-on-the-left case for 3.0.0. ::: src/core/screen.c @@ +509,3 @@ + +#ifdef HAVE_RANDR + resources = XRRGetScreenResourcesCurrent (display->xdisplay, screen->xroot); Believe this will result in, on stderr: Extension "randr" missing on display ":0" which is maybe a little unclean - should really add a check for Xrandr to meta_display_open() [though do I *really* care about servers without Xrandr? No] ::: src/core/window-private.h @@ +632,3 @@ void meta_window_update_role (MetaWindow *window); void meta_window_update_net_wm_type (MetaWindow *window); +void meta_window_update_monitor_for_resize (MetaWindow *window); confusing name - maybe for_screen_resize() or for_monitors_changed() ::: src/core/window.c @@ +4222,3 @@ + + rel_x = window->user_rect.x - old_area.x; + rel_y = window->user_rect.y - old_area.y; This algorithm shouldn't be duplicated in two places.
Created attachment 184418 [details] [review] When monitors change, keep windows on same output. If XRANDR is availible, we track the first (or primary) output per crtc (== xinerama monitor) so when the monitors change we can try to find the same output and move windows there. If we can't find the original monitor in the new set (or XRANDR is not supported) we move the window to the primary monitor.
Pushed the small original patch, should be easy to rebase the larger patch on top.
Created attachment 184526 [details] [review] When monitors change, keep windows on same output. If XRANDR is availible, we track the first (or primary) output per crtc (== xinerama monitor) so when the monitors change we can try to find the same output and move windows there. If we can't find the original monitor in the new set (or XRANDR is not supported) we move the window to the primary monitor.
Moving off the 3.0 blocker list, since the larger patch was deemed non-3.0 material.
Did the the larger patch go in now that we're past 3.0?
(In reply to comment #11) > Did the the larger patch go in now that we're past 3.0? Nope; can you look at rebasing it and committing it?
Created attachment 188210 [details] [review] When monitors change, keep windows on same output. If XRANDR is availible, we track the first (or primary) output per crtc (== xinerama monitor) so when the monitors change we can try to find the same output and move windows there. If we can't find the original monitor in the new set (or XRANDR is not supported) we move the window to the primary monitor.
This new patch is rebased against master, and I had to change it a bit to work with the first patch applied.
Can I get some review action on the new patch?
Review of attachment 188210 [details] [review]: ::: src/core/screen.c @@ +543,3 @@ + +#ifdef HAVE_RANDR + if (display->have_xfixes) what does have_xfixes have to do with this? Pretty sure xfixes substantially predates and is independent from xrandr ::: src/core/window.c @@ +126,3 @@ +static void meta_window_move_between_rects (MetaWindow *window, + const MetaRectangle *old_area, + const MetaRectangle *new_area); Though the static functions are pretty messy, all others do have aligned parameters within the function
*** Bug 654613 has been marked as a duplicate of this bug. ***
Are we still hoping to get this in for 3.2 ?
I hope so, it makes multimonitor changes much nicer, and it works fine here. The review comments are mainly cosmetic, but I unfortunately don't really have time to look at them, as i'm tight for time on Gnome Contacts work.
Created attachment 195984 [details] [review] When monitors change, keep windows on same output. (In reply to comment #19) > The review comments are mainly cosmetic, but I unfortunately don't really have > time to look at them, as i'm tight for time on Gnome Contacts work. Updated patch according to review - note that the have_xfixes change is completely untested.
Review of attachment 195984 [details] [review]: Let's go ahead and land this. ::: src/core/screen.c @@ +546,3 @@ + XRRScreenResources *resources; + + resources = XRRGetScreenResourcesCurrent (display->xdisplay, screen->xroot); I think this will produce Xlib: extension "RANDR" missing on display ":0.0" when returning NULL. I'm thinking is is probably OK, since AFAIK even the NVIDIA driver has some support for xrandr at this point.
Should we get this committed, then ?
Attachment 195984 [details] pushed as 19b6888 - When monitors change, keep windows on same output. (In reply to comment #21) > Let's go ahead and land this. OK.