GNOME Bugzilla – Bug 643786
The Workspace/WorkspacesView zooming and positioning is insane in the brain
Last modified: 2011-03-09 12:57:45 UTC
The way workspace zooming works atm is very weird, with a lot of scaling going on that is hard to understand. Additionally it has two major problems: It enforces that the workspaces region has the same aspect ratio as the full screen (including all monitors), and it has problems with window move distance calculations due to the different coordinate spaces used for overview slots and original window positions. I'm attaching a patch with a more detailed description.
Created attachment 182348 [details] [review] Restructure the way we handle positioning zooming in Workspace We currently show the workspace in the overview in a rectangle with the same aspect ratio as the screen. Originally this was probably done since it showed the desktop, but we don't do this anymore, and the positioning of the windows in the overview is strictly a grid, so its not in any way related to monitor geometry. Additionally, in the multihead case the screen aspect ratio is very different from the overview monitor geometry, so a lot of space is lost. So, instead we just fill the entire inner rectangle of the overview with the workspace. However, the way the zoom into and out of the workspace right now is by scaling the workspace so that it covers the entire monitor. This cannot really work anymore when the workspace is a different aspect ratio. Furthermore the coordinates of the window clone actors are of two very different types in the "original window" case and the "window in a slot case". One is screen relative, the other is workspace relative. This makes it very hard to compute the cost of window motion distance in computeWindowMotion. In order to handle this we change the way workspace actor positioning and scaling work. All workspace window clone actors are stored in true screen coordingates, both the original window positions and the in-a-slot ones. Global scaling of the workspace is never done, we just reposition everything in both the initial zoom and when the controls appear from the side. There is one issue in the initial and final animations, which is that the clip region we normally have for the workspacesView will limit the animation of the clones to/from the original positions, so we disable the clip region during these animations.
There is an issue with this patch. Having the workgroups be so large means we're covering the drop targets of other things. I'm working on a fix for this.
Created attachment 182451 [details] [review] Restructure the way we handle positioning zooming in Workspace We currently show the workspace in the overview in a rectangle with the same aspect ratio as the screen. Originally this was probably done since it showed the desktop, but we don't do this anymore, and the positioning of the windows in the overview is strictly a grid, so its not in any way related to monitor geometry. Additionally, in the multihead case the screen aspect ratio is very different from the overview monitor geometry, so a lot of space is lost. So, instead we just fill the entire inner rectangle of the overview with the workspace. However, the way the zoom into and out of the workspace right now is by scaling the workspace so that it covers the entire monitor. This cannot really work anymore when the workspace is a different aspect ratio. Furthermore the coordinates of the window clone actors are of two very different types in the "original window" case and the "window in a slot case". One is screen relative, the other is workspace relative. This makes it very hard to compute the cost of window motion distance in computeWindowMotion. In order to handle this we change the way workspace actor positioning and scaling work. All workspace window clone actors are stored in true screen coordingates, both the original window positions and the in-a-slot ones. Global scaling of the workspace is never done, we just reposition everything in both the initial zoom and when the controls appear from the side. There is one issue in the initial and final animations, which is that the clip region we normally have for the workspacesView will limit the animation of the clones to/from the original positions, so we disable the clip region during these animations.
New version makes the Workspace.actor not a drop target, and then instead adds a Clutter.Rectangle chile the real size and position of the drop target.
Was testing this to see if it fixed the long-standing bug where windows that end up at their original size in the overview are non-integer-positioned and fuzzed. And, yay! it does. But did note one issue which is that the DND snapback isn't working - if you start dragging a window and then release it without dropping it on a drop target, it turns back fully opaque, freezes in place, then pops back at the original location.
(In reply to comment #5) > But did note one issue which is that the DND snapback isn't working - if you > start dragging a window and then release it without dropping it on a drop > target, it turns back fully opaque, freezes in place, then pops back at the > original location. The dnd patch in bug 609258 fixes that, though the animation still ends up looking slightly odd (as discussed on IRC with Alex). Another issue I noticed: the positioning does not take the text direction into account, so in RTL locales the workspaces overlap the dash.
Created attachment 182798 [details] [review] Fix RTL positioning of workspace
Sorry about the RTL thing, that was a silly mistake.
Created attachment 182825 [details] [review] dnd: Avoid division by zero, etc for zero-size actors When rescaling due to a possible parent actor resize avoid problems if any of the sizes is zero.
Created attachment 182826 [details] [review] Restructure the way we handle positioning zooming in Workspace We currently show the workspace in the overview in a rectangle with the same aspect ratio as the screen. Originally this was probably done since it showed the desktop, but we don't do this anymore, and the positioning of the windows in the overview is strictly a grid, so its not in any way related to monitor geometry. Additionally, in the multihead case the screen aspect ratio is very different from the overview monitor geometry, so a lot of space is lost. So, instead we just fill the entire inner rectangle of the overview with the workspace. However, the way the zoom into and out of the workspace right now is by scaling the workspace so that it covers the entire monitor. This cannot really work anymore when the workspace is a different aspect ratio. Furthermore the coordinates of the window clone actors are of two very different types in the "original window" case and the "window in a slot case". One is screen relative, the other is workspace relative. This makes it very hard to compute the cost of window motion distance in computeWindowMotion. In order to handle this we change the way workspace actor positioning and scaling work. All workspace window clone actors are stored in true screen coordingates, both the original window positions and the in-a-slot ones. Global scaling of the workspace is never done, we just reposition everything in both the initial zoom and when the controls appear from the side. There is one issue in the initial and final animations, which is that the clip region we normally have for the workspacesView will limit the animation of the clones to/from the original positions, so we disable the clip region during these animations.
Created attachment 182828 [details] [review] dnd: Add a drag-cancelled signal This lets us start the workspace zoom out animation right when the snapback starts, rather than waiting for the snapback to finish.
Created attachment 182829 [details] [review] Don't rearrange dragged window when repositioning windows If we're dragging a window around and we need to reposition the windows, due to e.g. the sliding in of the thumbnails or some other reason, then we need to consider the original position of the dragged window, rather than the currend drag position. Otherwise we will unnecessarily rearrange the other windows for instance on snap-back if you moved the dragged window past some other window.
Created attachment 182830 [details] [review] Start the workspace zoom out immediately on dnd cancel. This means the snap-back animation happens at the same time as the zoom, which looks much better.
Review of attachment 182825 [details] [review]: One question, probably the 'etc' in the commit message: ::: js/ui/dnd.js @@ +483,3 @@ + let parentScale = 1.0; + if (parentScaledWidth != 0 && parentWidth != 0) + parentScale = parentScaledWidth / parentWidth; Obviously makes sense for parentWidth, but why do you guard against parentScaledWidth == 0?
Review of attachment 182826 [details] [review]: Overall a very nice cleanup, I like it. In addition to the comments below, I noticed that added windows are clipped during the animation - probably a simple question of setting the initial clone position to (this._x, this._y) in _windowAdded() ... ::: js/ui/overview.js @@ +541,1 @@ // Make the other elements fade in. Mmmh, does this comment still make sense? (same comment applies to hide()) ::: js/ui/workspace.js @@ +328,3 @@ this._windowClone = windowClone; this._parentActor = parentActor; + this._hidden = true; Nitpick, but that's not actually true - the label is not explicitly hidden, and thus visible when the overlay is added to the stage. @@ +560,2 @@ + this._dropRect = new Clutter.Rectangle({ opacity: 0 }); + this._dropRect._delegate = this; Mmmh, I don't quite see the reason for setting the actor's size to 0 and maintaining a separate drop rect? I think using this.actor.set_size(width, height) in setGeometry() should work just fine ... ::: js/ui/workspacesView.js @@ +165,3 @@ + + for (let w = 0; w < this._workspaces.length; w++) + this._workspaces[w].positionWindows(Workspace.WindowPositionFlags.ANIMATE); I don't think zoomOut()/zoomIn() still make a lot of sense here - the zoom effect is now done by updating the workspace geometry prior to calling this function; this._zoomOut becomes a mere safeguard against calling the function repeatedly. How about replacing both functions with a single updateWindowPositions()? @@ +202,2 @@ if (showAnimation) { + let params = { x: 0, Looks like x is not modified anywhere? No point in tweaning the property then, so it should be removed. @@ +590,3 @@ let totalAllocation = this.actor.allocation; + let width = totalAllocation.x2 - totalAllocation.x1; + let height = totalAllocation.y2 - totalAllocation.y1; x, y, width and height are no longer needed for the workspacesView (yay!), so the the entire code between this._thumbnailsBox.show(): and this._workspaces = []; is obsolete and should be removed. @@ +607,3 @@ + } + + let newView = new WorkspacesView(this._workspaces); Only personal preference, but as the constructor got much shorter, I'd get rid of the local newView variable (e.g. do this._workspacesView = new WorkspacesView() after destroying the old view). @@ +732,3 @@ let totalAllocation = this.actor.allocation; + let width = totalAllocation.x2 - totalAllocation.x1; + let height = totalAllocation.y2 - totalAllocation.y1; IMO, totalAllocation does not make a lot of sense without totalWidth/totalHeight
Review of attachment 182828 [details] [review]: Looks good to me. ::: js/ui/dnd.js @@ +500,2 @@ _cancelDrag: function(eventTime) { + this.emit('drag-cancelled', eventTime, false); What's the last parameter? It also looks unused, so probably better to just remove it.
Review of attachment 182829 [details] [review]: Makes sense.
Review of attachment 182830 [details] [review]: Makes sense UI-wise, just a minor nitpick in the code: ::: js/ui/workspacesView.js @@ +571,2 @@ this._inDrag = false; + this._cancelledDrag = false; Is this necessary? The original idea seems to be that this._inDrag is true between 'drag-begin' and 'drag-end', so this._cancelledDrag is needed as a test whether the drag has been cancelled - but as this._inDrag is set to false when the drag is cancelled, the variable looks useless now.
Review of attachment 182825 [details] [review]: ::: js/ui/dnd.js @@ +483,3 @@ + let parentScale = 1.0; + if (parentScaledWidth != 0 && parentWidth != 0) + parentScale = parentScaledWidth / parentWidth; If the parent width is zero, does it make sense to rescale the coordinates? They will always end up being 0,0 then, which is unlikely to ever be useful.
Review of attachment 182826 [details] [review]: ::: js/ui/overview.js @@ +541,1 @@ // Make the other elements fade in. Yes, this is the dash, notification area and thumbnails fading in/out ::: js/ui/workspace.js @@ +560,2 @@ + this._dropRect = new Clutter.Rectangle({ opacity: 0 }); + this._dropRect._delegate = this; Its not really the size that is problematic, its the position. The workspaces are put in the global.overlay_group at 0,0 (top left screen corner) in order to have the right window coord <-> clone coord correspondence. So, if we change the size of the workspace actor we'd be accepting drops in the wrong place. And if we also move it we'll create problems with the coordinates of the window clones.
Review of attachment 182828 [details] [review]: ::: js/ui/dnd.js @@ +500,2 @@ _cancelDrag: function(eventTime) { + this.emit('drag-cancelled', eventTime, false); Just leftover cut and paste, will delete.
Review of attachment 182830 [details] [review]: ::: js/ui/workspacesView.js @@ +571,2 @@ this._inDrag = false; + this._cancelledDrag = false; Oh, I left the this._inDrag = false there by mistake. In an initial version i did like you said and set inDrag to false when cancelling. That works for the workspacesdisplay. However it was problematic in some of the other objects that have a similar inDrag property, as they needed it set until the dragEnd. So, instead of behaving differently in different objects I added the _cancelledDrag thing, but forgot to remove this particular _indrag=false.
(In reply to comment #19) > If the parent width is zero, does it make sense to rescale the coordinates? > They will always end up being 0,0 then, which is unlikely to ever be useful. They'll end up at the parent's origin. If the parent has been resized to 0 during the drag, that's what I would expect from "Snap the actor back to its original position within its parent". The case where a Group or GenericContainer's size is explicitly set to 0, and paints all children outside its allocation (so the "apparent size" is non-zero) is special - the patch accounts for that special case, but ignores the "normal" case where the parent has been resized (and children are only drawn within the parent's allocation), so this needs at least a comment.
Created attachment 182939 [details] [review] Restructure the way we handle positioning zooming in Workspace We currently show the workspace in the overview in a rectangle with the same aspect ratio as the screen. Originally this was probably done since it showed the desktop, but we don't do this anymore, and the positioning of the windows in the overview is strictly a grid, so its not in any way related to monitor geometry. Additionally, in the multihead case the screen aspect ratio is very different from the overview monitor geometry, so a lot of space is lost. So, instead we just fill the entire inner rectangle of the overview with the workspace. However, the way the zoom into and out of the workspace right now is by scaling the workspace so that it covers the entire monitor. This cannot really work anymore when the workspace is a different aspect ratio. Furthermore the coordinates of the window clone actors are of two very different types in the "original window" case and the "window in a slot case". One is screen relative, the other is workspace relative. This makes it very hard to compute the cost of window motion distance in computeWindowMotion. In order to handle this we change the way workspace actor positioning and scaling work. All workspace window clone actors are stored in true screen coordingates, both the original window positions and the in-a-slot ones. Global scaling of the workspace is never done, we just reposition everything in both the initial zoom and when the controls appear from the side. There is one issue in the initial and final animations, which is that the clip region we normally have for the workspacesView will limit the animation of the clones to/from the original positions, so we disable the clip region during these animations.
Created attachment 182940 [details] [review] dnd: Add a drag-cancelled signal This lets us start the workspace zoom out animation right when the snapback starts, rather than waiting for the snapback to finish.
Created attachment 182941 [details] [review] Start the workspace zoom out immediately on dnd cancel. This means the snap-back animation happens at the same time as the zoom, which looks much better.
(In reply to comment #20) > Yes, this is the dash, notification area and thumbnails fading in/out I know what it does, the question is if a comment about "the other elements" is still useful with the comment about creating a zoom effect for the workspaces removed. > ::: js/ui/workspace.js > Its not really the size that is problematic, its the position. The workspaces > are put in the global.overlay_group at 0,0 (top left screen corner) in order to > have the right window coord <-> clone coord correspondence. Ah, OK.
Created attachment 182942 [details] [review] dnd: Avoid division by zero, etc for zero-size actors When rescaling due to a possible parent actor resize avoid problems if any of the sizes is zero.
(In reply to comment #23) > They'll end up at the parent's origin. If the parent has been resized to 0 > during the drag, that's what I would expect from "Snap the actor back to its > original position within its parent". The case where a Group or > GenericContainer's size is explicitly set to 0, and paints all children outside > its allocation (so the "apparent size" is non-zero) is special - the patch > accounts for that special case, but ignores the "normal" case where the parent > has been resized (and children are only drawn within the parent's allocation), > so this needs at least a comment. Agreed, i'll remove the parentscale zero check.
(In reply to comment #27) > (In reply to comment #20) > > Yes, this is the dash, notification area and thumbnails fading in/out > > I know what it does, the question is if a comment about "the other elements" is > still useful with the comment about creating a zoom effect for the workspaces > removed. I see what you mean, and i agree, will remove it.
Created attachment 182943 [details] [review] Restructure the way we handle positioning zooming in Workspace We currently show the workspace in the overview in a rectangle with the same aspect ratio as the screen. Originally this was probably done since it showed the desktop, but we don't do this anymore, and the positioning of the windows in the overview is strictly a grid, so its not in any way related to monitor geometry. Additionally, in the multihead case the screen aspect ratio is very different from the overview monitor geometry, so a lot of space is lost. So, instead we just fill the entire inner rectangle of the overview with the workspace. However, the way the zoom into and out of the workspace right now is by scaling the workspace so that it covers the entire monitor. This cannot really work anymore when the workspace is a different aspect ratio. Furthermore the coordinates of the window clone actors are of two very different types in the "original window" case and the "window in a slot case". One is screen relative, the other is workspace relative. This makes it very hard to compute the cost of window motion distance in computeWindowMotion. In order to handle this we change the way workspace actor positioning and scaling work. All workspace window clone actors are stored in true screen coordingates, both the original window positions and the in-a-slot ones. Global scaling of the workspace is never done, we just reposition everything in both the initial zoom and when the controls appear from the side. There is one issue in the initial and final animations, which is that the clip region we normally have for the workspacesView will limit the animation of the clones to/from the original positions, so we disable the clip region during these animations.
Review of attachment 182940 [details] [review]: Looks good.
Review of attachment 182941 [details] [review]: Yup.
Review of attachment 182943 [details] [review]: Go for it!
Attachment 182829 [details] pushed as 5743224 - Don't rearrange dragged window when repositioning windows Attachment 182940 [details] pushed as a80e88e - dnd: Add a drag-cancelled signal Attachment 182941 [details] pushed as df2f939 - Start the workspace zoom out immediately on dnd cancel. Attachment 182942 [details] pushed as 72120bb - dnd: Avoid division by zero, etc for zero-size actors Attachment 182943 [details] pushed as 0207f1f - Restructure the way we handle positioning zooming in Workspace