GNOME Bugzilla – Bug 610189
Move workspace controls into a single object
Last modified: 2010-03-16 17:41:16 UTC
Currently the code is split out between Overview, WorkspaceViewSwitch and the workspace views (the latter being unavoidable as we have view specific controls). The first patch renames WorkspacesViewSwitch to WorkspacesControls and let it hold the whole bottom bar. It's part of some code reorganization which aims to animate view switches. The second patch is a simple CSS adjustment to better match the mockups, which was hard to do before.
Created attachment 153959 [details] [review] Move workspace controls into a single object Rename WorkspacesViewSwitch to WorkspacesControls and let it manage all workspace controls. Do not destroy and recreate the controls bar actor on each view change, but add it to the overview once and let it update itself.
Created attachment 153960 [details] [review] Add additional padding to the view specific workspace controls In the latest mockup, there is more spacing around the scroll bar than between the other controls.
Created attachment 154090 [details] [review] Move workspace controls into a single object Rebased on master.
Comment on attachment 153960 [details] [review] Add additional padding to the view specific workspace controls >- let actor = new St.BoxLayout({ 'pack-start': true, vertical: true }); >+ let actor = new St.BoxLayout({ style_class: 'single-view-controls', >+ pack_start: true, vertical: true }); if it's multi-line, make it consistently multi-line. (so put pack_start and vertical on separate lines).
Created attachment 154416 [details] [review] Add additional padding to the view specific workspace controls Updated according to Dan's comment.
Comment on attachment 154416 [details] [review] Add additional padding to the view specific workspace controls Attachment 154416 [details] pushed as b6bb26e - Add additional padding to the view specific workspace controls
Created attachment 154418 [details] [review] Move workspace controls into a single object Rebased patch.
Created attachment 154451 [details] [review] Move workspace controls into a single object Rebased patch.
Comment on attachment 154451 [details] [review] Move workspace controls into a single object >+ this._workspaces = this._workspacesControls.createCurrentWorkspaceView(this._workspacesWidth, this._workspacesHeight, > this._workspacesX, this._workspacesY, false); fix the indentation of the second line. (yes, I know it was wrong before) >+ this._workspaces = this._workspacesControls.createCurrentWorkspaceView(this._workspacesWidth, this._workspacesHeight, > this._workspacesX, this._workspacesY, true); likewise. Though it would be better to only have that code in one place. >- // Make Dash fade in so that it doesn't appear to big. >+ // Make Dash fade in so that it doesn't appear too big. if you wanted, you could split this out, along with fixing the two comments in _onViewChanged that are missing spaces after the "//" (and one of which also has a typo), and just commit it. >+ canAddWorkspace: function() { >+ throw new Error("Not implemented"); >+ }, there's no need to have this be virtual, and then have each of the two subclasses implement it identically. just put the right implementation in the base class. (I'm not convinced that the [pre-existing] difference in canRemoveWorkspace implementations makes any sense either; why shouldn't you be able to delete the leftmost workspace in the linear view, as long as there are still other workspaces?) >@@ -419,45 +418,28 @@ MosaicView.prototype = { > }, > > createControllerBar: function() { >+ return new St.BoxLayout(); >+ }, seems pointless. it should just return null and WorkspacesControl._createControlsBar should DTRT. >+ removeWorkspace: function() { >+ if (this._workspaces.length <= 1) >+ return; you should't need to test that, since the button wouldn't be enabled if it's false. >+ _createControlsBar: function() { should probably be _updateControlsBar >+ // View switcher buttons >+ let button_class = 'workspace-controls switch-mosaic'; >+ this._mosaicViewButton = new St.Button({ toggle_mode: true, >+ style_class: button_class }); i don't think this is really any better than just putting 'workspace-controls switch-mosaic' inline in the constructor call like it was before >+ this.actor.add(this._singleViewButton, {y_fill: false, >+ y_align: St.Align.START}); fix spacing around {}s > global.screen.connect('notify::n-workspaces', >- Lang.bind(this, this._workspacesChanged)); >+ Lang.bind(this, this._workspacesChanged)); the indentation was correct before
Created attachment 155161 [details] [review] Move workspace controls into a single object Anything not mentioned above should be fixed by the update: (In reply to comment #9) > (From update of attachment 154451 [details] [review]) > >- // Make Dash fade in so that it doesn't appear to big. > >+ // Make Dash fade in so that it doesn't appear too big. > > if you wanted, you could split this out [...] and just commit it. Mmmh, I generally prefer fixing typos along the way instead of spamming the history with whitespace-only commits ... > (I'm not convinced that the [pre-existing] difference in canRemoveWorkspace > implementations makes any sense either; why shouldn't you be able to delete the > leftmost workspace in the linear view, as long as there are still other > workspaces?) Good question - I tried not to change any behavior with the patch (except for the small change of moving the controls bar destruction to a later point so that zoomToOverview() matches zoomFromOverview()) - not sure if I'm comfortable changing that without Jon's consent ...
(In reply to comment #9) > likewise. Though it would be better to only have that code in one place. I forgot to mention that this comment has not been addressed either, as the plan is to move the code in question out of the overview anyway (see the switch-animation patches).
Comment on attachment 155161 [details] [review] Move workspace controls into a single object sorry about the delay. looks good
Attachment 155161 [details] pushed as c02b57e - Move workspace controls into a single object