Created attachment 172753 [details] [review]
ShellGenericContainer: add 'spacing' property

Many ShellGenericContainer implementations use a CSS 'spacing' property
as part of their layout, so add support for that directly to
ShellGenericContainer.

Created attachment 172754 [details] [review]
st_theme_node_get_color: fix inheritance

This wasn't actually recursing if you passed the "inherit" flag

Created attachment 172755 [details] [review]
StThemeNode: simplify use of get_color/get_double/get_length

Although within St itself there are situations where the semantics of
these functions (return TRUE or FALSE and return the actual value in
an out parameter) is useful, it's mostly just annoying at the
application level, where you generally know that the CSS property is
going to specified, and there is no especially sane fallback if it's
not.

So rename the current methods to get_color_full, get_double_full, and
get_length_full, and add new get_color, get_double, and get_length
methods that don't take an "inherit" parameter, and return their values
directly.

And update the code to use either the old or new methods as appropriate.

Review of attachment 172754 [details] [review]:

Looks good

Review of attachment 172755 [details] [review]:

Clearly removing the found out parameter that the JS code was uniformly ignoring is a good thing. Should we actually warn if you call get_length() and the property isn't found? (That would require a couple of changes to your patch where you do want the default to silently be zero.)

I'm not much of a fan of the _full() naming ... we went with a rule in glib/gtk+ at some point that full() meant "with GDestroyNotify" and nothing else. I'm not that strict on it, but I think it is usually a non-descriptive way to have two functions. Other ideas:

 get_length() - requires length to be set
 lookup_length() - can fail
 
 get_length() vs. check_length()

 get_length(..., 0.0); versus gboolean get_length_no_default(...., &out);
  (this does prevent adding the warning)

::: src/shell-generic-container.c
@@ -251,3 @@
           StThemeNode *node = st_widget_get_theme_node (ST_WIDGET (object));
 
-          priv->spacing = st_theme_node_get_length (node, "spacing", FALSE, 0.0);

this seems to be patching some code that wouldn't have compiled?

::: src/st/st-theme-node.c
@@ +923,3 @@
+ * See also st_theme_node_get_length(), which provides more options.
+ *
+ * Return value: the length, or 0 if the property was not defined.

Misses important piece of the previous docs 'The returned length is resolved to pixels.' For all of these, I think 'not found' is more accurate than 'not defined'.

::: src/st/st-theme-node.h
@@ +115,3 @@
+/* Easier-to-use variants of the above, for application-level use */
+ClutterColor *st_theme_node_get_color  (StThemeNode *node,
+                                        const char  *property_name);

Returning the ClutterColor allocated seems to be a workaround for the lack of out caller-allocates in gjs. Convenient from GJS, weird and inconvenient in C. I think I'd prefer to see this marked as out-caller-allocates, and leave the JS code passing in a allocated Clutter.Color until gjs gets fixed and it breaks, at which point we can fix the JS code.

That does pose a bit of a consistency challenge for the in-out get_color_full() - I think what I'd say is that I'd annotate it as (out caller-allocates) and the other versions as (out) and leave the fact that it's in-out a C easter-egg.

Review of attachment 172753 [details] [review]:

Obviously, ShellGenericContainer is just C/GJS glue so the standards are low, but the fact that various classes that derive from ShellGenericContainer have a spacing property doesn't mean that a spacing property makes sense on ShellGenericContainer

In general, I'm not sold that there is actually any point in caching style properties and connecting to style-changed. Looking up lengths isn't free - you have to do a linear search through the nodes properties and walk up the hierarchy to the top if it isn't set on the leaf node, but it isn't that expensive.

But if we want to make things look clean and simple, I think much cooler than this would be something like:

 UIUtils.defineLength(altTab.prototype, 'spacing');

(Should that be called defineCSSLength and define the property getter 'css_spacing' rather than 'spacing'?)

::: src/shell-generic-container.c
@@ +297,3 @@
+   * The value (in pixels) of the 'spacing' property in the actor's
+   * CSS (or 0.0 if the CSS does not specify a value). The
+   * implementation can use this as it sees fit.

You'd need a 'NOTE: this property is not notified' here

(In reply to comment #5)
> Should we actually warn if you call get_length() and
> the property isn't found? (That would require a couple of changes to your patch
> where you do want the default to silently be zero.)

In every case in JS (and half of the C cases), we wanted lengths to default to 0 if they weren't explicitly specified in the CSS. (Which is the way most standard (non-inherited) CSS lengths work too.)

Whereas get_double is generic enough that having a default didn't seem sane (eg, if we got the animation slowdown factor out of the CSS we'd want it to default to 1.0, not 0.0). For get_color, I thought about adding a flag to specify if it should fall back to the foreground or background color, but we're really only using custom CSS colors in places where we explicitly don't want either of those, so fallback didn't make much sense.

>  get_length() vs. check_length()

Heh. I actually had that naming convention in an earlier version of the patch, but when you read through a method using check_length() it implied the wrong thing about what the code was doing. lookup_length() might work though.

>  get_length(..., 0.0); versus gboolean get_length_no_default(...., &out);

And the earliest version of the patch replaced the in-outs with a default_value, but every single JS call was passing 0.0 for the default, so I got rid of that.

> -          priv->spacing = st_theme_node_get_length (node, "spacing", FALSE,
> 0.0);
> 
> this seems to be patching some code that wouldn't have compiled?

oops, indeed, that's against the "default_value" version of the patch. I guess I didn't test the intermediate stages after rebasing...

> Returning the ClutterColor allocated seems to be a workaround for the lack of
> out caller-allocates in gjs.

guilty as charged

(In reply to comment #6)
> But if we want to make things look clean and simple, I think much cooler than
> this would be something like:
> 
>  UIUtils.defineLength(altTab.prototype, 'spacing');

There's a patch for something like that in bug 611647, which I also have a newer version of, which I'll attach.

Comment on attachment 172754 [details] [review]
st_theme_node_get_color: fix inheritance

Attachment 172754 [details] pushed as 33e9557 - st_theme_node_get_color: fix inheritance

(In reply to comment #5)
> I think I'd prefer to see this marked as out-caller-allocates, and leave the JS
> code passing in a allocated Clutter.Color until gjs gets fixed and it breaks,
> at which point we can fix the JS code.

gjs appears to currently treat (out caller-allocates) identically to (out), rather than ignoring it. So we have to leave it un-annotated.

Created attachment 172945 [details] [review]
StThemeNode: simplify use of get_color/get_double/get_length

rename _full methods to _lookup_
no longer depends on the ShellGenericContainer.spacing patch

Review of attachment 172945 [details] [review]:

Looks good to me

::: src/st/st-theme-node.c
@@ +917,3 @@
+ * specific getters (like st_theme_node_get_border_width()) exist, they
+ * should be used instead. They are cached, so more efficient, and have
+ * handling for shortcut properties and other details of CSS.

Wouldn't hurt to explicitly point out that this doesn't warn

Created attachment 172949 [details] [review]
css: add CSS.bindLength and CSS.bindColor

not hugely simplifying really... maybe not worth doing.

Comment on attachment 172945 [details] [review]
StThemeNode: simplify use of get_color/get_double/get_length

Attachment 172945 [details] pushed as 8886b74 - StThemeNode: simplify use of get_color/get_double/get_length

closing this now since the remaining uncommitted patch didn't simplify things enough to be worth it