GNOME Bugzilla – Bug 694469
Overview – thumbnails sometimes swap places
Last modified: 2015-01-19 21:09:48 UTC
The window thumbnails in the overview sometimes swap places when the thumbnail area width changes (e.g. when the workspace list is expanded). This might be related to the new overview sort algorithm... which otherwise is a huge improvement, by the way. If it's relevant, the screen is 1366x768; using gnome-shell 3.7.90-27-g6fcc7e3. To reproduce: 1) Remove all workspaces but the first one. 2) Position three windows like this. ├─────────screen width──────────┤ ....┌───────────────────────┐.... ┬ ....│ │.... │ ┌─────────────┐ ┌─────────────┐ │ │ │ │ │ │ │ 1 │ │ 2 │ │ │ │ │ │ │ │ │ 3 │ │ │ └─────────────┘ └─────────────┘ │ ....│ │.... │ ....└───────────────────────┘.... ┴ 3) Open the overview. The workspace list will be collapsed, and the window thumbnails will be shown like this: ┌───────┐ │ 1 │ │ │ └───────┘ ┌─────────┐ ┌───────┐ │ │ │ 2 │ │ │ │ │ │ 3 │ └───────┘ └─────────┘ 3) Mouse-over the workspace list, to expand it. Expected results: The workspace list expands, nothing else happens. Actual results: The workspace list expands, and the window thumbnails #2 and #3 swap places: ┌───────┐ │ 1 │ │ │ └───────┘ ┌─────────┐ │ │ ┌───────┐ │ │ │ 2 │ │ 3 │ │ │ └─────────┘ └───────┘ When the workspace list closes, the thumbnails swap back to their original places.
*** Bug 694961 has been marked as a duplicate of this bug. ***
*** Bug 696344 has been marked as a duplicate of this bug. ***
I have a giant set of patches for this, but they're probably more 3.10 material. It's a decently hard bug to fix.
Not happening for 3.8.1, unless we want to push a scary number of patches for it. I'll attach the patch series soon.
Created attachment 240343 [details] [review] overview: Move the group construction to the controls manager Instead of creating a bunch of random actors and then passing them off to the controls manager, let the controls manager construct them. This leaves the controls manager in charge of the ordeal.
Created attachment 240344 [details] [review] workspace: Use Workspace.WindowPositionFlags.NONE in another case
Created attachment 240345 [details] [review] workspace: Don't save the current layout This was saved so that doing something which called relayout but only changed the area rectangle would simply be needed to recompute window scaling parameters. With the new overview relayout, the flow control changed, it turns out that the current layout is always cleared. Remove this for now, and we'll put in a different strategy for this.
Created attachment 240346 [details] [review] workspace: Make positionWindows private It's not used outside, and it's going to be broken up soon.
Created attachment 240347 [details] [review] workspace: Remove more dead code By the time zoomToOverview is called, an animation will always be in progress.
Created attachment 240348 [details] [review] workspace: Only create one strategy Now that we don't have any other strategies but the unaligned one, we only need to construct it once.
Created attachment 240349 [details] [review] workspace: Abort relayouting much earlier This means that the code for computeAllWindowSlots is a bit cleaner, which will help in the future.
Created attachment 240350 [details] [review] workspace: Do window slot computing in three steps This ensures that we have the correct Y value when sorting windows.
Created attachment 240351 [details] [review] workspacesView: Fix indentation
Created attachment 240352 [details] [review] workspacesView: Minor cleanup
Created attachment 240353 [details] [review] workspacesView: Make setGeometry take a rectangle This cleanup will be more important in the future, but for now, we can simply pass a monitor.
Created attachment 240354 [details] [review] workspacesView: Calculate the workspaces geometry ourselves To ensure that we don't recalculate window layouts when zooming in or out, we need to always pass the full geometry. This will break window repositioning when we zoom back in; for the purposes of commit clarity, this breaks this feature for now. It will be added back soon.
Created attachment 240355 [details] [review] workspace: Make room for a second geometry to keep track of As we want to eventually track two geometries, we need to rename our very plain "_x, _y, _width, _height". While we could just prefix them, I think that stuffing them in an object makes more sense. At the same time, make the variable and method name more descriptive by adding such a prefix, as well as a bit of documentation.
Created attachment 240356 [details] [review] workspace: Separate out spacing/padding code This will be used when we introduce the second geometry.
Created attachment 240357 [details] [review] workspace: Calculate the window slots when we reposition Repositioning will eventually be separated from recalculation to accomodate two different geometries,
Created attachment 240358 [details] [review] workspace: Split out window repositioning logic and rename Split out the part that moves the window clones around from the part that calculates the window clone positions, and rename both methods so that the overall meaning is more clear.
Created attachment 240359 [details] [review] workspace: Lay out windows based on the real allocation Instead of doing an entire recalculation of window positions when sliding the thumbnails box, simply recalculate the position and scale with basic aspect ratio math. This also ensures that windows won't miraculously swap positions, even if we reposition windows while the thumbnails box is expanded.
*** Bug 697906 has been marked as a duplicate of this bug. ***
Review of attachment 240343 [details] [review]: Looks good to me.
Review of attachment 240344 [details] [review]: Sure
Review of attachment 240345 [details] [review]: ++
Review of attachment 240346 [details] [review]: ++
Review of attachment 240347 [details] [review]: ++
Review of attachment 240348 [details] [review]: ++
Review of attachment 240349 [details] [review]: ::: js/ui/workspace.js @@ +1015,2 @@ let clones = this._windows.slice(); + if (clones.length === 0) Why === matters here? @@ +1520,3 @@ + // All of the overlays have the same chrome sizes, + // so just pick the first one. + let overlay = this._windowOverlays[0]; You're assuming this._windows and this._windowOverlays have the same number of elements here, right?
Review of attachment 240350 [details] [review]: Looks good
Review of attachment 240351 [details] [review]: ++
Review of attachment 240352 [details] [review]: ++
Review of attachment 240353 [details] [review]: OK
Review of attachment 240354 [details] [review]: ::: js/ui/overviewControls.js @@ +544,3 @@ + let spacing = this.actor.get_theme_node().get_length('spacing'); + let dashWidth = this._dashSlider.getVisibleWidth() + spacing; + let thumbnailsWidth = this._thumbnailsSlider.getVisibleWidth() + spacing; getVisibleWidth() for the ThumbnailsSlider actually respects the alwaysZoomOut() policy as far as I can see; are you sure this really does what you want?
Review of attachment 240355 [details] [review]: ++
Review of attachment 240356 [details] [review]: Sure
Review of attachment 240357 [details] [review]: OK ::: js/ui/workspace.js @@ +1040,3 @@ let animate = flags & WindowPositionFlags.ANIMATE; + let layout = this._currentLayout; Didn't you remove this._currentLayout in a previous patch? You re-introduce it later though, so maybe this should be part of another patch.
Review of attachment 240358 [details] [review]: ::: js/ui/workspace.js @@ +1037,3 @@ clones.push(this._reservedSlot); + this._currentLayout = this._computeLayout(clones); Shouldn't this be part of a different patch as well?
Review of attachment 240359 [details] [review]: ::: js/ui/workspace.js @@ +924,3 @@ + // The actual geometry is the geometry we need to arrange windows + // in. If this is less than the full geometry, we'll simply do Nitpick: comment should probably say "if the area is smaller than" instead of "this is less than". ::: js/ui/workspacesView.js @@ +58,2 @@ this._fullGeometry = null; + this._actualGeometry = null; I feel like you don't really need to store this state here; the rectEqual check is the only place this._actualGeometry is used, and it could be moved down to Workspace instead (on the other hand if you did this to keep the fullGeometry and actualGeometry code paths consistent, it's probably worth keeping it this way).
Attachment 240343 [details] pushed as 8772edc - overview: Move the group construction to the controls manager Attachment 240344 [details] pushed as d7c377c - workspace: Use Workspace.WindowPositionFlags.NONE in another case Attachment 240345 [details] pushed as e981cae - workspace: Don't save the current layout Attachment 240346 [details] pushed as 7e5d8a8 - workspace: Make positionWindows private Attachment 240347 [details] pushed as ada70dd - workspace: Remove more dead code Attachment 240348 [details] pushed as b41902f - workspace: Only create one strategy Attachment 240349 [details] pushed as 1dac4d0 - workspace: Abort relayouting much earlier Attachment 240350 [details] pushed as caaac9b - workspace: Do window slot computing in three steps Attachment 240351 [details] pushed as 7cb1201 - workspacesView: Fix indentation Attachment 240352 [details] pushed as 2506673 - workspacesView: Minor cleanup Attachment 240353 [details] pushed as e3957f3 - workspacesView: Make setGeometry take a rectangle Pushing these for now. (In reply to comment #29) > Review of attachment 240349 [details] [review]: > > ::: js/ui/workspace.js > @@ +1015,2 @@ > let clones = this._windows.slice(); > + if (clones.length === 0) > > Why === matters here? It doesn't. It was just a habit I picked up in my web developer world. Removed. > @@ +1520,3 @@ > + // All of the overlays have the same chrome sizes, > + // so just pick the first one. > + let overlay = this._windowOverlays[0]; > > You're assuming this._windows and this._windowOverlays have the same number of > elements here, right? As far as I can tell, that should always be the case -- items are added and removed at the same time.
Created attachment 241988 [details] [review] overviewControls: Don't respect alwaysZoomOut in getVisibleWidth The slide will already take care of this, so it doesn't seem necessary to me.
Created attachment 241989 [details] [review] workspacesView: Calculate the workspaces geometry ourselves To ensure that we don't recalculate window layouts when zooming in or out, we need to always pass the full geometry. This will break window repositioning when we zoom back in; for the purposes of commit clarity, this breaks this feature for now. It will be added back soon.
Created attachment 241990 [details] [review] workspace: Make room for a second geometry to keep track of As we want to eventually track two geometries, we need to rename our very plain "_x, _y, _width, _height". While we could just prefix them, I think that stuffing them in an object makes more sense. At the same time, make the variable and method name more descriptive by adding such a prefix, as well as a bit of documentation.
Created attachment 241991 [details] [review] workspace: Separate out spacing/padding code This will be used when we introduce the second geometry.
Created attachment 241992 [details] [review] workspace: Calculate the window slots when we reposition Repositioning will eventually be separated from recalculation to accomodate two different geometries, so we'll need to do the padding and area manipulation in two different areas.
Created attachment 241993 [details] [review] workspace: Split out window repositioning logic and rename Split out the part that moves the window clones around from the part that calculates the window clone positions, and rename both methods so that the overall meaning is more clear.
Created attachment 241994 [details] [review] workspace: Lay out windows based on the real allocation Instead of doing an entire recalculation of window positions when sliding the thumbnails box, simply recalculate the position and scale with basic aspect ratio math. This also ensures that windows won't miraculously swap positions, even if we reposition windows while the thumbnails box is expanded.
currentLayout rebase blunders should be fixed. (In reply to comment #39) > Review of attachment 240359 [details] [review]: > > ::: js/ui/workspace.js > @@ +924,3 @@ > > + // The actual geometry is the geometry we need to arrange windows > + // in. If this is less than the full geometry, we'll simply do > > Nitpick: comment should probably say "if the area is smaller than" instead of > "this is less than". I forgot to fix it in the reattached patch here. Fixed locally. > ::: js/ui/workspacesView.js > @@ +58,2 @@ > this._fullGeometry = null; > + this._actualGeometry = null; > > I feel like you don't really need to store this state here; the rectEqual check > is the only place this._actualGeometry is used, and it could be moved down to > Workspace instead (on the other hand if you did this to keep the fullGeometry > and actualGeometry code paths consistent, it's probably worth keeping it this > way). I indeed did this to make it consistent with fullGeometry.
Review of attachment 241988 [details] [review]: I believe this would break the logic used by _getTranslation().
Review of attachment 241989 [details] [review]: This looks okay, pending resolution of the previous patch.
Review of attachment 241990 [details] [review]: Looks good.
Review of attachment 241991 [details] [review]: Yep
Review of attachment 241992 [details] [review]: Looks good
Review of attachment 241993 [details] [review]: OK ::: js/ui/workspace.js @@ +1037,3 @@ clones.push(this._reservedSlot); + this._currentLayout = this._computeLayout(clones); Can you initialize this to null in _init?
Review of attachment 241994 [details] [review]: Did you mean "mysteriously" instead of "miraculously" in the commit message? Patch looks good to me.
Created attachment 242024 [details] [review] overviewControls: Add an accessor for the visible-width property To add a geometry that's independent of the slide factor of the workspace, we need to get this from outside the sliding control.
Created attachment 242025 [details] [review] workspacesView: Calculate the workspaces geometry ourselves To ensure that we don't recalculate window layouts when zooming in or out, we need to always pass the full geometry. This will break window repositioning when we zoom back in; for the purposes of commit clarity, this breaks this feature for now. It will be added back soon.
Created attachment 242026 [details] [review] workspace: Make room for a second geometry to keep track of As we want to eventually track two geometries, we need to rename our very plain "_x, _y, _width, _height". While we could just prefix them, I think that stuffing them in an object makes more sense. At the same time, make the variable and method name more descriptive by adding such a prefix, as well as a bit of documentation.
Created attachment 242027 [details] [review] workspace: Separate out spacing/padding code This will be used when we introduce the second geometry.
Created attachment 242028 [details] [review] workspace: Calculate the window slots when we reposition Repositioning will eventually be separated from recalculation to accomodate two different geometries, so we'll need to do the padding and area manipulation in two different areas.
Created attachment 242029 [details] [review] workspace: Split out window repositioning logic and rename Split out the part that moves the window clones around from the part that calculates the window clone positions, and rename both methods so that the overall meaning is more clear.
Created attachment 242030 [details] [review] workspace: Lay out windows based on the real allocation Instead of doing an entire recalculation of window positions when sliding the thumbnails box, simply recalculate the position and scale with basic aspect ratio math. This also ensures that windows won't miraculously swap positions, even if we reposition windows while the thumbnails box is expanded.
Review of attachment 242024 [details] [review]: Yes
Review of attachment 242025 [details] [review]: Okay
Review of attachment 242026 [details] [review]: Okay
Review of attachment 242027 [details] [review]: Okay
Review of attachment 242028 [details] [review]: Okay
Review of attachment 242029 [details] [review]: Okay
Review of attachment 242030 [details] [review]: ::: js/ui/workspace.js @@ +993,3 @@ this._dropRect.set_size(geom.width, geom.height); + if (this._currentLayout != null) + this._updateWindowPositions(WindowPositionFlags.NONE); Why do you this inside a Meta.later_add()?
(In reply to comment #69) > Review of attachment 242030 [details] [review]: > > ::: js/ui/workspace.js > @@ +993,3 @@ > this._dropRect.set_size(geom.width, geom.height); > + if (this._currentLayout != null) > + this._updateWindowPositions(WindowPositionFlags.NONE); > > Why do you this inside a Meta.later_add()? Because setActualGeometry is called from notify::allocation, which means that we'll get allocation cycle warnings if we don't punt to an idle. Effectively, what we eventually want is a layout manager instead of this, but that's a separate patch set.
Comment on attachment 241988 [details] [review] overviewControls: Don't respect alwaysZoomOut in getVisibleWidth This is obsolete
Review of attachment 242030 [details] [review]: Sounds good then
Attachment 242024 [details] pushed as fc53a25 - overviewControls: Add an accessor for the visible-width property Attachment 242025 [details] pushed as f0c2ad0 - workspacesView: Calculate the workspaces geometry ourselves Attachment 242026 [details] pushed as b925322 - workspace: Make room for a second geometry to keep track of Attachment 242027 [details] pushed as 65eb5a3 - workspace: Separate out spacing/padding code Attachment 242028 [details] pushed as 59ba550 - workspace: Calculate the window slots when we reposition Attachment 242029 [details] pushed as bde8cc3 - workspace: Split out window repositioning logic and rename Attachment 242030 [details] pushed as c37259b - workspace: Lay out windows based on the real allocation
I can reproduce this with F19's 3.8 shell.
Yep, it seems the problem came back (recently ?). I'm running gnome-shell-3.8.4-2.fc19.x86_64. REOPENING
Probably fixed by bug #707197