Created attachment 174333 [details] [review]
window-manager: Swap workspace order for RTL locales

Workspaces should be aligned from right to left in RTL locales, so
take the text direction into account when switching workspaces.

Review of attachment 174333 [details] [review]:

windowManager.js changes look good except for a missing comment.

::: js/ui/windowManager.js
@@ +443,3 @@
 
+        if (St.Widget.get_default_direction() == St.TextDirection.RTL)
+            xDest *= -1;

This confused me initially: if I'm moving left, if I'm moving right, the animation should be the same no matter how the workspaces are numbered. Then I figured out that the code in meta_workspace_activate_with_focus() doesn't know about RTL so "right" and "left" might not be right and left. So you need a comment at the top of switch_workspace that explains the nature of the direction argument when it comes to RTL

::: js/ui/workspaceSwitcherPopup.js
@@ +93,3 @@
+
+        let x = rtl ? box.x2 - this._childWidth : box.x1;
+        let leftChildBoxX2 = x - this._itemSpacing;

Logic here no longer works as designed (a uniform spacing of itemSpacing between the items and the non-integer portions distributed among the items itself remaining.)

The logic here was probably somewhat overkill, but having it still here but not working in the RTL case doesn't make sense.

From experience, the easiest way to do RTL layout is usually to do the layout in LTR terms and flip at the last moment:

if (rtl) {
    childBox.x2 = box.x2 - (childBox.x1 - box.x1);
    childBox.x1 = box.x2 - (childBox.x2 - box.x1);
}

except with the added temporary so that actually works. Then you can not touch the overcomplicated layout code.

Created attachment 175496 [details] [review]
window-manager: Swap workspace order for RTL locales

(In reply to comment #2)
> +        if (St.Widget.get_default_direction() == St.TextDirection.RTL)
> +            xDest *= -1;
> 
> This confused me initially: if I'm moving left, if I'm moving right, the
> animation should be the same no matter how the workspaces are numbered. Then I
> figured out that the code in meta_workspace_activate_with_focus() doesn't know
> about RTL so "right" and "left" might not be right and left. So you need a
> comment at the top of switch_workspace that explains the nature of the
> direction argument when it comes to RTL

Fixed in mutter, so I removed this part of the patch.


> The logic here was probably somewhat overkill, but having it still here but not
> working in the RTL case doesn't make sense.
> 
> From experience, the easiest way to do RTL layout is usually to do the layout
> in LTR terms and flip at the last moment:
> 
> if (rtl) {
>     childBox.x2 = box.x2 - (childBox.x1 - box.x1);
>     childBox.x1 = box.x2 - (childBox.x2 - box.x1);
> }
> 
> except with the added temporary so that actually works. Then you can not touch
> the overcomplicated layout code.

Done.

Review of attachment 175496 [details] [review]:

Looks good

Attachment 175496 [details] pushed as 1efb021 - window-manager: Swap workspace order for RTL locales