GNOME Bugzilla – Bug 593844
Alternative overview design idea
Last modified: 2010-11-04 20:26:09 UTC
Created attachment 142272 [details] Mockup showing the design idea Hi, The current overview seems to be to much focused showing everything, rather than focusing on the specific workspace the user is working on. I also has the downside that when adding workspaces the windows in the overview become very small. I have made a basic mockup which shows an overview which focuses more on the current workspace and shows the other ones aligned at the bottom. We could also add some button that switches to the grid view and add an mouse over effect (ie. show the selected workspace in the main view) The mockup is attached.
Another idea would be to keep the current view but zoomed way in such that only one workspace is shown completely and just a little bit of the surrounding workspaces are on screen. Then if the mouse moved to one of surrounding workspaces the overview would zoom out a bit to show more workspaces.
(In reply to comment #1) > Another idea would be to keep the current view but zoomed way in such that only > one workspace is shown completely and just a little bit of the surrounding > workspaces are on screen. Then if the mouse moved to one of surrounding > workspaces the overview would zoom out a bit to show more workspaces. Something like Flickr's slideshow?
No, trying flickr's slideshow, that's more like the original mock up. I might try to make a mockup later to show what I mean, but I'll try to explain it a little more in detail now. Say there were 4 workspaces, and the active workspace was the upper left workspace. It would basically be the dash on the left side, and the right side almost entirely showing the active workspace, with the other workspaces also full size and next to the active workspace in the same layout as the current implementation (2x2). Since all workspaces are full size there wouldn't be enough room for most of the non-active workspaces to fit on screen. you'd only see a small part of the workspace to the right of the active workspace and a small part of the workspace below the active workspace. Then if the mouse moved to a screen edge the view would "zoom out". So if you moved to the right screen edge, the active workspace and the workspace to the right of it would be fully visible, each about half size, and the workspaces below would be half size as well, but those two workspaces would be almost entirely off screen. Then if you moved the mouse to the bottom edge, the view would "zoom out" again. Now all 4 workspaces would be about 25% size and on screen. You could potentially make it "zoom in" again if the mouse lingered over a workspace.
Does the grid view make sense at all? Why would you want to have a workspace under an other one rather than side by side? We can just show the current workspace and a way to navigate to left/right. (and show some kind of preview so that the user can move directly to a particular workspace). If we aren't showing all workspaces at once like we do now we don't really need a grid view. And than we should also allow removing any empty workspace, rather than just the last one. Toughs?
Created attachment 144675 [details] [review] Add workspace toggle buttons Add two quickly made icons to toggle between the zoomed workspace view and t They need to be replaced by better looking ones.
Created attachment 144676 [details] [review] [Overview] Add zoomed workspace view Zoom to the current active workspace in the overview to make it easier to select windows. In the zoomed view we don't display the active workspace border (because it would be redundant), we show parts of the surrounding workspaces (based on idea/comment from Ray) and also provide a way to toggle between the grid and the single workspace view. Known bug: After some time the button texture stops to be toggled between zoom/grid ... still have to debug this (help appreciated ;) ), wanted to get this out so people can test/comment on it.
Created attachment 144677 [details] [review] [Overview] Add zoomed workspace view Remove debug log()
Created attachment 144700 [details] [review] [Overview] Add zoomed workspace view Some cleanups.
Created attachment 144701 [details] [review] [Overview] Add zoomed workspace view Fix weird effect when switching to a window while in zoomed mode.
Created attachment 144713 [details] [review] [Overview] Add zoomed workspace view Thanks to the help from Owen, the button issue is fixed now, so this version should work as expected.
(In reply to comment #0) > The current overview seems to be to much focused showing everything, rather > than focusing on the specific workspace the user is working on. The primary purpose of the overview it so show everything. At this point, it doesn't really focus on the current workspace. > I also has the downside that when adding workspaces the windows in the overview > become very small. That is true. While there may be some value in this as a somewhat natural bounding factor - one that may be fairly well matched to the mental model of the system - I agree that it does somewhat diminish the value of workspace use in general and makes it harder to identify windows in even with 2-4 workspaces. It is these two problems that I'd be willing to try to fix. If we are going to allow workspaces we should probably try to make them not suck. Though for many people we could just turn them off and they'd be perfectly happy. > I have made a basic mockup which shows an overview which focuses more on the > current workspace and shows the other ones aligned at the bottom. > > We could also add some button that switches to the grid view and add an mouse > over effect (ie. show the selected workspace in the main view) > > The mockup is attached. Perhaps we can support two different views: grid, linear. The grid view might be what we have now. A linear view could be a (probably horizontal) sliding view with the current workspace filling the view, arrows towards the side if there are more workspaces in that direction, and a indicator that shows the overall position in the sequence. For this indicator I'm not sure if it would be useful to show workspace thumbnails since they'd be so small. Maybe just a series of shapes with the current one being distinguished by brightness, size, or color. The button to switch between grid and linear could appear near that positional indicator to make it easy to go back and forth if necessary. On touchable devices the user could just swipe horizontally to move the view. Keyboard users could probably use the same workspace switch shortcuts they use normally. This might make the workspace/window selector more useful on netbooks as well. Downsides/difficulties to this include: * Degrading the planar design of the overview * Scrolling left would be slightly harder than right due to the position of the launcher pane * How do we slide workspaces under the launcher pane. - Maybe they should scroll vertically then? * We'll have to do something different for the App Icon window list preview - though maybe we should anyway.
(In reply to comment #10) > Created an attachment (id=144713) [details] > [Overview] Add zoomed workspace view > > Thanks to the help from Owen, the button issue is fixed now, so this version > should work as expected. I've tried these patches. Behavior doesn't seem quite right. * It doesn't remember the state of the grid/single button after leaving the overview. Presumably this is a preference type thing. * The way we zoom into and out of the overview seems broken by this patch. * The alignment with the launch pane isn't right * The way you are showing edges/corners of other workspaces in the overview doesn't seem right. The spacing is weird. Also we don't want things to go under the add/view buttons I think. * If I'm in single mode it switches back to grid when I add a workspace. * The app icon over the window doesn't seem to scale properly when in single mode * In single mode I don't have a way to get to my other workspaces. I have to go back to grid. * In the 10 workspace case, if I'm on the center workspace in the block of 9 the single view draws over the launch pane.
(In reply to comment #12) > (In reply to comment #10) > > Created an attachment (id=144713) [details] [details] > > [Overview] Add zoomed workspace view > > > > Thanks to the help from Owen, the button issue is fixed now, so this version > > should work as expected. > > I've tried these patches. Behavior doesn't seem quite right. > > * It doesn't remember the state of the grid/single button after leaving the > overview. Presumably this is a preference type thing. This was by design, can be changed though (maybe makes sense). > * The way we zoom into and out of the overview seems broken by this patch. > * The alignment with the launch pane isn't right > * The way you are showing edges/corners of other workspaces in the overview > doesn't seem right. The spacing is weird. Yeah noticed that to .. that is the side effect of zooming (we don't remove the grid layout, we just zoom in) > Also we don't want things to go under the add/view buttons I think. Err .. what? It shouldn't be doing that, if it does its a bug. > * If I'm in single mode it switches back to grid when I add a workspace. Yeah I wasn't doing this before and it was kind odd, this way the animation is visible and when you add a workspace you would want to switch to it / start apps on it. > * The app icon over the window doesn't seem to scale properly when in single > mode Indeed ... > * In single mode I don't have a way to get to my other workspaces. I have to > go back to grid. Yeah the idea was that switching to windows is a more common case than switching the workspace. > * In the 10 workspace case, if I'm on the center workspace in the block of 9 > the single view draws over the launch pane. I though we only support up to 9 workspaces? As for the draws over the launch pane bug was able to reproduce that (have not seen it because I only use 4 workspaces).
(In reply to comment #11) > (In reply to comment #0) > > The current overview seems to be to much focused showing everything, rather > > than focusing on the specific workspace the user is working on. > > The primary purpose of the overview it so show everything. At this point, it > doesn't really focus on the current workspace. Yeah but this does not really work well when using multiple workspaces. > > I also has the downside that when adding workspaces the windows in the overview > > become very small. > > That is true. While there may be some value in this as a somewhat natural > bounding factor - one that may be fairly well matched to the mental model of > the system - I agree that it does somewhat diminish the value of workspace use > in general and makes it harder to identify windows in even with 2-4 workspaces. > It is these two problems that I'd be willing to try to fix. If we are going > to allow workspaces we should probably try to make them not suck. Though for > many people we could just turn them off and they'd be perfectly happy. Yeah by defaulting to only one workspace (ie. what we do know), those user should be already happy (instead of what we did before i.e 4 workspaces by default). > > I have made a basic mockup which shows an overview which focuses more on the > > current workspace and shows the other ones aligned at the bottom. > > > > We could also add some button that switches to the grid view and add an mouse > > over effect (ie. show the selected workspace in the main view) > > > > The mockup is attached. > > Perhaps we can support two different views: grid, linear. The grid view might > be what we have now. A linear view could be a (probably horizontal) sliding > view with the current workspace filling the view, arrows towards the side if > there are more workspaces in that direction, and a indicator that shows the > overall position in the sequence. For this indicator I'm not sure if it would > be useful to show workspace thumbnails since they'd be so small. Maybe just a > series of shapes with the current one being distinguished by brightness, size, > or color. The button to switch between grid and linear could appear near that > positional indicator to make it easy to go back and forth if necessary. Yeah, see comment 4 a linear view would make, should we allow the user to switch between linear and grid on the fly and just remember the last setting? > On touchable devices the user could just swipe horizontally to move the view. > Keyboard users could probably use the same workspace switch shortcuts they use > normally. Hmm.. maybe click, hold and drag to left/right would work? > This might make the workspace/window selector more useful on netbooks as well. > > Downsides/difficulties to this include: > * Degrading the planar design of the overview > * Scrolling left would be slightly harder than right due to the position of > the launcher pane > * How do we slide workspaces under the launcher pane. - Maybe they should > scroll vertically then? You mean new workspaces? Yeah sliding them in from the right side should work. > * We'll have to do something different for the App Icon window list preview - > though maybe we should anyway. Yeah the alternative would be sliding between the workspaces back and forth which would be weird.
Comment on attachment 144713 [details] [review] [Overview] Add zoomed workspace view See comments from Jon, we might want to implement the linear view instead.
My experience so far is that when I have one workspace open, I use the workspace/window part of the overview sometimes, but when I have two workspaces open, I don't. In fact, I don't even look at, because there's no reason to, because it's just a bunch of indistinguishable off-white squares. So rather than showing *everything* I'm doing, it effectively shows me *nothing*.
I have a small suggestion here. If we are going to have linear view of workspaces also, lets have grid view as a small box (may be beside (+) add workspacebutton ) which shows previews of all existing workspaces and their contents, when user goes to linear view. movement of windows should reflect in this small preview box and workspaces should be switchable using this box. If we follow slideshow approach, user has to press right key for '9' times to reach 10th workspace from first. and even most ppl dont remember how their contents are arranged when they have workspaces more than 3-4. basically, my proposal is when user switches to linear view, the grid view would be zoomed to current workspace and grid view becomes a small widget kind of box, and goes to the bottom, sitting beside (+).
(In reply to comment #14) > > Perhaps we can support two different views: grid, linear. The grid view might > > be what we have now. A linear view could be a (probably horizontal) sliding > > view with the current workspace filling the view, arrows towards the side if > > there are more workspaces in that direction, and a indicator that shows the > > overall position in the sequence. I made up some mockups of a layout like you describe. the first one is very similar to the first mockup at the top of this bug posting. Pic 1) http://farm3.static.flickr.com/2738/4096353130_bbcb179e7a_o.png But I think people should have the option of showing the workspace thumbnails vertically oriented as well as horizontally. Pic 2) http://farm3.static.flickr.com/2747/4096353164_9475e5ebf6_o.png Horizontal seems like it would be more space efficient for showing the main screen, but vertical view would be more intuitive to use the mouse scroll wheel (although horizontal could also use mouse scroll). Maybe you could look to Apple's iPhoto for some visual inspiration, because they use a fullscreen photo-editing tool that looks very similar to what is being described here. In the "Show all Workspaces" view, the main window would fade away, and the thumbnails would use clutter animations to rearrange themselves into a grid like is familiar to users now. Pic 3) http://farm3.static.flickr.com/2768/4095592755_26c8f75f08_o.png However, instaed of using the 1:2:5 3:4:6 7:8:9 grid pattern used now, for the sake of consistency with a linear model they would all be 1:2:3 4:5:6 7:8:9. It seems like that would be cognitively more consistent with the linear flow people would expect from the purely linear view. New workspaces could be added just from the one position of bottom right in grid view, and when need be the spaces would use clutter animations to re-orient themselves. >>For this indicator I'm not sure if it would > > be useful to show workspace thumbnails since they'd be so small. Maybe just a > > series of shapes with the current one being distinguished by brightness, size, > > or color. I personally think it would still be useful to have thumbnails, even if small. Just think about thumbnails of pictures in a photo album folder -- even when they are so small you can't see all the detail, your brain is still capable of distinguishing them most of the time, even if not precisely being able to see all the detail. Although, I think some kind of a live preview pop-up on mouse-over would go a long way to resolving this problem, something along the lines of Apple's Quicklook feature. I am not a developer, but I like your work and enjoy thinking about UI. So I hope these can be helpful in some way. Regards, Brian
*** Bug 592151 has been marked as a duplicate of this bug. ***
Maybe allowing to give names to workspaces and showing that names would help users to navigate around them ? (you can even incorporate that to Alt+F2 and GNOME Do) That feature could be combined with "Desktop contexts" (? couldn't find a wiki page)? P.S. I like 1:2:5 3:4:6 7:8:9 numbering in a grid mode, and 1:2:3 4:5:6 7:8:9 virtual numbering in a linear mode. I like the idea of not reordering workspaces on addition of a new one.
(came across this, please do tell you ordering of the workspaces should be treated in it's own bug) (In reply to comment #20) > Maybe allowing to give names to workspaces and showing that names would help > users to navigate around them ? I second this. I find that I work fastest if I know certain apps are on a certain desktop number. I've set up gnome so that Alt+N brings me to desktop number N. e.g. Alt-3 will always show me my browser window as it's always on desktop number 3. [...] > I like the idea of not reordering workspaces on addition of a new one. I can live with it. But it is a bit odd at first[1]. No major hassle, as I have a small shellscript that launches all the apps I generally need and then shuffles them to their expected desktops with wmctrl, but if I opened my apps by dragging them in the Activities to certain desktops then the current numbering would annoy me. [1] a 'sort numerically' button near the workspaces overview in the Activities would be nice.
(in addition to Comment 21) Forgot to add that all workspaces have names as well so that the 'Move to another workspace' list in a window's menu is nicely populated. Would be nice if the name was shown in the Activities overview.
*** Bug 604504 has been marked as a duplicate of this bug. ***
Bug 604504 which I duplicated on this bug has a mostly complete implementation of the current design from the mockups in this area. Testing/commentary by those interested in the subject would be appreciated.
Hi Maxim, First thanks for the great work. I have not looked at the code yet (just noticed a white space issue when applying it). It works quite well, I noticed two minor issues though: 1) When in linear mode we probably shouldn't show the "scrollbar" when only one workspace is active 2) When adding a new workspace it should scroll to it, (the user might want to use it right now that is why he/she added it in the first place).
Created attachment 149740 [details] [review] Corrected patch Difference with previous patch: 1. Fixed style problem. 2. scrollbar hide when only one workspace is active 3. When adding a new workspace, scroll to it
So trying out the latest patch. Looking pretty good! Some comments: * We shouldn't show the grid and single workspace buttons when there is only one workspace. Firstly since they don't have any noticeable effect and also because someone who doesn't use multiple workspaces shouldn't have that cluttering the screen. * We shouldn't show the remove workspace "-" button for the first workspace. It seems to crash if it is pressed * The artwork doesn't very closely match the mockups. Shouldn't that hard to pull svgs out of the mockups if that is what we need. Let me know. * When switching between single and multi we animate the placement of the windows again. We shouldn't do this. We should scale the workspace as it is already laid out. * Even in grid view we want to have the remove workspace button down in the controls area - not in the middle of the workspace like we have it today. * I don't think we want to support adding more than one empty workspace - at least not by default. This includes not adding a second before anything is running on the first. * I think we should smoothly scroll between workspaces instead of jumping between them. This will really help make it seem more physical and natural. We really want to avoid any jarring effects anywhere in the Shell. * We seem to be missing the positional indicators that are in the mockup underneath the slider. I think they still offer value and we should try to add them. Anyway - thanks for the good work. Will be fun to watch it progress.
Latest mockups: http://www.gnome.org/~mccann/shell/mockups/20091114/shell-mockup-overview.png http://www.gnome.org/~mccann/shell/mockups/20091114/shell-mockup-20091114.svg
Created attachment 149816 [details] [review] Corrected patch Difference with previous patch: 1. Show only '+' button when there is only one workspace 2. Used svgs from mockups 3. When switching between single and multi doesn't animate the placement 4. Added animation for scroll between workspaces 5. Added positional indicators > * I don't think we want to support adding more than one empty workspace - at > least not by default. This includes not adding a second before anything is > running on the first. Is it include cases when empty workspace not the last?
Great work! Some minor observations though: * the positional indicators "jump" when switching workspace - possibly additional spacing is not the right way to mark the active workspace indicator, IMO it looks way too busy * when switching workspaces in single view, title captions and close buttons are not placed correctly * when removing a workspace in single view, the window placement is still animated * the animation is way too slow What exactly is the design idea behind the positional indicators? From the mockups, I always imagined them to also function as traditional workspace switcher - is that desired behaviour?
Created attachment 149880 [details] [review] Corrected patch Difference with previous patch: 1. reduced animation's time(from 2sec to 0.5) 2. removed animation, when removing a workspace in single view 3. Fixed problem with position of title captions and close buttons 4. Workspace indicator is clickable.
Created attachment 150257 [details] [review] Updated patch Difference with previous patch: 1. Merge with current HEAD 2. Restore key focus, after switching workspacesView.
(In reply to comment #32) > Created an attachment (id=150257) [details] [review] > Updated patch > > Difference with previous patch: > > 1. Merge with current HEAD > 2. Restore key focus, after switching workspacesView. The workspace indicators seem to move around when switching between workspaces, also I doubt that it makes much sense to show thumbnails there, the design in the mockups is better suited imho.
Cool! A few comments: * In the single workspace view we should not show the "current workspace" white outline highlight thingy * The "add workspace" button doesn't look like the mockup. It seems stretched vertically. * The grid and single workspace buttons and spacing don't look like the mockup * I'm not sure we want to show the workspaces in miniature under the scrollbar. It isn't a bad idea but it wasn't what I was thinking of with the mockup. * We need to reserve space at the buttom of the screen for the message tray. * I was hoping that the scrollbar could smoothly slide between workspaces as if it was controlling a plane. So, a linear 1-1 scroll. * I think we need a drag affordance on the scrollbar - see the mockup. * The scrollbar and buttons are too tall. * The workspace size is changed for some reason. It no longer lines up with the top of the Applications label box on the left.
Review of attachment 150257 [details] [review]: This is a partial review - it's a huge patch, and in particular the changes to workspaces.js are quite intricate. But let's do this iteratively, I think it would be better to land an intermediate step in this patch and then work through some of the details to workspaces.js as we go. Thanks for the awesome work here in general! ::: js/ui/overview.js @@ +90,3 @@ this._group._delegate = this; + this._workspacesViewSwitch.connect('view-changed', Lang.bind(this, function () { + this._workspacesViewSwitch = new Workspaces.WorkspacesViewSwitch(); Let's move this callback into its own named function; I think anonymous callbacks should be limited to 4 lines at most typically. @@ +99,3 @@ + if (!this.visible) + this._workspacesViewSwitch.connect('view-changed', Lang.bind(this, function () { + this._workspacesViewSwitch = new Workspaces.WorkspacesViewSwitch(); Why do we hide the group? @@ +102,3 @@ + if (!this.visible) + this._workspacesViewSwitch.connect('view-changed', Lang.bind(this, function () { + this._workspacesViewSwitch = new Workspaces.WorkspacesViewSwitch(); Typo; should be "getCurrentWorkspaceView"? @@ +108,3 @@ + if (!this.visible) + this._workspacesViewSwitch.connect('view-changed', Lang.bind(this, function () { + this._workspacesViewSwitch = new Workspaces.WorkspacesViewSwitch(); I'd personally prefer calling it "ControlsBar" rather than "SwitchBar", if you agree? @@ +109,3 @@ + + let switchBar = this._workspacesViewSwitch.getSwitchBar(); + this._workspacesBar = new St.BoxLayout({ 'pack-start': true}); Missing trailing space (there's other scattered inconsistent indentation too). @@ +110,3 @@ + let switchBar = this._workspacesViewSwitch.getSwitchBar(); + this._workspacesBar = new St.BoxLayout({ 'pack-start': true}); + this._workspacesBar.set_size(this._workspacesBarWidth, this._workspacesBarHeight); Should be CSS. @@ +120,3 @@ + + this._dash.show(); + this.emit('showing'); And why show the dash? Did we hide it? Also why do we re-emit 'showing' here? @@ +224,3 @@ // place the 'Add Workspace' button in the bottom row of the grid + this._workspacesBarWidth = primary.width - this._workspacesBarX - 2 * WORKSPACE_GRID_PADDING; + this._workspacesBarX = this._workspacesX; I think it'd be cleaner if the scrollbar was just packed into a box with the controls, and was set to expand and fill, rather than being a calculated width. Like: [ Control ] [ Control ] [ Padding ] [ ...Scrollbar...] [ Padding ] [ Control [ @@ +225,3 @@ + this._workspacesBarHeight = displayGridRowHeight - 10; + this._workspacesBarWidth = primary.width - this._workspacesBarX - 2 * WORKSPACE_GRID_PADDING; + this._workspacesBarX = this._workspacesX; This is too magic; I think it'd be better to either pick a fixed height for the scrollbar from CSS, or to have it be the same size as the controls. You'd get the latter behavior for free by packing them all into a St.BoxLayout. @@ +335,3 @@ + this._workspacesX, this._workspacesY, true); + this._workspaces = this._workspacesViewSwitch.getCurrentWorspaceView(this._workspacesWidth, this._workspacesHeight, Same typo here, "Workspace" not "Worspace" @@ +354,3 @@ + this._workspacesBar.move_by(this._workspacesBarX, this._workspacesBarY); + this._workspacesBar = new St.BoxLayout({ 'pack-start': true}); + // Create WorkspacesViewSwitch bar This is all duplicating the code in the 'view-changed' callback above; can we have a common function for creating this layout? @@ -478,3 @@ - this._init(buttonSize, buttonX, buttonY, acceptDropCallback); -function AddWorkspaceButton(buttonSize, buttonX, buttonY, acceptDropCallback) { - Why the Group actor? Can't you just make the texture actor reactive? @@ -485,3 @@ - this._init(buttonSize, buttonX, buttonY, acceptDropCallback); -function AddWorkspaceButton(buttonSize, buttonX, buttonY, acceptDropCallback) { - You can set the width and height from CSS too. @@ -486,3 @@ - this._init(buttonSize, buttonX, buttonY, acceptDropCallback); -function AddWorkspaceButton(buttonSize, buttonX, buttonY, acceptDropCallback) { - This should use ShellTextureCache. ::: js/ui/workspaces.js @@ +29,3 @@ const ZOOM_OVERLAY_FADE_TIME = 0.15; +const ANIMATION_TIME = 0.5; I'd prefer calling this something different; it's too similar to Overview.ANIMATION_TIME which we use for everything. Probably WORKSPACE_SWITCH_TIME or something? Can you explain a bit why you chose 0.5 over 0.25? @@ +33,2 @@ +const SPACE_BETWEEN_INDICATOR = 5; +const WORKSPACE_INDICATOR_SIZE = 50; These two should be CSS. @@ +347,3 @@ + parentActor.connect('notify::visible', + Lang.bind(this, this._positionItems)); + windowClone.actor.connect('notify::allocation', Feels a bit hacky. First we probably only want to react if (this._parentActor.visible). But _positionItems actually needs to be called at the *end* of the animation, not when the overview is first shown. @@ +405,2 @@ }, + _positionItems: function(win) { Hmm, isn't this doing the same thing as updatePositions? Why do we have both? @@ +983,3 @@ */ * @workspaceZooming: If true, then the workspace is moving at the same time and we need to take that into account. + positionWindows : function(workspaceZooming, animate) { You didn't document animate. And rather than having multiple boolean arguments, I think a cleaner approach is to use flags, like: const WindowPositionFlags = { ZOOM: 1 << 0, ANIMATE: 1 << 1 }; positionWindows(WindowPositionFlags.ZOOM | WindowPositionFlags.ANIMATE) @@ +989,3 @@ let visibleWindows = this._getVisibleWindows(); + animate = true; + if (animate === undefined) Definitely prefer flags rather than this. @@ +1005,3 @@ overlay.hide(); + let t; This looks unused? @@ +1766,1 @@ + let bin = new St.Bin({}); I'd just remove the {} here. @@ +1781,2 @@ }, + _correctRemoveButtonVisibility: function() { Let's call this "update" rather than "correct"; the latter term makes me feel like it's a workaround for a bug. @@ +1782,3 @@ + return; + if (this._removeButton === undefined) + _correctRemoveButtonVisibility: function() { When would it be undefined? @@ +1814,3 @@ +} + this._init(width, height, x, y, animate); +function SingleView(width, height, x, y, animate) { Hmm, perhaps "this.actor.clip_to_allocation = true" ? @@ +1819,3 @@ +} + this._init(width, height, x, y, animate); +function SingleView(width, height, x, y, animate) { Calling .hide() feels cleaner to me. @@ +1852,3 @@ + return; + + if (this._scroll !== undefined) { I prefer null rather than undefined; there are other places like this too. @@ +1882,3 @@ + this._workspaces[w].actor.visible = false; + } + this._workspaces[w].positionWindows(undefined, false); Definitely don't like explicitly passing undefined, flags would be better. @@ +1969,3 @@ + })); + })); + this._scroll.adjustment = adj; I'm confused; why this dance connecting to notify::adjustment and then setting the adjustment? Can't you just pass "adjustment: adj" above? @@ +2001,3 @@ + + _addIndicatorClone: function(i, x, active) { + let clone = new Clutter.Clone({source: this._workspaces[i].actor, reactive: true}); As mentioned in the bug, let's just use grey rectangles as in the design. @@ +2195,3 @@ + + this._gconf.set_int(this.VIEW_KEY, this.SINGLE_VIEW_KEY_VALUE); + this.emit('view-changed'); This callback is almost identical to the other one; I think it'd be cleaner if the 'clicked' handlers just set the GConf key, and then you watch the gconf key changes and update the actor states. @@ +2199,3 @@ + actor.add(this._singleView, {'y-fill' : false, 'y-align' : St.Align.START}); + + if (this._gconf.get_int(this.VIEW_KEY) == 1) Should use MOSAIC_VIEW_KEY_VALUE instead of 1. @@ +2209,3 @@ + + actor.connect('destroy', Lang.bind(this, function() { + this._switchBar = undefined; I prefer "null" rather than "undefined". @@ +2224,3 @@ + this._switchBar.set_opacity(0); + else + this._switchBar.set_opacity(255); Why not hide/show instead of opacity?
Created attachment 151413 [details] [review] Corrected patch > I was hoping that the scrollbar could smoothly slide between workspaces as > if it was controlling a plane. So, a linear 1-1 scroll. Is it mean that we can have noninteger scroll position(should show parts of two workspaces)? > Hmm, perhaps "this.actor.clip_to_allocation = true" ? actor have full screen allocation (need for some animation effects). > I'm confused; why this dance connecting to notify::adjustment and then setting the adjustment? > Can't you just pass "adjustment: adj" above? adjustment can be changed.
(In reply to comment #36) > Created an attachment (id=151413) [details] [review] > Corrected patch > > > I was hoping that the scrollbar could smoothly slide between workspaces as > > if it was controlling a plane. So, a linear 1-1 scroll. > > Is it mean that we can have noninteger scroll position(should show parts of two > workspaces)? Yes. It is getting there. A other comments: * The graphics look a bit different from the mockups still. Particularly the scrollbar. * I don't like the way the boxes under the scrollbar move around when scrolling. I think they should stay put. * I don't think it makes sense to be able to add more than one empty workspace but I suppose this should be handled in a separate bug? * Need more space between the buttons on both the left and right side of the scrollbar.
... and some more comments: * the transition between single and mosaic views appears very abrupt - I think we'd eventually want some kind of animation there * probably not within the scope of this bug: in single view, the window selection (right click on app well icon) does not work well if all the app's windows are on different workspaces than the currently visible - McCann: I think that'll need some design ... * drag and drop to / between the workspaces is severely limited - the drop area is either extremely limited, or simply does not exi But hey, exciting work - can't wait to see it getting in!
(In reply to comment #37) > > > I was hoping that the scrollbar could smoothly slide between workspaces as > > > if it was controlling a plane. So, a linear 1-1 scroll. > * The graphics look a bit different from the mockups still. Particularly the > scrollbar. > * I don't like the way the boxes under the scrollbar move around when > scrolling. I think they should stay put. > * I don't think it makes sense to be able to add more than one empty workspace > but I suppose this should be handled in a separate bug? > * Need more space between the buttons on both the left and right side of the > scrollbar. If there are boxes under the desktop area showing which workspace is displayed, why even keep using a scrollbar at all? Why not just make the boxes into a clickable/draggable replacement for the traditional scrollbar? As far as I can tell, they both serve the same purpose of indicating spatial context, and either can serve as a clickable or draggable means of switching between workspaces. I think getting rid of the scrollbar entirely is especially attractive if one considers the touch-interface tablet PC form factor on which the Gnome-shell would really shine. Who would bother to drag their fingers along a scrollbar when they can just poke one of the boxes to switch workspace context? The scrollbar takes up space and feels to me like a relic of the traditional purely mouse-dominated desktop.
Created attachment 151514 [details] [review] Corrected patch
(In reply to comment #39) > (In reply to comment #37) > > > > I was hoping that the scrollbar could smoothly slide between workspaces as > > > > if it was controlling a plane. So, a linear 1-1 scroll. > > > * The graphics look a bit different from the mockups still. Particularly the > > scrollbar. > > * I don't like the way the boxes under the scrollbar move around when > > scrolling. I think they should stay put. > > * I don't think it makes sense to be able to add more than one empty workspace > > but I suppose this should be handled in a separate bug? > > * Need more space between the buttons on both the left and right side of the > > scrollbar. > > If there are boxes under the desktop area showing which workspace is displayed, > why even keep using a scrollbar at all? Why not just make the boxes into a > clickable/draggable replacement for the traditional scrollbar? As far as I can > tell, they both serve the same purpose of indicating spatial context, and > either can serve as a clickable or draggable means of switching between > workspaces. I think getting rid of the scrollbar entirely is especially > attractive if one considers the touch-interface tablet PC form factor on which > the Gnome-shell would really shine. Who would bother to drag their fingers > along a scrollbar when they can just poke one of the boxes to switch workspace > context? The scrollbar takes up space and feels to me like a relic of the > traditional purely mouse-dominated desktop. Yeah that makes sense to me. Jon?
Created attachment 151795 [details] [review] Updated patch disable the AddWorkspaceButton if there are >= 16 workspaces. > If there are boxes under the desktop area showing which workspace is displayed, > why even keep using a scrollbar at all? Why not just make the boxes into a > clickable/draggable replacement for the traditional scrollbar? As far as I can > tell, they both serve the same purpose of indicating spatial context, and > either can serve as a clickable or draggable means of switching between > workspaces. I think getting rid of the scrollbar entirely is especially > attractive if one considers the touch-interface tablet PC form factor on which > the Gnome-shell would really shine. Who would bother to drag their fingers > along a scrollbar when they can just poke one of the boxes to switch workspace > context? The scrollbar takes up space and feels to me like a relic of the > traditional purely mouse-dominated desktop. Easily can be done by changing gnome-shell.css (set zero height to SwitchScroll).
Created attachment 151947 [details] [review] Different Workspaces View Added new workspaces view and ability to switch between views.
(In reply to comment #43) > Created an attachment (id=151947) [details] [review] > Different Workspaces View (Since Colin didn't describe what he attached, I asked him, and it's just a rebase of the last patch to take care of a few conflicts with the current code.)
Review of attachment 151947 [details] [review]: Some more comments; I took the liberty of fixing some of these. Will attach a new patch in a minute. ::: js/ui/overview.js @@ +231,2 @@ // place the 'Add Workspace' button in the bottom row of the grid this._dash.searchResults.actor.height = this._workspacesHeight; This comment no longer applies. ::: js/ui/workspaces.js @@ +383,3 @@ + let title = this.title; + let button = this.closeButton; + _positionItems: function(win) { I deleted this, and instead changed it so when we show the close button, we call the other function updatePositions. @@ +1262,3 @@ this.actor = null; this.actor.destroy(); + this._parentActor.remove_actor(this._windowOverlaysGroup); I changed this to: this._windowOverlaysGroup.destroy() @@ +1792,3 @@ + if (this._removeButton === null) + //_removeButton may yet not exist. + _updateButtonsVisibility: function() { I changed this (and others) from '===' to '=='. The first feels like overkill here, no one's going to redefine null. @@ +2067,3 @@ + actor.destroy(); + return; + } This is pretty ugly =/ I understand the intent here though. To do this a little more scalably, we could use a Shell.GenericContainer to allocate the rectangles at a size where they would fit. @@ +2224,3 @@ + this._currentView = view; + this._gconf.set_int(this.VIEW_KEY, view); + this.emit('view-changed'); I fixed this function's double-intentation.
Created attachment 151968 [details] [review] New workspaces view This patch fixes up the things marked as such in this review: https://bugzilla.gnome.org/review?bug=593844&attachment=151947
Review of attachment 151968 [details] [review]: Maxim, Overall: nice job - you even fixed the dnd issues! The patch is rather big, so the following only reviews part of it - apart from some minor comments on Makefile and CSS it's basically a WorkspacesViewSwitch review ... Another important thing to note before digging into details: I haven't done much reviewing before, so you should await the review's review (preferably from Colin) before putting any work into this ;) ::: data/Makefile.am @@ +38,3 @@ + theme/mosaic-view-active.svg \ + theme/remove-workspace.svg \ + theme/section-more.svg \ It is good style to sort this list alphabetically; to avoid spreading out related items all over the list, I'd propose a naming scheme along the lines of: workspace-add.svg workspace-remove.svg workspace-view-single.svg workspace-view-single-active.svg ... ::: data/theme/gnome-shell.css @@ +200,3 @@ + background-image: url("single-view-active.svg"); + width: 24px; + height: 15px; It should not be necessary to repeat width/height in pseudo-classes. @@ +224,3 @@ + +#SwitchScroll StButton#hhandle { + border-image: url("switch-scroll-hhandle.svg") 5; 1. I love your scrollbars 2. I hate the inconsistency they introduce So my suggestion would be to rather update the generic sections above (rename switch-scroll-hhandle.svg to scroll-hhandle.svg and create a vertical version as well!). This should probably be split out in a separate (mini-)patch. @@ +233,3 @@ + width: 0px; + background-color: rgba(0, 0, 0, 0); + border: 0px solid rgba(128,128,128,0); This looks overly chatty - I can replace all those properties with just "border: 0;" and get the same result. Maybe a comment should be added that it's "display: none;" what we really want. ::: js/ui/workspaces.js @@ +1371,2 @@ Signals.addSignalMethods(Workspace.prototype); +function GenericWorkspacesView(width, height, x, y, animate) { We might want to make a cut here, rename workspaces.js to workspace.js and move everything below in a new file workspaces-view.js. This is getting huge ... @@ +1594,3 @@ +} + +function Workspaces(width, height, x, y, animate) { I suggest renaming this to MosaicView - that would make the relationship to GenericWorkspacesView and SingleView much more obvious. @@ +2178,3 @@ return (screen - curOverviewPos) / curOverviewScale - params.workspacePos; +function WorkspacesViewSwitch() { + Not sure about this, but it might make sense to call this WorkspacesControls and let it manage: - the view mode (that's what it currently does) - the view's controls (e.g. scrollbar, indicators) - the add/remove buttons (Read: everything below the workspacesView) @@ +2187,3 @@ + this._init(); +function WorkspacesViewSwitch() { + Constants usually go to the top of the file (and if there is agreement on a split, that won't be >2000 LOCs away ...) Grouping can be done with pseudo-enum objects, e.g. const WorkspacesViewType = { SINGLE: 0, MOSAIC: 1 }; @@ +2190,3 @@ + this._init(); +function WorkspacesViewSwitch() { + You might consider monitoring gconf for changes. @@ +2193,3 @@ + this._init(); +function WorkspacesViewSwitch() { + These are not named clearly; I'd assume that _mosaicView is an instance of Workspaces, _singleView one of SingleView, and _currentView a reference to one of _mosaicView or _singleView. Better in my opinion: _switchMosaicViewButton _switchSingleViewButton _currentViewType @@ +2208,3 @@ + }, + + getCurrentWorkspaceView: function(width, height, x, y, animate) { I prefer something like createCurrentWorkspaceView(), because the funtion always returns a fresh instance. The same applies to the various getControllerBar() functions above. @@ +2215,3 @@ + return new Workspaces(width, height, x, y, animate); + + throw new Error("Incorrect workspaceView"); This looks a little dangerous - a user who is made to set this to an invalid value is left without overview. Of course you can catch this in overview.js (hint: you don't!), but I'd suggest something like: switch (this._currentView) { case WorkspaceViewType.SINGLE: return new SingleView(...); case WorkspaceViewType.MOSAIC: default: return new MosaicView(...); } I'm really indifferent about which view should be the default, as long as we do pick one ... @@ +2218,3 @@ + }, + + getControlsBar: function() { Again: this looks like an accessor to some object property. createControlsBar would be nicer, although my favorite would be to make this private, calling it from the constructor. Access from the outside via the actor property (which should equal either this._controlsBar or overview._workspacesBar, see comment above on WorkspacesViewSwitch).
Created attachment 151985 [details] [review] Corrected patch > I haven't done much reviewing before I like this review. > It should not be necessary to repeat width/height in pseudo-classes. It is not pseudo-class. > So my suggestion would be to rather update the generic sections above (rename > switch-scroll-hhandle.svg to scroll-hhandle.svg and create a vertical version > as well!). It should be a separate patch. It will affect other places(that not related with this patch) > Not sure about this, but it might make sense to call this WorkspacesControls and let it manage: It is not a good idea. RemoveButton have different meaning in different views. ScrollBar is widely integrated in MosaicView.
(In reply to comment #48) > Created an attachment (id=151985) [details] [review] > Corrected patch It would be better to give patches descriptive titles - pretty useful for instances when searching bz for "gnome-shell bugs with patches". You might want to have a look at git-bz[1], which does this pretty much automagically, and which is almost as addictive as git itself ;) Regarding your comments: > It is not pseudo-class. Mmmmh - I was referring to .switch-view-single:checked and .switch-view-mosaic:checked - :checked is a pseudo class which is applied in addition to the "normal" class, so the width and height properties are redundant. There's another similar case here: If you change actor.style_class = "workspace-indicator-active"; to actor.style_class = "workspace-indicator active"; (note the space) you may replace .workspace-indicator-active { [...] }; with .workspace-indicator.active { background-rgba(255,255,255,0.8); } > > So my suggestion would be to rather update the generic sections above > > It should be a separate patch. It will affect other places(that not related > with this patch) Actually that was the idea - creating a small patch apart which updates the look of the scroll bars. It is of course pretty irrelevant if it's created before or after this patch lands ... > > Not sure about this, but it might make sense to call this WorkspacesControls and let it manage: > > It is not a good idea. > RemoveButton have different meaning in different views. > ScrollBar is widely integrated in MosaicView. If it is a bad idea, it's not for the reasons you mention - sorry if I was unclear there. The suggestion was not to move code out of SingleView/MosaicView (getControllerBar basically), but to move the code from overview.createControlsBar() into WorkspacesSwitch/WorkspacesControls, so that the code in overview.js becomes something along the lines of: this._workspacesControl = new WorkspacesControl(); this._workspacesControl.connect('view-changed', Lang.bind(this, this._onViewChanged)); this._group.add_actor(this._workspacesControl.actor); this._group.add_actor(this._workspacesControl.createWorkspaceView()); The idea might still be rubbish though ... [1] git://git.fishsoup.net/git-bz
Created attachment 151991 [details] [review] New workspaces view Fixes two trivial issues with the previous patch: - correct include of workspace.js in appDisplay.js - fix duplicate constant in switch statement in createCurrentWorkspaceView()
Comment on attachment 151991 [details] [review] New workspaces view Ok, let's go ahead with this. There's still some things I think that could be cleaned up, but it's been outstanding a long time and it'd be better to get this in than have it stay out too long =)
Actually before I commit, is it OK if I change all of the "!==" to just "!=" ?
(In reply to comment #52) > Actually before I commit, is it OK if I change all of the "!==" to just > "!=" ? yes.
Attachment 151991 [details] pushed as 20abc4c - New workspaces view
"Does the grid view make sense at all? Why would you want to have a workspace under an other one rather than side by side?" The grid view is very very popular, and I think it's worth examining why. My use of a grid is to organize windows in 2 dimensions. The vertical dimension is used to seperate closely related windows, especially windows within the same task. The horizontal dimension is used to seperate out different tasks. I'd hate to see that kind of task organization functionality dissapear without a big innovation in task management to replace it... Thoughts?