GNOME Bugzilla – Bug 690171
Overview: Make Workspace manage its own padding
Last modified: 2013-01-18 20:04:59 UTC
I know there is another bug for this somewhere, but I can't find it...
Created attachment 231499 [details] [review] Overview: Make Workspace manage its own padding Move window clone padding down to LayoutStrategy, and apply it on both sides of the computed layout. Move spacing between the dash and the content down in the view selector page, so it can be removed for the workspaces page. This fixes spacing in multimonitor cases (as there the workspace gets the full monitor as geometry), and fixes spacing between the clones and the thumbnails box, without introducing an asymmetry in the layout code to account for the overview-group spacing.
Review of attachment 231499 [details] [review]: Generally looks like a nice cleanup, but the unrelated parts need to be split out. ::: js/ui/viewSelector.js @@ -41,3 @@ _init : function(searchEntry, showAppsButton) { - this.actor = new St.BoxLayout({ name: 'viewSelector', - vertical: true }); Yeah, this is a left-over from the tabbed overview. Should be split out. @@ +171,3 @@ }, + _addPage: function(actor, name, a11yIcon, params) { Using Params for optional arguments makes sense, but should be a separate patch as well.
Created attachment 231518 [details] [review] ViewSelector: use Params to pass optional parameters to addPage This allows to introduce new parameters without making the signature overly complex.
Created attachment 231519 [details] [review] ViewSelector: remove unnecessary StBoxLayout It was a remnant from the tabbed interface.
Created attachment 231520 [details] [review] Overview: Make Workspace manage its own padding Move window clone padding down to LayoutStrategy, and apply it on both sides of the computed layout. Move spacing between the dash and the content down in the view selector page, so it can be removed for the workspaces page. This fixes spacing in multimonitor cases (as there the workspace gets the full monitor as geometry), and fixes spacing between the clones and the thumbnails box, without introducing an asymmetry in the layout code to account for the overview-group spacing.
Created attachment 231521 [details] [review] ViewSelector: use Params to pass optional parameters to addPage This allows to introduce new parameters without making the signature overly complex.
I think we really need either this or the patch in bug 690174 to fix the missing spacing regression between the workspaces view and the selector controls. While some patches in here would be nice to have regardless, if I understood correctly what attachment 231520 [details] [review] does (adding the spacing also on the external sides of the workspaces view), I think I would prefer fixing the bug with my patch in bug 690174: spacing is always intended in between children, not on the external sides of the container. Also, my patch can just be reverted once we further refactor how layouting works in the overview (which I do in a later patch related to the search results feature I'm working on).
Review of attachment 231518 [details] [review]: ::: js/ui/viewSelector.js @@ +178,3 @@ }, _addPage: function(actor, a11yFocus, name, a11yIcon) { Shouldn't you aldo change the signature to _addPage: function(actor, name, a11yIcon, params)?
Review of attachment 231519 [details] [review]: LGTM
Comment on attachment 231518 [details] [review] ViewSelector: use Params to pass optional parameters to addPage Ah sorry this is obsoleted by the later one
Review of attachment 231521 [details] [review]: LGTM
(In reply to comment #7) > I think we really need either this or the patch in bug 690174 to fix the > missing spacing regression between the workspaces view and the selector > controls. > > While some patches in here would be nice to have regardless, if I understood > correctly what attachment 231520 [details] [review] does (adding the spacing also on the external > sides of the workspaces view), I think I would prefer fixing the bug with my > patch in bug 690174: spacing is always intended in between children, not on the > external sides of the container. Also, my patch can just be reverted once we > further refactor how layouting works in the overview (which I do in a later > patch related to the search results feature I'm working on). It is actually intended to add the spacing on the external sides of the workspace view. Otherwise we need a patch to fix the geometry of workspaces on external monitors, since currently windows are allocated the full monitor size, with no padding. And to me, it looks cleaner to pass the full allocation to the layout code, and then decide there how to pad the content.
What I'm trying to say is that - It's wrong to use "spacing" when you mean "padding" (i.e. the external one) - I don't think it's a good idea to add padding-left to other overview pages. It makes things harder for RTL and we should just use the spacing in the container. Since the controls are possibly going to be separated from the WorkspaceDisplay actors and added directly to the overview horizontal group in the future, using spacing makes everything much easier. - The external monitor issue seems different to me. Since on an external monitor we don't have any other UI elements, we might even use a different padding value altogether than in the overview. I think we should have a different style class for a workspace view on an external monitor.
I agree with your points in principle, but your analisys of point 3 is wrong. Unless we change the delicate geometry handling code in workspacesView.js, workspace.js needs to take care of adding padding itself, and the best place to do that is LayoutStrategy. Now, I'm perfectly fine with adding another parameter (hpadding?), similar to bottomPadding, instead of reusing columnSpacing. But using spacing from the outside container looks just wrong to me.
I see your point, and I think supporting a padding around the workspace view actor is a good idea for future tweaks (and I think it should be a separate property, yeah). On the other hand, right now WorkspacesDisplay is a container for two actors, the workspace views and the controls. I don't see any reason why we couldn't support spacing in such a container. After all, the patch in bug 690174 just adds back some code that was accidentally taken out in http://git.gnome.org/browse/gnome-shell/commit/?id=04d68c6e36dbd8b6d8933a5edea77d9ca15846e7
Ok, so let's do both. The patch in bug 690174 is good to go, I'll update mine to account for separate padding properties (which would be 0 in the primary monitor, and non 0 outside) Meanwhile, let me push the clean ups too.
Comment on attachment 231519 [details] [review] ViewSelector: remove unnecessary StBoxLayout Attachment 231519 [details] pushed as be10f3c - ViewSelector: remove unnecessary StBoxLayout
Created attachment 231622 [details] [review] Overview: Make Workspace manage its own padding Consolidate window clone padding application to Workspace, and pass the padded area down to LayoutStrategy. The padding is now configurable in CSS. This allows us to fix spacing in multimonitor cases (as there the workspace gets the full monitor as geometry), by adding a new style class to external workspaces. Ok, so now it deals with external monitors only.
Review of attachment 231521 [details] [review]: ++
Review of attachment 231622 [details] [review]: Looks good to me, except for these minor style comments ::: js/ui/workspace.js @@ +1167,3 @@ Name: 'Workspace', + _init : function(metaWorkspace, monitorIndex, styleClass) { Not a huge fan of passing down the styleClass here...I'd rather either have Workspace doing the check on its own by comparing monitorIndex to Main.layoutManager.primaryIndex or having the callers call workspace.actor.add_style_class_name() ::: js/ui/workspacesView.js @@ +699,3 @@ let metaWorkspace = global.screen.get_workspace_by_index(w); + let workspace = new Workspace.Workspace(metaWorkspace, i, + i != this._primaryIndex ? 'external-monitor' : null); We already do a comparison between 'i' and 'this._primaryIndex' in other two places in the outer loop; I'd rather have a let isPrimary = (i == this._primaryIndex); at the beginning of the loop iteration and use that instead for clarity.
Created attachment 233792 [details] [review] Overview: apply extra padding to windows in external monitor Add an style class targetting workspaces located outside the overview, and use it for extra padding around the window clones. Padding is passed down and applied inside LayoutStrategy, consolidating code that previously handled the bottom side only. With a better commit message too!
Review of attachment 233792 [details] [review]: Yes!
Attachment 231521 [details] pushed as c97b4dd - ViewSelector: use Params to pass optional parameters to addPage Attachment 233792 [details] pushed as 27ad830 - Overview: apply extra padding to windows in external monitor