(In reply to comment #0)
> It will be extremely useful to mix colors in the gtk theme to pass to mutter as
> a named color @foobar

Unless we tie the mutter theme to the gtk theme, we'll need a fallback when the gtk theme does not contain the custom color. Something like gtk:custom(foobar,#c0102) maybe?

gtk:custom(foobar,fallback) sounds good to me

Created attachment 187291 [details] [review]
theme: Allow using custom colors from the GTK+ theme

Add an additional color type to pick up colors defined with
@define-color in the GTK+ theme's CSS:

  gtk:custom/name/fallback

(where "name" refers to the name defined in GTK+'s CSS, and fallback
refers to an alternative color spec which is used when the color
referenced by "name" is not found)

The main intent of the change is to allow designers to improve
Adwaita's dark theme variant without having to compromise on colors
which work in the light variant as well.

(In reply to comment #2)
> gtk:custom(foobar,fallback) sounds good to me

I went with gtk:custom/foobar/fallback, which is
 - in line with the existing blend/shade functions
 - slightly easier to parse ;-)

Of course, if you insist I can update the patch to use parentheses instead ...

Created attachment 187386 [details] [review]
Test custom colors

Small patch against gnome-themes-standard for testing.

Comment on attachment 187386 [details] [review]
Test custom colors

(last patch is not meant for inclusion, so getting it of the review queue)

Shouldn't you be to mix blend/shade functions and this? isn't there then a problem with the syntax?

Created attachment 187995 [details] [review]
theme: Allow using custom colors from the GTK+ theme

(In reply to comment #7)
> Shouldn't you be to mix blend/shade functions and this? isn't there then a
> problem with the syntax?

Uhm, yeah ... I tested with blend/shade functions as fallback, but not with gtk:custom as parameter to those. So back to the initial gtk:custom(foo,bar) syntax.

Review of attachment 187995 [details] [review]:

some concerns about the parsing code (in particular, the first one, the rest are more questions)

::: src/ui/theme.c
@@ +1203,3 @@
+      MetaColorSpec *fallback = NULL;
+
+      color_name_start = strchr (str, '(');

Hmm, I don't think it's good to accept gtk:custom hah hah fooled you(tab-border,color) as a valid color. AFAICT, the theme format doesn't accept whitespace anywhere, so I think you can just demand that the ( is the next character.

@@ +1205,3 @@
+      color_name_start = strchr (str, '(');
+      fallback_str_start = strchr (str, ',');
+      end = strrchr (str, ')');

this means you can't do gtk:custom(color_name,gtk:custom(other_color_name,<last_gasp>)) which might be useful. Not sure if adding code to scan for a matching paren is worth the complexity though.

@@ +1207,3 @@
+      end = strrchr (str, ')');
+
+      if (color_name_start == NULL || fallback_str_start == NULL || end == NULL)

would it make sense to allow fallback not to be specified?

@@ +1225,3 @@
+
+      color_name = g_strndup (color_name_start + 1,
+                              fallback_str_start - color_name_start - 1);

I wonder if validating this to be a valid name would prevent weirdness for syntax errors. A quick look seems to indicate that this has to be a valid param name which has to be

 [A-Za-z][A-Za-z0-9-_]+

(In reply to comment #9)
> Review of attachment 187995 [details] [review]:
> 
> Hmm, I don't think it's good to accept gtk:custom hah hah fooled
> you(tab-border,color) as a valid color. AFAICT, the theme format doesn't accept
> whitespace anywhere, so I think you can just demand that the ( is the next
> character.

OK.


> @@ +1205,3 @@
> +      color_name_start = strchr (str, '(');
> +      fallback_str_start = strchr (str, ',');
> +      end = strrchr (str, ')');
> 
> this means you can't do
> gtk:custom(color_name,gtk:custom(other_color_name,<last_gasp>)) which might be
> useful. Not sure if adding code to scan for a matching paren is worth the
> complexity though.

No, that case is expected to work - note the use of strrchr instead of strchr for end (now, I only tested with correct syntax, so the primitive parsing might fail very badly on erroneous syntax ...)


> @@ +1207,3 @@
> +      end = strrchr (str, ')');
> +
> +      if (color_name_start == NULL || fallback_str_start == NULL || end ==
> NULL)
> 
> would it make sense to allow fallback not to be specified?


Not sure - meta_color_spec_render() looks like it's supposed to always fill in a color, but I guess we can use a nice shade of pink as hardcoded fallback in case it's omitted in the theme.


> @@ +1225,3 @@
> +
> +      color_name = g_strndup (color_name_start + 1,
> +                              fallback_str_start - color_name_start - 1);
> 
> I wonder if validating this to be a valid name would prevent weirdness for
> syntax errors. A quick look seems to indicate that this has to be a valid param
> name which has to be
> 
>  [A-Za-z][A-Za-z0-9-_]+

Makes sense ...

Created attachment 190025 [details] [review]
theme: Allow using custom colors from the GTK+ theme

Add some validation to parsing code.

(In reply to comment #9)
> I think you can just demand that the ( is the next character.

Done.


> I wonder if validating this to be a valid name would prevent weirdness for
> syntax errors. A quick look seems to indicate that this has to be a valid param
> name which has to be
> 
>  [A-Za-z][A-Za-z0-9-_]+

Done, but using [A_Za-z0-0-_]+ (which is what GTK+ uses in gtkcssparser.c:_gtk_css_parser_try_name)

Review of attachment 190025 [details] [review]:

Logic looks right to me; I'll note that there's one stylistic weirdness in the parsing code, but not a blocker - feel to commit without fixing.

::: src/ui/theme.c
@@ +1204,3 @@
+
+      color_name_start = str + 10;
+      if (*color_name_start != '(')

It's weird that color_name_start is one before the start of the color name, and you keep on correcting for that below.

@@ +1241,3 @@
+
+      fallback_str = g_strndup (fallback_str_start + 1,
+                                end - fallback_str_start - 1);

and same for fallback_str_start

Created attachment 191534 [details] [review]
theme: Allow using custom colors from the GTK+ theme

Updated to master.

Review of attachment 191534 [details] [review]:

Does this need review?

(In reply to comment #13)
> Created an attachment (id=191534) [details] [review]
> theme: Allow using custom colors from the GTK+ theme
> 
> Updated to master.

Sorry, missed above review - the attached patch is just a plain rebase, no need to look at it again.

Created attachment 191535 [details] [review]
Proposed interdiff

(In reply to comment #14)
> Does this need review?

This one does ;-)

Review of attachment 191535 [details] [review]:

Looks good

Attachment 191534 [details] pushed as 54b2fab - theme: Allow using custom colors from the GTK+ theme