Created attachment 285758 [details] [review]
popupMenu: Adjust submenu arrows to RTL icon changes

Since commit e04e507659027, we will already get the right icon for the
submenu arrow, so we must not mirror it again. However we do need to
take the text direction into account for the rotation now (but that's
not actually too bad - the resulting code gets quite a bit easier).

It look ok with your patch, thanks!

Review of attachment 285758 [details] [review]:

LGTM overall

::: js/ui/popupMenu.js
@@ +925,3 @@
+        let targetAngle = 90;
+        if (this.actor.text_direction == Clutter.TextDirection.RTL)
+            targetAngle *= -1;

Personal preference, but looks clearer in this way maybe? Feel free to skip my proposal tho.
let targetAngle = this.actor.text_direction == Clutter.TextDirection.RTL ? -90 : 90

@@ -1058,3 @@
         this._triangleBin.add_child(this._triangle);
-        if (this._triangleBin.get_text_direction() == Clutter.TextDirection.RTL)
-            this._triangleBin.set_scale(-1.0, 1.0);

This is a drive-by change right?

Attachment 285758 [details] pushed as 6823bad - popupMenu: Adjust submenu arrows to RTL icon changes

(In reply to comment #3)
> Review of attachment 285758 [details] [review]:
> Personal preference, but looks clearer in this way maybe? Feel free to skip my
> proposal tho.
> let targetAngle = this.actor.text_direction == Clutter.TextDirection.RTL ? -90
> : 90

I tend to fit code into 80-character lines, but yeah, that's a fairly old-fashioned and outdated convention :-)
Changed.


> @@ -1058,3 @@
>          this._triangleBin.add_child(this._triangle);
> -        if (this._triangleBin.get_text_direction() ==
> Clutter.TextDirection.RTL)
> -            this._triangleBin.set_scale(-1.0, 1.0);
> 
> This is a drive-by change right?

Not at all - that's how we used to handle the RTL case: always use the LTR icon, mirror it horizontally, and use relative angles for rotation.
Now that we no longer use the LTR icon unconditionally, mirroring it becomes plain wrong, and we can no longer use the same rotation for both cases. (However we could still use relative angles for the rotation, but now that we no longer have to, it makes sense to simplify that code - though you could consider that part a drive-by change ...)

(In reply to comment #4)
> Attachment 285758 [details] pushed as 6823bad - popupMenu: Adjust submenu arrows to RTL
> icon changes
> 
> (In reply to comment #3)
> > Review of attachment 285758 [details] [review] [details]:
> > Personal preference, but looks clearer in this way maybe? Feel free to skip my
> > proposal tho.
> > let targetAngle = this.actor.text_direction == Clutter.TextDirection.RTL ? -90
> > : 90
> 
> I tend to fit code into 80-character lines, but yeah, that's a fairly
> old-fashioned and outdated convention :-)
> Changed.
> 
ah yeah, me too. Still more clear in this way I think =)
> 
> > @@ -1058,3 @@
> >          this._triangleBin.add_child(this._triangle);
> > -        if (this._triangleBin.get_text_direction() ==
> > Clutter.TextDirection.RTL)
> > -            this._triangleBin.set_scale(-1.0, 1.0);
> > 
> > This is a drive-by change right?
> 
> Not at all - that's how we used to handle the RTL case: always use the LTR
> icon, mirror it horizontally, and use relative angles for rotation.
> Now that we no longer use the LTR icon unconditionally, mirroring it becomes
> plain wrong, and we can no longer use the same rotation for both cases.
> (However we could still use relative angles for the rotation, but now that we
> no longer have to, it makes sense to simplify that code - though you could
> consider that part a drive-by change ...)
Ah yeah, I though it was only the animation that was wrong.