GNOME Bugzilla – Bug 620744
When closing a window, select the next-topmost window without restacking anything
Last modified: 2012-07-14 17:21:49 UTC
- Open two windows of the same application, e.g. two terminals. - Switch to another application with Alt+Tab. - Come back to one of the two windows, still with Alt+Tab. - Close the window that is on top. - See the other window return to the bottom of the windows stack. I don't think that's intended, at any rate that feels very weird. Cases when it happens include closing Empathy contacts list, closing a New message window in Evolution... Depending on what the user did before (e.g. checking a web page while writing the mail, and coming back to it), this unexpected behavior will happen, and for users it's not really obvious why.
Agreed.
Created attachment 171314 [details] [review] correct raise windows in shell_app_activate_window meta_window_raise doesn't change order of getting focus
Comment on attachment 171314 [details] [review] correct raise windows in shell_app_activate_window >+ meta_window_activate (other_window, meta_display_get_current_time (display)); Why do you call meta_display_get_current_time() everywhere instead of using timestamp? Timestamp is supposed to be right, and if it's not, then presumably the caller has some reason for passing an old timestamp... (If nothing else, you should at least use shell_global_get_current_time(); meta_display_get_current_time() may not return the right value if this is called from a clutter event callback.) Also, it's probably worth commenting why we need to use activate() (just add a comment with the same text as that commit message).
Created attachment 171337 [details] [review] correct raise windows in shell_app_activate_window > Why do you call meta_display_get_current_time() everywhere instead of using > timestamp? After focus windows with same timestamp, this windows will have random order. > If nothing else, you should at least use shell_global_get_current_time(); It return same timestamp. CurrentTime is perfectly acceptable here.
(In reply to comment #4) > > Why do you call meta_display_get_current_time() everywhere instead of using > > timestamp? > After focus windows with same timestamp, this windows will have random order. But meta_display_get_current_time() only changes when a new X event comes in... > > If nothing else, you should at least use shell_global_get_current_time(); > It return same timestamp. CurrentTime is perfectly acceptable here. CurrentTime is *not* acceptable; it causes meta_window_activate() to print a warning and then do a round-trip to the x server to get a real timestamp. Ah, so in fact, your patch *depends* on the fact that meta_display_get_current_time() will return 0 when called from the Alt-Tab code; if someone called shell_app_activate_window() from a mutter-related event handler, you'd end up passing the same timestamp to meta_window_activate() each time and get a random ordering. And even if you call meta_display_get_current_time_roundtrip(), X server time only has a 1ms resolution, so that only works if things are slow enough that each meta_window_activate() call is at least 1ms after the last. I think we need to add code to mutter to deal with this case. Rather than actually focusing each window, just add a method that lets you move it to the front of workspace->mru_list like meta_window_notify_focus() does. (Or perhaps, just make meta_window_raise() do this? It seems like a very plausible behavior...)
Created attachment 172388 [details] [review] add meta_window_make_mru > Or perhaps, just make meta_window_raise() do this? It is bad idea, because meta_window_raise widely used in mutter code. (change it behaviour can bring a lot bugs).
Created attachment 172389 [details] [review] correct raise windows in shell_app_activate_window
gonna reassign this to owen since it involves a mutter patch now.
Review of attachment 172389 [details] [review]: Restacking windows when you close a window looks really broken. It looks broken enough that I'm confident to say that: A) It's never right B) We didn't notice that Metacity focuses the MRU window because the MRU window is almost always the top stacked window Rather than modifying the MRU order (and losing information, since the other application windows are *not* the most recently used windows by the user), we should change Mutter so that when you close a window, the topmost stacked window is focused.
Review of attachment 172388 [details] [review]: As noted in the other bug, I don't think this is the right approach.
Reassigning to Mutter since I think the correct fix has no gnome-shell portion.
Created attachment 172973 [details] [review] change behaviour of meta_workspace_focus_default_window in META_FOCUS_MODE_CLICK use meta_stack_get_default_focus_window for determinate default window.
Hmm, this patch is going to reintroduce bug 97635
[ Didn't meant to actually submit the changes there without additional commentary ] My feeling is that change done in bug 97635 is actually wrong - switching to an empty workspace and back shouldn't be changing the stacking order on this workspace, and adding per-workspace MRU lists that are *sort* of like the stacking order, but not quite just confuses things. You can see that there is still weirdness from the restacking if you have three windows on the workspace in the order: A (not sticky) B (not sticky) C (sticky) Flipping to an empty workspace and back will give you the order A C B, which is certainly unexpected. The real solution is probably per-workspace stacks, but since sticky windows are really a marginal feature, such a big invasive change seems unwarranted. I'd like for us to try the other approach that Havoc suggested in https://bugzilla.gnome.org/show_bug.cgi?id=97635#c2 with making meta_window_raise() only raise the window sufficiently high so it is above other windows on the same workspace. From a quick look at stack.c this probably isn't that hard - I think it would work to make meta_stack_raise() set the position to MAX(position of windows on the same workspace in the same layer) instead of stack->n_positions - 1. Of course, that might be harder than it sounds - maybe things like layers inheriting from parent to transient make it tricky to do.
Okay a few questions: 1) Why does switching to an empty workspace and back change the stacking order? When switching back it's going to focus the most recently used window. That should be the one on the top of the stack right? 2) If you switch to a workspace that only has a sticky window on it (say a movie player or a gedit window you're using for notes) and no other windows. Should it get focus even though it was unfocused before? I think the answer is yes. We always want to focus some window, and if the only window available is a sticky one, it should get focused. 3) If the user was using that gedit window or movie player on the empty workspace and switches back to their original workspace, should it keep focus? I think the answer is no. That's what bug 97635 was about. If you switch workspaces you're switching workspaces because you want switch focus to another app, so not switching focus is wrong. (i think my conclusions in 2 and 3 are non-contraversial and generally regarded as right, but i'm just bring them up for clarity) 4) Now the user is back on their workspace. Should hitting alt-tab once focus that gedit window/movie player? Owen, if I understand comment 14 correctly, you're saying that's unexpected, but to me it seems pretty natural. When I hit alt-tab once, i want it to flip to the last thing i was working on (if its on this workspace, at least). I mean for sticky windows, the user identifies the window as the same window even though it's on multiple workspaces. If it gets focus on one workspace, I'd expect the history of it getting focused to be considered for other workspaces, too.
(I will say the model is probably different in the shell case where alt-tab shows windows from all workspaces. I haven't thought about it in that context)
(In reply to comment #15) > Okay a few questions: > > 1) Why does switching to an empty workspace and back change the stacking order? > When switching back it's going to focus the most recently used window. That > should be the one on the top of the stack right? Switching to an empty workspace changes the stacking order because: A) Metacity has a global stack of windows, not a per-workspace stack B) Focusing a window moves it to the top of the global stack So when we switch back to the original workspace, the top window isn't the MRU window on that workspace, it's the sticky window. This to me is the heart of the problem, and the thing that should have been fixed in 2003. I think the user's mental model is a per-workspace stack of windows, but that's a lot of work, and fixing B) instead of A) probably gets you "close enough". If you just fix B) then switching workspaces doesn't restack windows, but restacking windows on one workspace can affect the stacking of a sticky window on another workspace. > 2) If you switch to a workspace that only has a sticky window on it (say a > movie player or a gedit window you're using for notes) and no other windows. > Should it get focus even though it was unfocused before? I think the answer is > yes. We always want to focus some window, and if the only window available is > a sticky one, it should get focused. Yes. > 3) If the user was using that gedit window or movie player on the empty > workspace and switches back to their original workspace, should it keep focus? > I think the answer is no. That's what bug 97635 was about. If you switch > workspaces you're switching workspaces because you want switch focus to another > app, so not switching focus is wrong. > > (i think my conclusions in 2 and 3 are non-contraversial and generally regarded > as right, but i'm just bring them up for clarity) Certainly. Switching to an empty workspace (or a non-empty workspace) and back should have no effect on the focused window, and it should have no effect on the window stack. The fix to bug 97635 fixed the focus window, but did not fix the effect on the window stack. > 4) Now the user is back on their workspace. Should hitting alt-tab once focus > that gedit window/movie player? Owen, if I understand comment 14 correctly, > you're saying that's unexpected, but to me it seems pretty natural. Whether it's expected or unexpected, it doesn't happen currently. When you switch back to the workspace, the stacking order has been changed so that the gedit window/movie player is second in the stacking order. But that is a figment of display. If I hit alt tab, the next window will be the window B... the other non-sticky window, and in fact, if I close window A, window B will be focused and come to the top despite the fact that window C (the gedit/movie player) looks like it is right underneath window A. (This is a direct consequence of your change in 2003 that gave each workspace a separate MRU list.) If we do want switching workspaces to affect the MRU list and stacking order but not change focus, then we'd need a different for bug 97635 - we'd want to have a global MRU list and a per-workspace saved focus window. (Again that's an intrusive change for a marginal feature.)
(In reply to comment #17) > (In reply to comment #15) > > Okay a few questions: > > > > 1) Why does switching to an empty workspace and back change the stacking order? > > When switching back it's going to focus the most recently used window. That > > should be the one on the top of the stack right? > > Switching to an empty workspace changes the stacking order because: > > A) Metacity has a global stack of windows, not a per-workspace stack > B) Focusing a window moves it to the top of the global stack > > So when we switch back to the original workspace, the top window isn't the MRU > window on that workspace, it's the sticky window. Oh, when you're saying empty workspace you mean "workspace with only a sticky window". I didn't realize that before, so you're talking about the specific scenario mentioned in bug 97635 comment 0. But when you switch back to the original workspace, the MRU window *for that workspace* should get focused and when it gets focus it should get raised (just as the sticky window got focused and raised when you went to the empty workspace.) That's what bug bug 97635 comment 7 is about, I think (although 2003 was an awful long time ago...) So that should mean the top of the stack is the same window that was at the top of the stack before, the sticky window gets bumped to just under it, and everything is the way i would it expect it to be. > This to me is the heart of > the problem, and the thing that should have been fixed in 2003. I think the > user's mental model is a per-workspace stack of windows, I totally agree that normal windows should follow that mental model, but I guess what i'm saying, is sticky windows don't fit the "per-workspace stack" mental model at all. If they move to the top of the stack on one workspace, I don't think they should stay on the bottom of the stack on another workspace. I think that would be weird ("I just brought my movie player to the foreground when I was on that other workspace, why is at the bottom now?!"). My opinion is, we really want to treat this situation like there's an implicit quick "alt-tab" when switching workspaces, if the currently focused window is going to still be around on the new workspace. The implicit alt-tab behavior can be justified because if the user is switching workspaces, they're almost certainly doing it because they want to use an app that's only available on the workspace they're switching to. > > 4) Now the user is back on their workspace. Should hitting alt-tab once focus > > that gedit window/movie player? Owen, if I understand comment 14 correctly, > > you're saying that's unexpected, but to me it seems pretty natural. > > Whether it's expected or unexpected, it doesn't happen currently. When you > switch back to the workspace, the stacking order has been changed so that the > gedit window/movie player is second in the stacking order. But that is a > figment of display. If I hit alt tab, the next window will be the window B... That's bizarre, the alt-tab popup should follow the mru list, and I would expect the mru list for workspace 1 to be something like this: 1 Start of the excerise: A B C 2 After the user switches to workspace 2: A B C 3 When the user switches back before the focus fixup: C A B 4 After the focus fix up: A C B So you're saying that's not working? > If we do want switching workspaces to affect the MRU list right, the mru list should get changed by 3 and 4 above. > and stacking order right, the stacking order should get changed after 2 and 4 > but not change focus, The focus should get changed after 2, but changed back by 4 > then we'd need a different fix for bug 97635 Sounds like maybe we could: a) make sure the mru list is updated when the user first switches back if the focused window is still available to reflect the fact that it's focused (since MRU ~= Most Recently Focused, and the sticky window hasn't lost focus yet immediately after the workspace switch) b) do the "implicit alt-tab" if the focused window was also the focused window on the previous workspace. This means focus the top window in the mru list (ignoring the one we just added) Anyway, I agree with you that sticky windows are a niche feature, so we shouldn't get too fancy here. Also, my whole line of thought above is with the metacity mindset, not a shell mindset (which I haven't really thought about at all). I think it's more important that we make sure whatever change happens here is right for the shell than is right for metacity...
(In reply to comment #18) > a) make sure the mru list is updated when the user first switches back if the > focused window is still available to reflect the fact that it's focused (since > MRU ~= Most Recently Focused, and the sticky window hasn't lost focus yet > immediately after the workspace switch) Restacking the windows will have reordered the sticky window, no matter whether the sticky window is still the focused window when we return to to the first workspace. > Anyway, I agree with you that sticky windows are a niche feature, so we > shouldn't get too fancy here. Also, my whole line of thought above is with the > metacity mindset, not a shell mindset (which I haven't really thought about at > all). I think it's more important that we make sure whatever change happens > here is right for the shell than is right for metacity... The shell mindset really is "sticky windows don't matter" - I don't want to regress bug 97635 on general principle; I also don't want to see huge code changes to try and fix some detail of sticky window operation.... it just has to end up as "reasonable" (that is, sticky windows shouldn't obsessively end up as your focused window when you switch workspaces.)
*** Bug 650626 has been marked as a duplicate of this bug. ***
*** Bug 649696 has been marked as a duplicate of this bug. ***
Created attachment 199304 [details] [review] stack: Make meta_window_raise() and meta_window_lower() smarter
Created attachment 199305 [details] [review] workspace: Don't try to focus the MRU window
I tested minimally last night with these two patches. There's probably a number of things that I missed, but it seems to work here. Are there some basic things that I can test to make sure it gets the right behavior?
Created attachment 199375 [details] [review] workspace: Don't try to focus the MRU window Focus the top window on a workspace instead. -- Excuse me, I'm an idiot and didn't notice that no window was getting focused. This selects the top-most window on the stack. This will "do the wrong thing" when it comes to sticky windows, but the only potential way I can see us fixing this is maintaining a per-workspace stack and syncing it every time the user switches workspaces. If the user only has two sticky windows open and two workspaces open, for a per-workspace stack, one workspace having "A B" and the other having "B A" is impossible unless we re-sync on every workspace switch. I can try to do something like this.
Created attachment 199695 [details] [review] workspace: Don't try to focus the MRU window Focus the top window on a workspace instead. -- Fix documentation, as well as an infinite loop I swore I fixed and a segfault
*** Bug 668821 has been marked as a duplicate of this bug. ***
How funny, I don't remember filing bugs twice before... ;-) That damn mutter/gnome-shell distinction... So, can we wake up this report? :-)
*** Bug 671385 has been marked as a duplicate of this bug. ***
Review of attachment 199304 [details] [review]: Generally looks like something like this should work - some cosmetic and non-cosmetic issues reading through this though ::: src/core/stack.c @@ +191,3 @@ + GList *l; + int max_stack_position; + max_stack_position = -1; I know the older Metacity code has some sort of thing against inline initializations, but something like this reads like "I'm not allowed to do inline initializations, so I'm breaking it up into two lines. (Read the white space line). Currently we consider inline initializations fine. @@ +193,3 @@ + max_stack_position = -1; + + l = stack->sorted; Hmm, things look a bit confusing - I was thinking that there might be windows in ->added, but it appears that added is only ever populated in meta_window_stack_add() until the call to sync_stack_to_server() at the bottom of the function. Check over my logic, and if its right, maybe add a g_assert(stack->added == NULL) here. @@ +194,3 @@ + + l = stack->sorted; + while (l != NULL) My preference for loop-with-no-funny-business is: for (l = stack->sorted; l; l = l->next) { } since it makes it explicit that it's just straightforward iteration. @@ +197,3 @@ + { + MetaWindow *w = (MetaWindow *) l->data; + if (w->workspace == window->workspace && w->stack_position > max_stack_position) you have to worry about w being on_all_workspaces too @@ +204,3 @@ + + if (max_stack_position == -1) + max_stack_position = stack->n_positions - 1; If there are no windows on this workspace, we simply need to do nothing, not raise the window to the to of the global stack, right? (Or can this just not happen because this window is going to be in ->sorted?) @@ +207,3 @@ + + meta_window_set_stack_position_no_sync (window, + max_stack_position); Logically it cant be right that: meta_window_set_stack_position_no_sync (window, max_stack_position); Raises the window above all windows on the workspace and: meta_window_set_stack_position_no_sync (window, min_stack_position); lowers below all windows on the workspace - one or the other has to be wrong. @@ +225,3 @@ + GList *l; + int min_stack_position; + min_stack_position = 99999; G_MAXINT seems clearer, even if it doesn't matter @@ +238,3 @@ + + if (min_stack_position == 99999) + min_stack_position = 0; same question here - if nothing on the workspace, we need to do anything?
Review of attachment 199695 [details] [review]: The commit message needs more detail - there's no "why" or even "what" here. The relevant information is: - bug 97635/2fc880db switched from focusing the topmost window as the default window to focusing the mru window. This was done in conjection introduction of per-workspace mru lists to avoid problems where the window stack was inadvertently changed when focusing windows during window switches - Now that we've switch focusing windows to have less effect on stacking order we can revert back to focusing the top window, which is less confusing to the user - But we leave the per-workspace MRU lists because they work pretty much as well as a global MRU list and to avoid code churn You also should leave a comment where workspace->mru_list is defined in window-private.h describing the history of why mru-lists are per-workspace so that someone who wanted to fix some other bug would know that switching back to a global mru list would at least be an option. ::: src/core/stack.c @@ +1363,3 @@ MetaWindow* +meta_stack_get_top_on_workspace (MetaStack *stack, + MetaWorkspace *workspace) Isn't this just an incomplete version of meta_stack_get_default_focus_window() that doesn't handle the corner cases?
(In reply to comment #30) > @@ +207,3 @@ > + > + meta_window_set_stack_position_no_sync (window, > + max_stack_position); > > Logically it cant be right that: > > meta_window_set_stack_position_no_sync (window, > max_stack_position); > > Raises the window above all windows on the workspace and: > > meta_window_set_stack_position_no_sync (window, > min_stack_position); > > lowers below all windows on the workspace - one or the other has to be wrong. Of course it can be. It's not an "insert before" or "insert after" situation here, it's a "set position" -- the length of the list of windows isn't changing, so calling set_stack_position_no_sync (w, max_stack_position); will make w take the the place of the current top of the stack's stack position, bumping everything else, including the previous top of stack's position, down. Another way to think about it is that the index that you're passing here is effectively the index you would have if you removed the window first, so you can treat it as a -1 +1 situation.
(In reply to comment #32) > (In reply to comment #30) > > @@ +207,3 @@ > > + > > + meta_window_set_stack_position_no_sync (window, > > + max_stack_position); > > > > Logically it cant be right that: > > > > meta_window_set_stack_position_no_sync (window, > > max_stack_position); > > > > Raises the window above all windows on the workspace and: > > > > meta_window_set_stack_position_no_sync (window, > > min_stack_position); > > > > lowers below all windows on the workspace - one or the other has to be wrong. > > Of course it can be. It's not an "insert before" or "insert after" situation > here, it's a "set position" -- the length of the list of windows isn't > changing, so calling set_stack_position_no_sync (w, max_stack_position); will > make w take the the place of the current top of the stack's stack position, > bumping everything else, including the previous top of stack's position, down. > > Another way to think about it is that the index that you're passing here is > effectively the index you would have if you removed the window first, so you > can treat it as a -1 +1 situation. OK, I agree you're right - if we're sure that the window is in the set of windows on the same workspace - and we should be - then moving the window to the first or last position and shuffling the other windows back into the place left behind does work out.
Created attachment 210196 [details] [review] stack: Make meta_window_raise() and meta_window_lower() smarter
Created attachment 210197 [details] [review] workspace: Don't try to use per-workspace MRU lists as a hint for focusing Commit 2fc880db switched from focusing the topmost window as the default window to focusing the MRU window. This was done in alignment with the introduction of per-workspace MRU lists to avoid problems where the window stack was inadvertently changed when focusing windows during window switches. Now that focusing windows don't have as big an impact on the stacking order, we can revert back to focusing the top window, which is less confusing to the user. For now, leave per-workspace MRU lists, as they're a pretty good approximation of a global MRU list, and it works well enough.
Review of attachment 210196 [details] [review]: ::: src/core/stack.c @@ +189,3 @@ + { + meta_window_set_stack_position_no_sync (window, + stack->n_positions - 1); Hmm, this means that moving to an empty workspace with only a sticky window causes it to end up at the top of the global stack, which was the original problem with sticky windows. I think meta_stack_raise() has to be treated as "raise this window considering only windows visible on the same workspace, or, for sticky windows, visible on the current workspace" @@ +207,3 @@ + + if (max_stack_position == -1) + return; this is impossible, right? @@ +222,3 @@ + if (meta_window_is_on_all_workspaces (window)) + { + meta_window_set_stack_position_no_sync (window, 0); similarly
Review of attachment 210197 [details] [review]: This looks good except for some comments about the added comment. What we should verify in testing: * Without any sticky windows, with windows on multiple workspaces, the stacking and focused window on each workspace appears entirely independent * With a sticky window, switching workspaces does not cause stacking changes, even if you switch to a workspace with only that sticky window on it. * Focusing a sticky window on one workspace causes it to be raised above none, some, or all windows on other workspaces in a unpredictable fashion. ::: src/core/workspace-private.h @@ +50,3 @@ + * For historical reasons, we keep an MRU list per workspace. + * It used to be used to calculate stacking order, but isn't + * anymore. Currently, it's used for the alt-tab popup, but that I don't think it was ever used to calculate stacking order. "It used to be used to calculate a per-workspace default window to focus, but now we just use the stacking order" @@ +53,3 @@ + * could (read: should) be cleaned up in the future, moving back + * to a global MRU list, as that's how stacking under X actually + * works. I don't think it has anything to do with how stacking works - the global stacking order doesn't have to be the same as what was most-recently-used - "since that's all we actually need" perhaps.
Created attachment 210200 [details] [review] stack: Make meta_window_raise() and meta_window_lower() smarter
Created attachment 210201 [details] [review] workspace: Don't try to use per-workspace MRU lists as a hint for focusing Commit 2fc880db switched from focusing the topmost window as the default window to focusing the MRU window. This was done in alignment with the introduction of per-workspace MRU lists to avoid problems where the window stack was inadvertently changed when focusing windows during window switches. Now that focusing windows don't have as big an impact on the stacking order, we can revert back to focusing the top window, which is less confusing to the user. For now, leave per-workspace MRU lists, as they're a pretty good approximation of a global MRU list, and it works well enough.
Review of attachment 210200 [details] [review]: Looks good
Review of attachment 210201 [details] [review]: ::: src/core/workspace-private.h @@ +56,3 @@ + * windows in alt-tab popup, which could (read: should) be cleaned + * up in the future by using the same stacking order that we + * currently use elsewhere. I don't really agree with this - stacking order is stacking order, mru order is mru order - once you go across the full range of possibilities - focus follows mouse, autoraise, etc, of Mutter behavior modes, if you want mru-order, as we do for alt-tab just track mru order. In GNOME Shell, our alt-Tab actually wants global MRU order, we've fooled around a lot to get that computed correctly, not sure where it stands now. Maybe just delete this last paragraph.
Attachment 210200 [details] pushed as 402b477 - stack: Make meta_window_raise() and meta_window_lower() smarter Attachment 210201 [details] pushed as a3bf9b0 - workspace: Don't try to use per-workspace MRU lists as a hint for focusing Pushed with paragraph removed.
Patch a3bf9b0 is causing some strange behavior: https://bugzilla.gnome.org/show_bug.cgi?id=675982