GNOME Bugzilla – Bug 707580
appDisplay: Fix indicator animation position
Last modified: 2013-09-13 16:21:17 UTC
Animation of indicators are wrong in some situations i.e. when super + a pressed, since the calculation is now done based on the transformed position of the indicatorr when mapped, which its values are wrong. So base that on the padding instead.
Created attachment 254195 [details] [review] appDisplay: Fix indicator animation position The original position was calculated with the stage and the transformed position of the indicator when mapped. The values were wrong on some situations, so lets calculate the position based on the padding.
Created attachment 254199 [details] [review] appDisplay: Fix indicator animation position The original position was calculated with the stage and the transformed position of the indicator when mapped. The values were wrong on some situations, so lets calculate the position based on the padding.
Review of attachment 254195 [details] [review]: ::: data/theme/gnome-shell.css @@ +943,2 @@ .page-indicator { + padding: 15px 20px; Can you split out this change please? (It's a prerequisite of what you are doing, but: - it was necessary before, to have some spacing between scrollbar and monitor edge - it has the added benefit of making page indicators fully fittsable) ::: js/ui/appDisplay.js @@ +233,3 @@ + let side = Clutter.get_default_text_direction() == Clutter.TextDirection.RTL ? + St.Side.LEFT : St.Side.RIGHT; + let padding = this.actor.get_theme_node().get_padding(side); Not wrong, but we could just rely on this.actor not having any padding (for fittsability) @@ +239,2 @@ else + offset = monitor.x + children[0].width + padding; You can't drop monitor.width here
Review of attachment 254199 [details] [review]: Comments still apply
Review of attachment 254199 [details] [review]: ::: js/ui/appDisplay.js @@ +239,2 @@ else + offset = monitor.x + children[0].width + padding; Scrap the last comment, I was not reading the code correctly; however, the calculation is still wrong, I suggest: let node = this.actor.get_theme_node(); if (this.actor.get_text_direction() == Clutter.TextDirection.RTL) offset = -(children[0].width + node.get_padding(St.Side.LEFT)); else offset = children[0].width + node.get_padding(St.Side.RIGHT); You no longer need monitor at all as the calculation is not using screen coordinates anymore.
(In reply to comment #5) > Review of attachment 254199 [details] [review]: > > ::: js/ui/appDisplay.js > @@ +239,2 @@ > else > + offset = monitor.x + children[0].width + padding; > > Scrap the last comment, I was not reading the code correctly; however, the > calculation is still wrong, I suggest: > > let node = this.actor.get_theme_node(); > if (this.actor.get_text_direction() == Clutter.TextDirection.RTL) > offset = -(children[0].width + node.get_padding(St.Side.LEFT)); > else > offset = children[0].width + node.get_padding(St.Side.RIGHT); > > You no longer need monitor at all as the calculation is not using screen > coordinates anymore. True, I feel stupid, I thought: oh, if we have two monitors I still need that...no of course not for the offset =)
Created attachment 254213 [details] [review] theme: Erase appDisplay padding Since now AllView doesn't have a scrollbar this padding is not necesary, and will allow to place the indicators correctly using only the indicators padding in a upcoming patch.
Created attachment 254214 [details] [review] appDisplay: Fix indicator animation position The original position was calculated with the stage and the transformed position of the indicator when mapped. The values were wrong on some situations, so lets calculate the position based on the padding.
Review of attachment 254213 [details] [review]: I'd use "remove" in the commit message instead of "erase", looks good otherwise.
Review of attachment 254214 [details] [review]: ::: js/ui/appDisplay.js @@ +230,3 @@ + let node = this.actor.get_theme_node(); + if (this.actor.get_text_direction() == Clutter.TextDirection.RTL) + offset = -(children[0].width + node.get_padding(St.Side.LEFT)); Style nit: wrong indentation. I'm not sure the padding part makes a lot of sense to me: the code already relies on no other actor higher up the hierarchy adding padding on that side, why not make the same assumption for this.actor?
(In reply to comment #10) > Review of attachment 254214 [details] [review]: > > ::: js/ui/appDisplay.js > @@ +230,3 @@ > + let node = this.actor.get_theme_node(); > + if (this.actor.get_text_direction() == Clutter.TextDirection.RTL) > + offset = -(children[0].width + node.get_padding(St.Side.LEFT)); > > Style nit: wrong indentation. > > I'm not sure the padding part makes a lot of sense to me: the code already > relies on no other actor higher up the hierarchy adding padding on that side, > why not make the same assumption for this.actor? you're right, but in your previous comment you proposed this code, that's why I use this code (but yeah, I was using the padding previously, so I confused you) Fixed in upcomming patch
Created attachment 254632 [details] [review] appDisplay: Fix indicator animation position The original position was calculated with the stage and the transformed position of the indicator when mapped. The values were wrong on some situations, so lets calculate the position based on the dots width.
Review of attachment 254632 [details] [review]: LGTM
Attachment 254213 [details] pushed as 72f0a48 - theme: Erase appDisplay padding Attachment 254632 [details] pushed as 7c78e1f - appDisplay: Fix indicator animation position