GNOME Bugzilla – Bug 694261
Applications grid isn't horizontally centered
Last modified: 2013-03-01 17:26:30 UTC
This is a pretty obvious issue with the new applications view. It might be a more generic issue with horizontal alignment in the overview. A few other examples where horizontal centering is lacking: * Dragging a launcher from search results causes the dash to slide in. The search results respond by moving across - they should stay in their centered position. * In the window selector view, if the window thumbnails are narrower than the available space they aren't centered in the middle of the screen, but are instead positioned half-way between the dash and workspace switcher. It *might* be better if they made an effort to be centered (I'm not entirely sure on this point).
(In reply to comment #0) > It might be a more generic issue with horizontal alignment in the overview. Yup. The content part (window picker, app picker, search results) are centered between the dash's right edge and the right monitor edge. Or in other words: when the dash is visible, it's not centered at all. The correct solution is probably to actually layer the dash on top of the content. (I'll take a look into that post-release).
(In reply to comment #0) > * In the window selector view, if the window thumbnails are narrower than the > available space they aren't centered in the middle of the screen, but are > instead positioned half-way between the dash and workspace switcher. It *might* > be better if they made an effort to be centered (I'm not entirely sure on this > point). We've tried this. It looks... bad. (In reply to comment #1) > The correct solution is probably to actually layer the dash on top of the > content. (I'll take a look into that post-release). ... except in the window picker case
Created attachment 237092 [details] [review] overview: Move dash to a separate layer We generally want view content centered, in particular where the view itself is symmetrical. So move the dash to a separate layer and use a placeholder to account for its size when showing the window picker, which is the only view where it doesn't make sense to center the content.
Created attachment 237093 [details] [review] overviewControls: Make DashSpacer fancier Make dash-actor a GObject property, which is a bit fancier :-) To be merged with the previous patch if we consider it a good idea, or dropped otherwise.
Review of attachment 237093 [details] [review]: ::: js/ui/overviewControls.js @@ +335,3 @@ Extends: St.Widget, + Properties: { + 'dash_actor': GObject.ParamSpec._new_internal('dash-actor', GObject.type_from_name('ClutterActor'), 'dash-actor', 'dash-actor', GObject.ParamFlags.WRITABLE, null, null, null) You should just be able to use Clutter.Actor here and have everything work out.
Created attachment 237094 [details] [review] overviewControls: Make DashSpacer fancier Cool, that's helpful.
Created attachment 237115 [details] [review] appDisplay: Center AllView content If the AllView is scrolled, the vertical scrollbar will take away some horizontal space on one side, resulting in the content ending up slightly off-center. Account for this by setting appropriate padding on the opposite side. (This is the non-fancy variant)
Created attachment 237116 [details] [review] st-scroll-view: Add :overlay-scrollbars property If enabled, scrollbars take away from the allocation given to the view's content. This is usually preferrable to painting the bars on top of the content, but there are exceptions, for instance when the content needs to be centered with regard to the view as a whole. Add a :overlay-scrollbars property to account for those cases.
Created attachment 237117 [details] [review] appDisplay: Center AllView content If the AllView is scrolled, the vertical scrollbar will take away some horizontal space on one side, resulting in the content ending up slightly off-center. Account for this by using overlay-scrollbars instead. (This is the fancy variant)
Created attachment 237123 [details] [review] appDisplay: Center AllView content Rebased to master.
Created attachment 237124 [details] [review] appDisplay: Apply the same padding to AllView and FrequentView We add some horizontal padding to the AllView's content to make sure content does not end up underneath the scrollbar; while this is not required in case of the FrequentView which does not have a scrollbar, applying the same padding ensures that both views end up with the same spacing, which makes switching between them less disruptive.
Created attachment 237133 [details] [review] appDisplay: Center AllView content Fix CSS.
Created attachment 237134 [details] [review] appDisplay: Apply the same padding to AllView and FrequentView Fix CSS.
Review of attachment 237116 [details] [review]: Looks good, but you should probably add a test in tests/scroll-view-sizing.js (we do have tests for all other properties here).
Created attachment 237149 [details] [review] st-scroll-view: Add :overlay-scrollbars property Good point, added test case.
Created attachment 237150 [details] [review] appDisplay: Center AllView content Rebase to master
Created attachment 237151 [details] [review] appDisplay: Apply the same padding to AllView and FrequentView Rebase to master
Review of attachment 237092 [details] [review]: Looks good.
Review of attachment 237149 [details] [review]: Looks good.
*** Bug 694260 has been marked as a duplicate of this bug. ***
Review of attachment 237094 [details] [review]: ::: js/ui/overviewControls.js @@ +334,3 @@ Extends: St.Widget, + Properties: { + 'dash_actor': GObject.ParamSpec._new_internal('dash-actor', Clutter.Actor, 'dash-actor', 'dash-actor', GObject.ParamFlags.WRITABLE, null, null, null) Line breaks might not be a bad idea here ;)
Review of attachment 237150 [details] [review]: OK.
Review of attachment 237151 [details] [review]: Makes sense.
Created attachment 237156 [details] [review] overview: Move dash to a separate layer Turned out that the previous patch messed up the transition from app-picker to normal mode (e.g. <super>a - <super>).
(In reply to comment #21) > Extends: St.Widget, > + Properties: { > + 'dash_actor': GObject.ParamSpec._new_internal('dash-actor', > Clutter.Actor, 'dash-actor', 'dash-actor', GObject.ParamFlags.WRITABLE, null, > null, null) > > Line breaks might not be a bad idea here ;) Right, fixed locally. I'd still like some feedback from Jasper on whether he considers GObject properties safe to use before squashing this.
Attachment 237149 [details] pushed as 1bd8c67 - st-scroll-view: Add :overlay-scrollbars property Attachment 237150 [details] pushed as 6ea8f35 - appDisplay: Center AllView content Attachment 237151 [details] pushed as 62ca4ba - appDisplay: Apply the same padding to AllView and FrequentView
(In reply to comment #25) > Right, fixed locally. I'd still like some feedback from Jasper on whether he > considers GObject properties safe to use before squashing this. We use them in MonitorConstraint, so they're safe in terms of runtime. I don't really see the utility of bouncing through GObect for this. _new_internal API may go away at some point (if we add fundamentals support and use the native g_param_spec_* functions), but we probably really should add GObject.ParamSpec.new_object or something.
Review of attachment 237156 [details] [review]: ::: js/ui/overviewControls.js @@ +348,3 @@ + if (dashActor) { + this._bindConstraint = new Clutter.BindConstraint({ source: dashActor, + coordinate: Clutter.BindCoordinate.SIZE }); Are we sure the bind constraint is the best idea? It's given us nothing but allocation warnings in the past...
(In reply to comment #28) > Are we sure the bind constraint is the best idea? It's given us nothing but > allocation warnings in the past... That was an align constraint, I've been running with this patch for a while now without any allocation warnings.
(In reply to comment #29) > (In reply to comment #28) > > Are we sure the bind constraint is the best idea? It's given us nothing but > > allocation warnings in the past... > > That was an align constraint, I've been running with this patch for a while now > without any allocation warnings. I've just tested explicitly for dash size changes without warnings.
(In reply to comment #28) > Review of attachment 237156 [details] [review]: > > ::: js/ui/overviewControls.js > @@ +348,3 @@ > + if (dashActor) { > + this._bindConstraint = new Clutter.BindConstraint({ source: > dashActor, > + coordinate: > Clutter.BindCoordinate.SIZE }); > > Are we sure the bind constraint is the best idea? It's given us nothing but > allocation warnings in the past... Huh? why would a constraint cause allocation warnings? If anything it avoids them ... We had issue like binding to the stage / uiGroup causing full screen redraws but I can't recall constraints ever causing allocation loops (there purpose is to avoid them).
(In reply to comment #31) > Huh? > why would a constraint cause allocation warnings? If anything it avoids them > ... Not in every case, unfortunately: http://git.gnome.org/browse/gnome-shell/commit?id=95602eb85d167c9
(In reply to comment #32) > (In reply to comment #31) > > Huh? > > why would a constraint cause allocation warnings? If anything it avoids them > > ... > > Not in every case, unfortunately: > http://git.gnome.org/browse/gnome-shell/commit?id=95602eb85d167c9 OK, but there the reason was that the constraint referenced one of the actors parents. (So it changed the allocation of the children caused the parent to reallocate caused the constraint to fire .... )
Yeah, I'm not arguing against using a constraint in *this* bug, which is a different case (and should be completely fine) ...
Review of attachment 237156 [details] [review]: OK.
Attachment 237156 [details] pushed as cca008b - overview: Move dash to a separate layer