Created attachment 182428 [details] [review]
st-widget: Add automagic rtl/ltr pseudo classes

At times, RTL locales require different CSS, so always create theme
nodes with :ltr/:rtl pseudo classes depending on the widget's text
direction, to provide an easy way to handle those cases without
requiring a separate stylesheet.

Created attachment 182429 [details] [review]
Remove manually added :rtl pseudo classes

To deal with different CSS in RTL locales, we used to manually add
an :rtl pseudo class to some actors. With automatically assigned
:ltr/:rtl selectors this is no longer necessary.

Created attachment 182430 [details] [review]
theme: Adjust CSS for RTL locales

Use :ltr/:rtl pseudo selectors where the CSS depends on the locale's
text direction.

Review of attachment 182428 [details] [review]:

basically good, few details

::: src/st/st-widget.c
@@ +555,3 @@
       ClutterStage *stage = NULL;
       ClutterActor *parent;
+      char *pseudo_classes;

This isn't any more plural than priv->pseudo_class - so stick with pseudo_class for consistency

@@ +588,3 @@
+                                         is_rtl ? "rtl" : "ltr");
+      else
+        pseudo_classes = g_strdup_printf("%s", is_rtl ? "rtl" : "ltr");

Not really sure it's worth it (OK pretty sure it's not really worth it), but since we do allocate a lot of theme nodes, I probably would go for some malloc microptimization here.

 direction_pseudo_class = <....> ? "rtl" : "ltr";
 if (priv->pseudo_class)
    pseudo_class = g_strconcat(priv->pseudo_class, " ", direction_pseudo_class, NULL);
 else
   pseudo_class = direction_pseudo_class;

[...]
  if (pseudo_class != direction_pseudo_class)
  g_free (pseudo_class);

@@ +1461,3 @@
   self->priv->direction = dir;
+
+  if (old_direction != self->priv->direction)

This should be st_widget_get_direction() again or repeated calls to st_widget_direction(widget, NONE) will all cause notification

Review of attachment 182429 [details] [review]:

Looks good

Review of attachment 182430 [details] [review]:

The advantage of avoiding :ltr pseudo-classes is that we considerably reduce the total number of rules which is the #1 thing that worries me about our css performance. For each ST actor we scan *all* the CSS rules linearly. Without really profiling, I think it's going to be much more important to reduce the total number of CSS rules we have than to avoid having a few extra properties on individual theme nodes in RTL mode. So, my preference here would be to use the existing style of css for ltr mode + rtl overrides rather than the approach you've taken here. I agree that the approach here is a bit easier to read and more logically consistent.

Created attachment 182568 [details] [review]
st-widget: Add automagic rtl/ltr pseudo classes

(In reply to comment #4)
> This isn't any more plural than priv->pseudo_class - so stick with pseudo_class
> for consistency

I had that first, but then decided that plural were better for both (though only changing the new variable). Fixed.


> @@ +588,3 @@
> Not really sure it's worth it, but since we do allocate a lot of theme nodes,
> I probably would go for some malloc microptimization here.

OK.


> @@ +1461,3 @@
> This should be st_widget_get_direction() again or repeated calls to
> st_widget_direction(widget, NONE) will all cause notification

Fixed.

Created attachment 182572 [details] [review]
theme: Adjust CSS for RTL locales

(In reply to comment #6)
> The advantage of avoiding :ltr pseudo-classes is that we considerably reduce
> the total number of rules which is the #1 thing that worries me about our css
> performance.

OK, I removed all :ltr pseudo-classes, except where all node properties are locale dependent.

Review of attachment 182568 [details] [review]:

Looks good

Review of attachment 182572 [details] [review]:

Thanks! I'm just really guessing about performance, .but I think the difference might be significant.

The changes look right reading through the patch, could be there's still something missed somewhere, but we can always fix remaining nits as they are found.

Attachment 182429 [details] pushed as 8ed1912 - Remove manually added :rtl pseudo classes
Attachment 182568 [details] pushed as fefe2df - st-widget: Add automagic rtl/ltr pseudo classes
Attachment 182572 [details] pushed as 0b1bf5f - theme: Adjust CSS for RTL locales

(In reply to comment #10)
> The changes look right reading through the patch, could be there's still
> something missed somewhere, but we can always fix remaining nits as they are
> found.

Yeah, that's what I was thinking. There's still stuff to do for popup menus anyway ...