I think it makes sense to support the text-align property.

Some random thoughts though:

 - relying on StTextAlign to be in sync with PangoAlignment
   looks like a really bad idea

 - it should be possible to set text-align on a label's parent
   (I know text-decoration isn't inherited either, but I'd argue
    that it should)

 - I think it would be better to set the alignment in StLabel instead of
   st-theme-node-private, so the alignment can be flipped depending
   on the label's text-direction (I'm not sure if this would require an
   option to enable/disable or if we always want this - I guess the latter,
   but we'd have to check)

1) Why? You save a table lookup / switch statement.
And its not that directions appear suddenly: it's either left, right or center.

2) For that to work, you need to put
* { text-align: inherit; }
at the beginning of the global (UA-level) stylesheet.
Cascade will do the rest.
(Else you can modify st-theme-node-get-text-align() to look for the parent's value if no valid declaration is found).

3) I think left should be left and right should be right, matching existing CSS specification. Then you can add start and end, as other implementations did, to get automatic reversing based on direction.

(In reply to comment #2)
> 1) Why? You save a table lookup / switch statement.
> And its not that directions appear suddenly: it's either left, right or center.

Possible case: someone decides to reflect 'inherit' in the enum itself and adds it - without the context of your patch it is not obvious at all that this can break alignment. If you insist on re-using the pango enum, you can either replace StTextAlign with PangoAlignment or assign explicit values in the enum declaration.


> 2) For that to work, you need to put
> * { text-align: inherit; }
> at the beginning of the global (UA-level) stylesheet.
> Cascade will do the rest.

Sure - on the other hand,
* { color: white; }
is not necessary to make text pick up the parent's color, so I'm suggesting to be consistent here. Also note that there is no UA-level in the shell, so we must either rely on theme authors to include it, or to always assign a style class to a label, even when we consider it as part of a compound widget.


> 3) I think left should be left and right should be right, matching existing CSS
> specification.

We should at least consider diverging from the spec here if following the spec is the only reason not to. GNOME Shell will never be CSS compliant anyway (the HTML box model works fundamentally different than Clutter/GTK+), so it is better to be pragmatic.

1) I cannot use directly PangoAlignment, as "justify" is missing. I will assign values explicitly then.

2) Ok, I'll correct the patch to support automatic inheritance.

3) Uhm... what is currently implemented for padding and border?

Created attachment 164385 [details] [review]
Corrected patch

(In reply to comment #4)
> 3) Uhm... what is currently implemented for padding and border?

Hmm, this is a valid point - there's no flipping there (which looks pretty much always wrong if the values for left and right differ, but still ...)

Yeah, from earlier discussion about RTL and CSS, I think we agreed that CSS should NOT auto-flip. Either widgets would automatically get a pseudo class of "rtl" in RTL locales, and the CSS could have overrides using that, or else there'd be a separate "gnome-shell-rtl.css" that would only be loaded in RTL locales.

Review of attachment 164385 [details] [review]:

Thanks for the update, looks good except for minor comments.

There's one thing which needs a little discussion though - with the current code, st_theme_node_get_text_align() always returns an alignment (there's no ST_TEXT_ALIGN_NONE). This means that it is no longer possible to set the text alignment from code(*) to change the default alignment - it will always be overwritten by the style's default, even if not specified anywhere in the CSS. If this is what we want, you should remove the alignment code in appDisplay.js and update the style. Otherwise, the patch should be modified to allow setting a default value in the code.

(*) except in a handler for the style-changed signal overwriting the theme, but that would be oh-so wrong

::: src/st/st-theme-node.c
@@ +1780,3 @@
+{
+st_theme_node_get_text_align(StThemeNode *node)
+StTextAlign

Unnecessary if done by default anyway.

::: src/st/st-theme-node.h
@@ +4,2 @@
 #define __ST_THEME_NODE_H__
+#include "st-types.h"

Not using anything from st-types, so no reason to change the original include

Created attachment 164419 [details] [review]
Third patch


1) Except that you can have actor.style="text-align: whatever";

2) No, "inherit" as a value may override other properties set earlier on the cascade, so needs to be handled there as well.

3) I added it because without it the patched didn't seem to compile, but actually works also without. Just killed that part of the patch.

Comment on attachment 164419 [details] [review]
Third patch

(In reply to comment #9)
> Except that you can have actor.style="text-align: whatever";

That is not the same, look at the order in which it is applied:
 1. clutter_text.line_alignment = ...
 2. [CSS]
 3. actor.style=...

So (1) is setting a default which can be overwritten by the theme(2), (3) overwrites the theme.

That does not mean that we absolutely need the possibility to do (1) - it just happens that we currently do, so the patch breaks the current design.

So please, either
 (1) Don't set the alignment unless it's specified in the CSS

 (2) Port the existing code to CSS


Also, you should update the commit message - apart from missing capitalization, it suggests that the property only applies to multi-line text, which is not the case. Maybe:

[StThemeNode] Implement "text-align" property

Implement the CSS text-align property to control the alignment of text.
If not specified, text is left-aligned.

(Assuming you prefer (2) - otherwise you can say that it leaves the Clutter defaults in place or something)

Created attachment 164436 [details] [review]
Removes use of clutter_text_set_line_alignment

I'd say that the way forward is 2).
Having 1) would be confusing, as text-align is default inherited (and the patch was explicitly modified to get default inheritance), so only elements without any parent's text-align property would get the setting from code.
After a rapid search, AppDisplay.AppIcon was the only class using line_alignment.


For what concerns commit message, I defer to ClutterText docs, that describe clutter_text_set_line_alignment as applying only to multiline text.

(In reply to comment #11)
> I'd say that the way forward is 2).
> Having 1) would be confusing, as text-align is default inherited (and the patch
> was explicitly modified to get default inheritance), so only elements without
> any parent's text-align property would get the setting from code.

I disagree that it would be confusing, but 2) is as good as 1), so let's just go with that - can you squash the patches, please?


> For what concerns commit message, I defer to ClutterText docs, that describe
> clutter_text_set_line_alignment as applying only to multiline text.

Then the clutter docs are wrong - guess how I noticed that the patch was overwriting code ;)

The labels in appDisplay are hardly multiline - most of them have a larger-then-necessary allocation though, which is what triggers the alignment ...

Also please take a look at http://lists.cairographics.org/archives/cairo/2008-September/015092.html

Created attachment 164440 [details] [review]
Implement "text-align"

"text-align" allows setting the alignment of text, with respect to
other lines and allocated space, without requiring a reference to
the ClutterText (which is private for most widgets).
If not specified, all text is left-aligned.

Thanks!

One more suggestion:

+.app-icon {
+    text-align: center;
+}
+
 .app-well-app {
     border: 1px solid #181818;
     border-radius: 4px;

app-icon is not used outside of app-well-app, so you could move the text-align property there to cut down on style classes (and it's equally correct - app-icon does not refer to the label directly either) ...

Created attachment 164442 [details] [review]
Incorporate last idea

Comment on attachment 164442 [details] [review]
Incorporate last idea

Thanks!

Pushed.