After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 707580 - appDisplay: Fix indicator animation position
appDisplay: Fix indicator animation position
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2013-09-05 17:43 UTC by Carlos Soriano
Modified: 2013-09-13 16:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
appDisplay: Fix indicator animation position (2.10 KB, patch)
2013-09-05 17:43 UTC, Carlos Soriano
needs-work Details | Review
appDisplay: Fix indicator animation position (1.92 KB, patch)
2013-09-05 17:50 UTC, Carlos Soriano
needs-work Details | Review
theme: Erase appDisplay padding (805 bytes, patch)
2013-09-05 22:46 UTC, Carlos Soriano
committed Details | Review
appDisplay: Fix indicator animation position (1.53 KB, patch)
2013-09-05 22:46 UTC, Carlos Soriano
reviewed Details | Review
appDisplay: Fix indicator animation position (1.47 KB, patch)
2013-09-10 23:15 UTC, Carlos Soriano
committed Details | Review

Description Carlos Soriano 2013-09-05 17:43:55 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.
Comment 1 Carlos Soriano 2013-09-05 17:43:58 UTC
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.
Comment 2 Carlos Soriano 2013-09-05 17:50:20 UTC
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.
Comment 3 Florian Müllner 2013-09-05 17:53:30 UTC
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
Comment 4 Florian Müllner 2013-09-05 17:54:58 UTC
Review of attachment 254199 [details] [review]:

Comments still apply
Comment 5 Florian Müllner 2013-09-05 20:16:08 UTC
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.
Comment 6 Carlos Soriano 2013-09-05 22:27:38 UTC
(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 =)
Comment 7 Carlos Soriano 2013-09-05 22:46:30 UTC
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.
Comment 8 Carlos Soriano 2013-09-05 22:46:50 UTC
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.
Comment 9 Florian Müllner 2013-09-10 19:22:34 UTC
Review of attachment 254213 [details] [review]:

I'd use "remove" in the commit message instead of "erase", looks good otherwise.
Comment 10 Florian Müllner 2013-09-10 19:22:39 UTC
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?
Comment 11 Carlos Soriano 2013-09-10 22:56:22 UTC
(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
Comment 12 Carlos Soriano 2013-09-10 23:15:18 UTC
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.
Comment 13 Florian Müllner 2013-09-13 15:32:01 UTC
Review of attachment 254632 [details] [review]:

LGTM
Comment 14 Carlos Soriano 2013-09-13 16:21:08 UTC
Attachment 254213 [details] pushed as 72f0a48 - theme: Erase appDisplay padding
Attachment 254632 [details] pushed as 7c78e1f - appDisplay: Fix indicator animation position