GNOME Bugzilla – Bug 640996
Use thumbnails for workspace management in the overview
Last modified: 2011-02-17 09:18:16 UTC
Here is a pretty complete implementation of the designs from: http://jimmac.musichall.cz/log/?p=1125 http://jimmac.musichall.cz/log/?p=1126 There are a few stray bugs floating around. More major issues I'm aware of: - Newly positioned windows aren't positioned correctly in the workspace thumbnails - There is no handling of overflow if more workspaces are created than fit (need design) - If you have a workspace with a stack of maximized windows, then the accurate preview of the workspace which just shows the top window isn't very representative and maybe we need to do something different like show the spread-out view. (need design)
Created attachment 179676 [details] [review] Switch to a vertical layout for workspaces The new plans for a row of workspace thumbnails on the right side of the overview means that the mental model we present to the user will be vertical, so switch the Metacity workspace layout to be vertical and adjust the keybinding handling, animations, and workspace layout in the overview to match. (This commit does not change the workspace switching indicator pending finalization of what we want to do with that - it still shows workspaces arranged vertically.)
Created attachment 179677 [details] [review] Remove workspace indicators Once we have workspace thumbnails in the overview, workspace indicators will no longer be needed.
Created attachment 179678 [details] [review] Don't check for Workspaces.WindowClone for window drops When checking the type of a DND source, instead of checking 'instanceof Workspaces.WindowClone' accept any actor with realWindow and metaWindow properties. This will be useful to support a separate type of actor dragged from workspace thumbnails.
Created attachment 179679 [details] [review] Use a single "zoomed out" view for both workspace controls hover and DND Instead of having a separation between popping the controls out on hover and zooming out for DND, always do both at once. This is necessary because when we added workspace thumbnails the controls will get bigger, so we need to make sure we zoom out far enough so that the windows don't overlap the controls.
Created attachment 179680 [details] [review] Move restacking handling from WorkspacesView to WorkspacesDisplay Moving the base tracking of restacking to WorkspacesDisplay will allow us to use it to update stacking in the workspace thumbnails as well as in the main workspaces.
Created attachment 179681 [details] [review] Add workspace thumbnails to the overview Add workspace thumbnails to the workspace controls area. The user can click on the thumbnail to switch workspaces and can also drag windows out of the thumbnail to other workspaces.
Created attachment 179682 [details] [review] Don't activate newly added workspaces With workspace thumbnails, we want to make workspace switching something that happens largely under the users control, so don't switch to newly added workspaces in the overview.
Created attachment 179683 [details] [review] Add automatic workspace management Automatically add and remove workspaces so that the last workspace is always empty and no workspaces are empty other than that workspace.
Created attachment 179684 [details] [review] Remove now unnecessary workspace controls With automatic workspace management, explicit controls to add and remove workspaces are no longer necessary.
Created attachment 179685 [details] [review] Avoid popping the workspace controls in and out at the end of DND At the end of a drag operation, we would invoke the code to slide the controls in (because we were no longer DND'ing and not hovering) and then immediately afterwards invoke the code to slide it back out when we got the ENTER event from the end of DND. While the immediately overridden tween probably won't have any visible effect it's better to avoid this, so wait to update the zoom state until BEFORE_REDRAW.
Created attachment 179686 [details] [review] Don't switch to a workspace when dragging it to launch on that workspace With workspace thumbnails, we don't switch workspaces when dragging windows between workspaces or adding new workspaces, so we also shouldn't switch on launch. * Add workspace parameters to shell_doc_system_open(), shell_app_activate, shell_app_open_new_window() * Pass a 'params' object when activating items in the overview with two currently defined parameters: workspace and timestamp. (timestamp is only implemented where it is easy and doesn't require interface changes - using the global current timestamp for the shell is almost always right or at least good enough.)
Created attachment 179687 [details] [review] Improve workspace controls slide-in positioning Intead of using a St.Group and tweening the position of the controls actor, use a St.GenericLayout and tween a Javascript property. This allows us to more reliably track the height of the overall workspace display and propagate it to the controls actor.
Created attachment 179962 [details] [review] Switch to a vertical layout for workspaces (Various minor changes, rebased to master, reattaching everything to maintain order) The new plans for a row of workspace thumbnails on the right side of the overview means that the mental model we present to the user will be vertical, so switch the Metacity workspace layout to be vertical and adjust the keybinding handling, animations, and workspace layout in the overview to match. (This commit does not change the workspace switching indicator pending finalization of what we want to do with that - it still shows workspaces arranged vertically.)
Created attachment 179963 [details] [review] Remove workspace indicators Once we have workspace thumbnails in the overview, workspace indicators will no longer be needed.
Created attachment 179964 [details] [review] Don't check for Workspaces.WindowClone for window drops When checking the type of a DND source, instead of checking 'instanceof Workspaces.WindowClone' accept any actor with realWindow and metaWindow properties. This will be useful to support a separate type of actor dragged from workspace thumbnails.
Created attachment 179965 [details] [review] Use a single "zoomed out" view for both workspace controls hover and DND Instead of having a separation between popping the controls out on hover and zooming out for DND, always do both at once. This is necessary because when we added workspace thumbnails the controls will get bigger, so we need to make sure we zoom out far enough so that the windows don't overlap the controls.
Created attachment 179966 [details] [review] Move restacking handling from WorkspacesView to WorkspacesDisplay Moving the base tracking of restacking to WorkspacesDisplay will allow us to use it to update stacking in the workspace thumbnails as well as in the main workspaces.
Created attachment 179967 [details] [review] Add workspace thumbnails to the overview Add workspace thumbnails to the workspace controls area. The user can click on the thumbnail to switch workspaces and can also drag windows out of the thumbnail to other workspaces.
Created attachment 179968 [details] [review] Don't activate newly added workspaces With workspace thumbnails, we want to make workspace switching something that happens largely under the users control, so don't switch to newly added workspaces in the overview.
Created attachment 179969 [details] [review] Add automatic workspace management Automatically add and remove workspaces so that the last workspace is always empty and no workspaces are empty other than that workspace.
Created attachment 179970 [details] [review] Remove now unnecessary workspace controls With automatic workspace management, explicit controls to add and remove workspaces are no longer necessary.
Created attachment 179971 [details] [review] Avoid popping the workspace controls in and out at the end of DND At the end of a drag operation, we would invoke the code to slide the controls in (because we were no longer DND'ing and not hovering) and then immediately afterwards invoke the code to slide it back out when we got the ENTER event from the end of DND. While the immediately overridden tween probably won't have any visible effect it's better to avoid this, so wait to update the zoom state until BEFORE_REDRAW.
Created attachment 179972 [details] [review] Don't switch to a workspace when dragging it to launch on that workspace With workspace thumbnails, we don't switch workspaces when dragging windows between workspaces or adding new workspaces, so we also shouldn't switch on launch. * Add workspace parameters to shell_doc_system_open(), shell_app_activate, shell_app_open_new_window() * Pass a 'params' object when activating items in the overview with two currently defined parameters: workspace and timestamp. (timestamp is only implemented where it is easy and doesn't require interface changes - using the global current timestamp for the shell is almost always right or at least good enough.)
Created attachment 179973 [details] [review] Improve workspace controls slide-in positioning Intead of using a St.Group and tweening the position of the controls actor, use a St.GenericLayout and tween a Javascript property. This allows us to more reliably track the height of the overall workspace display and propagate it to the controls actor.
Created attachment 179974 [details] [review] Handle changes in window position for workspace thumbnails Connect to the 'position-set' signal of MetaWindowActor and move actors when the source windows move.
Review of attachment 179962 [details] [review]: Mostly makes sense to me. ::: js/ui/windowManager.js @@ -528,3 @@ - /* We don't support this kind of layout */ - if (binding == 'switch_to_workspace_up' || binding == 'switch_to_workspace_down') - return; I think it would make sense to exclude 'switch_to_workspace_left'/'switch_to_workspace_right' - it feels rather weird that Ctrl-Alt-Left/Right can be used to switch workspaces up/down, especially as Ctrl-Alt-Shift-Left/Right doesn't work anymore. (I think it makes sense though to keep the code for actionMoveWorkspaceLeft()/actionMoveWorkspaceRight() just in case) ::: js/ui/workspacesView.js @@ +304,3 @@ + workspace.x = this._x + this._activeWorkspaceX; + workspace.y = this._y + this._activeWorkspaceY + + (w - active) * (_height + this._spacing); Misaligned.
Review of attachment 179963 [details] [review]: Good, just one minor comment: ::: js/ui/workspacesView.js @@ +820,2 @@ this._workspacesBin = new St.Bin(); workspacesBox.add(this._workspacesBin, { expand: true }); workspacesBox is pretty pointless now - if this._workspacesBin is added directly to this.actor, it can be removed.
Review of attachment 179964 [details] [review]: Good except for one minor comment ::: js/ui/appDisplay.js @@ +632,2 @@ _findMetaWindowForActor: function (actor) { + if (actor._delegate.metaWindow) We probably don't want to rely on actor._delegate being defined, so if (actor._delegate && actor._delegate.metaWindow) ... should be safer.
Review of attachment 179965 [details] [review]: Mostly good - not sure if it makes sense to address the dual-monitor issue now, all other comments are trivial (marked needs-work because of the exception). ::: js/ui/workspacesView.js @@ +34,3 @@ this.actor.set_clip(x, y, width, height); + // The actor itself isn't a drop target, so we don't want to pick on its aea Typo: s/aea/area @@ +35,3 @@ + // The actor itself isn't a drop target, so we don't want to pick on its aea + this.actor.set_size(0, 0); With this change the call to set_clip() above becomes pointless, right? @@ +332,3 @@ + // except when we are switching between workspaces + workspace.y = this._y + this._activeWorkspaceY + + (w - active) * (_height + this._spacing) / zoomScale; Still misaligned. @@ -706,3 @@ - this._controls.track_hover = true; - this._controls.connect('notify::hover', - Lang.bind(this, this._onHoverChanged)); The removals here look a bit arbitrary - all of WorkspaceControlsContainer is now unused, so the entire prototype should be removed. @@ -856,2 @@ show: function() { - this._controlsContainer.show(); There's a stray reference to this._controlsContainer in hide() resulting in an exception when leaving the overview. @@ +852,3 @@ + let totalWidth = totalAllocation.x2 - totalAllocation.x1; + // XXXX: 50 is just a hack for message tray compensation + let totalHeight = totalAllocation.y2 - totalAllocation.y1 - 50; Why? The viewSelector should already be sized correctly in the overview (the spacing is wrong the first time it's shown, which is a separate bug, but there's still no overlap with the message tray ...) @@ +880,3 @@ + x += controlsReserved; + + this._controls.x = this._getControlsX(); It's a bit ugly to have the controls "leak" to the 2nd monitor in dual-monitor setups. (But then, currently dual-monitor is broken in many interesting ways anyway) @@ +991,3 @@ + }, + + _dragBegin: function() { Currently unused - it would make sense to me to connect the drag signals in this patch instead of leaving it for the "Add workspace thumbnails to the overview" one
Review of attachment 179966 [details] [review]: Good, except for the sneaked in code from a later patch causing an exception ;-) ::: js/ui/workspacesView.js @@ +918,3 @@ + this.workspacesView.syncStacking(stackIndices); + for (let i = 0; i < this._workspaceThumbnails.length; i++) + this._workspaceThumbnails[i].syncStacking(stackIndices); Wrong patch.
Review of attachment 179967 [details] [review]: I recall some discussions about re-orderable thumbnails, but having tried this patch, the "classic" workspace-switcher behavior of draggable window previews makes more sense to me. Jakub kind of agreed with that, too. There are some more differences to the mockups, but addressing those later should be fine. ::: data/theme/gnome-shell.css @@ +273,3 @@ +.workspace-thumbnails { + spacing: 7px; + padding: 8px; Any particular reason for the 1px difference? ::: js/ui/workspaceThumbnail.js @@ +13,3 @@ + +// Fraction of original screen size for thumbnails +let THUMBNAIL_SCALE = 1/8.; According to the latest mockups[0], the thumbnail scale should be variable to avoid overflowing the controls (similar to the dash I guess). Probably OK to leave it fixed for now to get this landed. [0] http://live.gnome.org/GnomeShell/Design/Whiteboards/OverviewWindowDND @@ +22,3 @@ + _init : function(realWindow) { + this.actor = new Clutter.Clone({ source: realWindow.get_texture(), + clip_to_allocation: true, It's not obvious to me why the clipping would be needed - if it is, a comment would be helpful. @@ +29,3 @@ + this.realWindow = realWindow; + this.metaWindow = realWindow.meta_window; + this.metaWindow._delegate = this; So - depending on the order Workspaces/WorkspaceThumbnails are created, metaWindow._delegate either points to Workspace.WindowClone or WorkspaceThumbnail.WindowClone ... @@ +56,3 @@ + + destroy: function () { + this.actor.destroy(); There's quite a bit of code duplication with Workspace.WindowClone, not sure though if it's enough to warrant a common base type. @@ +110,3 @@ +WorkspaceThumbnail.prototype = { + _init : function(metaWorkspace) { + // When dragging a window, we use this slot for reserve space. Left-over comment from copy-paste. @@ +132,3 @@ + + this._background = new Clutter.Clone({ source: global.background_actor }); + this._group.add_actor(this._background); Having the background shine through where the panel is supposed to be looks a bit weird to me (in particular when there's a maximized window). Adding a panel clone is probably overkill, I imagine a panel-sized bin with style class 'panel:in-overview' would look close enough. @@ +154,3 @@ + }, + + _lookupIndex: function (metaWindow) { Same question about code duplication as for WindowClone. ::: js/ui/workspacesView.js @@ +812,3 @@ + this._thumbnailIndicatorConstraints.push(new Clutter.BindConstraint({ coordinate: Clutter.BindCoordinate.Y })); + this._thumbnailIndicatorConstraints.push(new Clutter.BindConstraint({ coordinate: Clutter.BindCoordinate.WIDTH })); + this._thumbnailIndicatorConstraints.push(new Clutter.BindConstraint({ coordinate: Clutter.BindCoordinate.HEIGHT })); Clutter now has BindCoordinate.POSITION and BindCoordinate.SIZE ... @@ +863,3 @@ + // The thumbnails indicator actually needs to be on top of the thumbnails, but + // there is also something more subtle going on as well - actors in a StBoxLayout + // are allocated from bottom to to top (start to end), and we need the "to to" @@ +923,3 @@ + if (this._itemDragBeginId == 0) + this._itemDragBeginId = Main.overview.connect('item-drag-begin', + Lang.bind(this, this._dragBegin)); As noted before, I think that code belongs to an earlier patch ... @@ +1021,3 @@ _onRestacked: function() { let stack = global.get_window_actors(); let stackIndices = {}; ... and the stack syncing for thumbnails in the previous patch should be here. @@ +1050,3 @@ + let thumbnail = new WorkspaceThumbnail.WorkspaceThumbnail(metaWorkspace); + this._workspaceThumbnails[w] = thumbnail; + this._thumbnailsBox.add(thumbnail.actor); Jimmac has a post with a video-mockup[0] where the thumbnails slide in/out. Still makes sense to me, but I guess we can still do it after landing the initial version. [0] http://jimmac.musichall.cz/log/?p=1107
Review of attachment 179968 [details] [review]: Looks good.
Review of attachment 179969 [details] [review]: Looks good. ::: js/ui/workspacesView.js @@ +1080,3 @@ + // If we removed the current workspace then we'll be animating workspace indicator + // to an adjacent workspace, but that is wrong, since now that adjacent workspace + // is in the current slot, so remove the animation Looks like the indicator only is in the right slot for the first workspace, otherwise the indicator is moved up ("move" as in "jump" ...) I think it's best addressed later when dealing with animations for thumbnail addition/removal.
Review of attachment 179970 [details] [review]: Looks good, except for a little oversight: ::: js/ui/workspacesView.js @@ -228,3 @@ - }, - - addWorkspace: function() { This function is still used in appDisplay.js - probably best to change the code there to not add a workspace, but launch on the last one.
Review of attachment 179971 [details] [review]: Sounds good.
Review of attachment 179972 [details] [review]: Looks good. ::: js/ui/appDisplay.js @@ +226,3 @@ + activateResult: function(id, params) { + params = Params.parse(params, { workspace: null, + timestamp: null }); The timestamp parameter is never used. @@ +234,3 @@ + dragActivateResult: function(id, params) { + params = Params.parse(params, { workspace: null, + timestamp: null }); Dto. @@ +403,1 @@ if (newWorkspace != null) { That part stopped working as noted in a comment on a previous patch. ::: js/ui/placeDisplay.js @@ +170,3 @@ }, + function (params) { + // BUG: nautilus-connect-server doesn't have a desktop file, so we can' can't ::: js/ui/workspace.js @@ +1423,3 @@ } else if (source.shellWorkspaceLaunch) { + source.shellWorkspaceLaunch({ workspace: this.metaWorkspace, + timestamp: time }); The same change should be in WorkspaceThumbnail. (It's done in a later patch, but really belongs here)
Review of attachment 179974 [details] [review]: Good, except for one comment: ::: js/ui/workspaceThumbnail.js @@ +300,3 @@ } else if (source.shellWorkspaceLaunch) { + source.shellWorkspaceLaunch({ workspace: this.metaWorkspace, + timestamp: time }); Belongs in a previous patch.
Review of attachment 179973 [details] [review]: Looks mostly good, but causes critical clutter warnings. ::: js/ui/workspacesView.js @@ +859,3 @@ this._constrainThumbnailIndicator(); this._zoomOut = false; + this._zoomFraction = 0; This should be moved to _init(), because @@ +934,3 @@ + + // Amount of space on the screen we reserve for the visible control + let controlsReserved = controlsNatural * (1 - (1 - this._zoomFraction) * CONTROLS_POP_IN_FRACTION); otherwise _zoomFactor is undefined the first time the actor is allocated, resulting in the following warnings: (mutter:8494): Clutter-CRITICAL **: clutter_paint_volume_set_width: assertion `width >= 0.0f' failed (mutter:8494): Clutter-CRITICAL **: clutter_paint_volume_set_width: assertion `width >= 0.0f' failed
Review of attachment 179974 [details] [review]: ::: js/ui/workspaceThumbnail.js @@ +71,3 @@ + if (this._positionChangedId != 0) { + this.realWindow.disconnect(this._positionChangedId); Another issue I noticed: this is the same issue fixed for Workspace.WindowClone in commit 00df20c6184b6e, e.g. gsignal throws a warning if the clone is destroyed because the original window has been closed.
(In reply to comment #26) > Review of attachment 179962 [details] [review]: > > Mostly makes sense to me. > > ::: js/ui/windowManager.js > @@ -528,3 @@ > - /* We don't support this kind of layout */ > - if (binding == 'switch_to_workspace_up' || binding == > 'switch_to_workspace_down') > - return; > > I think it would make sense to exclude > 'switch_to_workspace_left'/'switch_to_workspace_right' - it feels rather weird > that Ctrl-Alt-Left/Right can be used to switch workspaces up/down, especially > as Ctrl-Alt-Shift-Left/Right doesn't work anymore. > > (I think it makes sense though to keep the code for > actionMoveWorkspaceLeft()/actionMoveWorkspaceRight() just in case) Done, though it's a bit funny until we fix the popup (need final design for what we want to do there)
(In reply to comment #27) > Review of attachment 179963 [details] [review]: > > Good, just one minor comment: > > ::: js/ui/workspacesView.js > @@ +820,2 @@ > this._workspacesBin = new St.Bin(); > workspacesBox.add(this._workspacesBin, { expand: true }); > > workspacesBox is pretty pointless now - if this._workspacesBin is added > directly to this.actor, it can be removed. It's removed in a later patch, not worth moving the removal between patches.
(In reply to comment #29) > @@ +35,3 @@ > > + // The actor itself isn't a drop target, so we don't want to pick on > its aea > + this.actor.set_size(0, 0); > > With this change the call to set_clip() above becomes pointless, right? Don't see any reason that would be the case. The set_clip() call is probably pointless for other reasons - we shouldn't be lapping anything out of the workspacesView bounds anyways, but it's independent of the size of the group - remember that the children of a StGroup can be positioned outside the bounds of the group. > @@ -706,3 @@ > - this._controls.track_hover = true; > - this._controls.connect('notify::hover', > - Lang.bind(this, this._onHoverChanged)); > > The removals here look a bit arbitrary - all of WorkspaceControlsContainer is > now unused, so the entire prototype should be removed. Yeah, removed. > @@ -856,2 @@ > show: function() { > - this._controlsContainer.show(); > > There's a stray reference to this._controlsContainer in hide() resulting in an > exception when leaving the overview. Fixed. > @@ +852,3 @@ > + let totalWidth = totalAllocation.x2 - totalAllocation.x1; > + // XXXX: 50 is just a hack for message tray compensation > + let totalHeight = totalAllocation.y2 - totalAllocation.y1 - 50; > > Why? The viewSelector should already be sized correctly in the overview (the > spacing is wrong the first time it's shown, which is a separate bug, but > there's still no overlap with the message tray ...) I debugged this when coming up with the 'Improve workspace controls slide-in positioning' but know I forget the details - I think it's basically that because various elements are hidden when the overview is not showing allocation doesn't propagate. In any case, the above is just a hack to make things usable that is fixd up later in the patch set, so going to leave it. > @@ +880,3 @@ > + x += controlsReserved; > + > + this._controls.x = this._getControlsX(); > > It's a bit ugly to have the controls "leak" to the 2nd monitor in dual-monitor > setups. > > (But then, currently dual-monitor is broken in many interesting ways anyway) Probably ore than a bit ugly, but the entire design with a hot edge doesn't really work in dual monitor... I'm guessing we should just always show the thumbnails without the hover if there is a monitor to the right. Filed as bug 641877 > @@ +991,3 @@ > + }, > + > + _dragBegin: function() { > > Currently unused - it would make sense to me to connect the drag signals in > this patch instead of leaving it for the "Add workspace thumbnails to the > overview" one Yeah, splitting problem, fixed.
(In reply to comment #30) > Review of attachment 179966 [details] [review]: > > Good, except for the sneaked in code from a later patch causing an exception > ;-) > > ::: js/ui/workspacesView.js > @@ +918,3 @@ > + this.workspacesView.syncStacking(stackIndices); > + for (let i = 0; i < this._workspaceThumbnails.length; i++) > + this._workspaceThumbnails[i].syncStacking(stackIndices); > > Wrong patch. Fixed.
(In reply to comment #31) > Review of attachment 179967 [details] [review]: > > I recall some discussions about re-orderable thumbnails, but having tried this > patch, the "classic" workspace-switcher behavior of draggable window previews > makes more sense to me. Jakub kind of agreed with that, too. There are some > more differences to the mockups, but addressing those later should be fine. > > ::: data/theme/gnome-shell.css > @@ +273,3 @@ > +.workspace-thumbnails { > + spacing: 7px; > + padding: 8px; > > Any particular reason for the 1px difference? That's what the mockups had. I measured. > ::: js/ui/workspaceThumbnail.js > @@ +13,3 @@ > + > +// Fraction of original screen size for thumbnails > +let THUMBNAIL_SCALE = 1/8.; > > According to the latest mockups[0], the thumbnail scale should be variable to > avoid overflowing the controls (similar to the dash I guess). Probably OK to > leave it fixed for now to get this landed. Filed as bug Bug 641879 > [0] http://live.gnome.org/GnomeShell/Design/Whiteboards/OverviewWindowDND > > @@ +22,3 @@ > + _init : function(realWindow) { > + this.actor = new Clutter.Clone({ source: realWindow.get_texture(), > + clip_to_allocation: true, > > It's not obvious to me why the clipping would be needed - if it is, a comment > would be helpful. Oh, oops, this was supposed to be on workspaceThumbnail, where it hopefully is more obvious why it is needed. > @@ +29,3 @@ > + this.realWindow = realWindow; > + this.metaWindow = realWindow.meta_window; > + this.metaWindow._delegate = this; > > So - depending on the order Workspaces/WorkspaceThumbnails are created, > metaWindow._delegate either points to Workspace.WindowClone or > WorkspaceThumbnail.WindowClone ... Yeah, we just dont' need that backpointer here. Actually, I can't find where we need it for workspace.js anymore either, but leaving that for now. > @@ +56,3 @@ > + > + destroy: function () { > + this.actor.destroy(); > > There's quite a bit of code duplication with Workspace.WindowClone, not sure > though if it's enough to warrant a common base type. It felt bad to just cut-and-paste-and-diverge, but the complexity to make things subclassable to the extent of being able to add overlays, zooming, etc, didn't seem worth it and basically still don't to me. workspaceThumbnail.js is only 300 lines of code total. > @@ +110,3 @@ > +WorkspaceThumbnail.prototype = { > + _init : function(metaWorkspace) { > + // When dragging a window, we use this slot for reserve space. > > Left-over comment from copy-paste. Fixed. > @@ +132,3 @@ > + > + this._background = new Clutter.Clone({ source: global.background_actor > }); > + this._group.add_actor(this._background); > > Having the background shine through where the panel is supposed to be looks a > bit weird to me (in particular when there's a maximized window). Adding a panel > clone is probably overkill, I imagine a panel-sized bin with style class > 'panel:in-overview' would look close enough. Filed as bug 641880 > ::: js/ui/workspacesView.js > @@ +812,3 @@ > + this._thumbnailIndicatorConstraints.push(new Clutter.BindConstraint({ > coordinate: Clutter.BindCoordinate.Y })); > + this._thumbnailIndicatorConstraints.push(new Clutter.BindConstraint({ > coordinate: Clutter.BindCoordinate.WIDTH })); > + this._thumbnailIndicatorConstraints.push(new Clutter.BindConstraint({ > coordinate: Clutter.BindCoordinate.HEIGHT })); > > Clutter now has BindCoordinate.POSITION and BindCoordinate.SIZE ... OK, saves a tiny bit of code and adds some efficiency. > @@ +1050,3 @@ > + let thumbnail = new > WorkspaceThumbnail.WorkspaceThumbnail(metaWorkspace); > + this._workspaceThumbnails[w] = thumbnail; > + this._thumbnailsBox.add(thumbnail.actor); > > Jimmac has a post with a video-mockup[0] where the thumbnails slide in/out. > Still makes sense to me, but I guess we can still do it after landing the > initial version. > > [0] http://jimmac.musichall.cz/log/?p=1107 Filed as bug 641881
(In reply to comment #42) > Don't see any reason that would be the case. The set_clip() call is probably > pointless for other reasons - we shouldn't be lapping anything out of the > workspacesView bounds anyways, but it's independent of the size of the group - > remember that the children of a StGroup can be positioned outside the bounds of > the group. The group itself used to be screen-sized and the workspaces reactive ... anyway, the clip is still needed while scrolling between workspaces, so it was rather my comment being pointless.
(In reply to comment #33) > Review of attachment 179969 [details] [review]: > > Looks good. > > ::: js/ui/workspacesView.js > @@ +1080,3 @@ > + // If we removed the current workspace then we'll be animating > workspace indicator > + // to an adjacent workspace, but that is wrong, since now that > adjacent workspace > + // is in the current slot, so remove the animation > > Looks like the indicator only is in the right slot for the first workspace, > otherwise the indicator is moved up ("move" as in "jump" ...) > > I think it's best addressed later when dealing with animations for thumbnail > addition/removal. Took me a while to understand what you are saying here - you are saying if we remove anything other than the first workspace, then the animation is actually correct, because we switch to the workspace *before* not the workspace afterwards. You are right, so I tried implementing that just now, and it didn't look could, because if you have:; 1 2* 3 And remove 2* then what you see is an animation from 3 to 1 and you weren't on 3. So, yeah, switched the comment to: // If we removed the current workspace, then metacity will have already // switched to an adjacent workspace. Leaving the animation we // started in response to that around will look funny because it's an // animation for the *old* workspace configuration. So, kill it. // If we animate the workspace removal in the future, we should animate // the indicator as part of that.
(In reply to comment #36) > Review of attachment 179972 [details] [review]: > > Looks good. > > ::: js/ui/appDisplay.js > @@ +226,3 @@ > + activateResult: function(id, params) { > + params = Params.parse(params, { workspace: null, > + timestamp: null }); > > The timestamp parameter is never used. Yeah. It has to be in the parse() or it will be an error for the caller to pass it, but explained in the commit message: timestamp is only implemented where it is easy and doesn't require interface changes - using the global current timestamp for the shell is almost always right or at least good enough. Little sloppy, but IMO, OK. > ::: js/ui/workspace.js > @@ +1423,3 @@ > } else if (source.shellWorkspaceLaunch) { > + source.shellWorkspaceLaunch({ workspace: this.metaWorkspace, > + timestamp: time }); > > The same change should be in WorkspaceThumbnail. > > (It's done in a later patch, but really belongs here) Fixed.
(In reply to comment #34) > Review of attachment 179970 [details] [review]: > > Looks good, except for a little oversight: > > ::: js/ui/workspacesView.js > @@ -228,3 @@ > - }, > - > - addWorkspace: function() { > > This function is still used in appDisplay.js - probably best to change the code > there to not add a workspace, but launch on the last one. Good catch, suggestion makes sense, implemented.
(In reply to comment #38) > Review of attachment 179973 [details] [review]: > > Looks mostly good, but causes critical clutter warnings. > > ::: js/ui/workspacesView.js > @@ +859,3 @@ > this._constrainThumbnailIndicator(); > this._zoomOut = false; > + this._zoomFraction = 0; > > This should be moved to _init(), because > > @@ +934,3 @@ > + > + // Amount of space on the screen we reserve for the visible control > + let controlsReserved = controlsNatural * (1 - (1 - this._zoomFraction) > * CONTROLS_POP_IN_FRACTION); > > otherwise _zoomFactor is undefined the first time the actor is allocated, > resulting in the following warnings: > > (mutter:8494): Clutter-CRITICAL **: clutter_paint_volume_set_width: assertion > `width >= 0.0f' failed > > (mutter:8494): Clutter-CRITICAL **: clutter_paint_volume_set_width: assertion > `width >= 0.0f' failed Hmm, weird to be allocated without being shown even though the "Windows" page is the default page, guess we start off with no page visible, and allocate all the pages anyways because we have a "stack". Doesn't really matter - added zoomFraction initialization to init - we were already initializing zoomOut there and zoomFraction is closely tied to that.
(In reply to comment #39) > Review of attachment 179974 [details] [review]: > > ::: js/ui/workspaceThumbnail.js > @@ +71,3 @@ > > + if (this._positionChangedId != 0) { > + this.realWindow.disconnect(this._positionChangedId); > > Another issue I noticed: this is the same issue fixed for Workspace.WindowClone > in commit 00df20c6184b6e, e.g. gsignal throws a warning if the clone is > destroyed because the original window has been closed. Annoying. Borrowed your fix. Perhaps an argument for the shared base class.
Thanks for the careful review - lot of good stuff found there. I think most of the changes that I made in response are pretty straightforward and uncontroversial, and while I'm sure they could use a double-check, I don't think it's worth your time to spend time sorting through all the comments above and finding what patches need to be looked at again, so I'm going to go ahead and merge the branch. If you see anything above that looks wrong, feel free to comment and we can fix up with additional commits.
Closing this as other issues / patches are being discussed / reviewed in different bugs anyway.