Created attachment 188120 [details] [review]
gradient: Port from GdkColor to GdkRGBA

GdkColor is about to be deprecated, so move to GdkRGBA instead.
It might be worth considering using cairo patterns for the gradients
rather than using custom code to render gradients to a pixbuf which
is then drawn with cairo, but for now this is just a straight port
of the existing code.

Created attachment 188121 [details] [review]
theme: Port from GdkColor to GdkRGBA

GdkColor is about to be deprecated, so move to GdkRGBA instead.

Created attachment 188122 [details] [review]
theme-viewer: Port from GdkColor to GdkRGBA

GdkColor is about to be deprecated, so move to GdkRGBA instead.

Created attachment 188123 [details] [review]
mutter-window-demo: Port from GdkColor to GdkRGBA

GdkColor is about to be deprecated, so move to GdkRGBA instead.

Created attachment 188124 [details] [review]
theme: Add helper functions for light/dark colors

GtkStyleContext no longer has dark/light colors GtkStyle used to
have. We already have compatibility code for them in theme.c, so
add two helper functions to make it available outside theme.c.

Created attachment 188125 [details] [review]
draw-workspace: Update to current GTK style API

Move from GtkStyle to GtkStyleContext.

Created attachment 188126 [details] [review]
theme: Port from GtkStateType to GtkStateFlags

We now use GtkStyleContext exclusively, so it's a bit weird to store
widget state as GtkStateType and translate it always to GtkStateFlags.
Just use GtkStateFlags instead of GtkStateType.

Review of attachment 188120 [details] [review]:

Seems OK - lots of dead code of course - only meta_gradient_create_multi() is actually used.

::: src/ui/testgradient.c
@@ +39,3 @@
 #define SPACING 2  
 
+  color1.red = 30000./65535.;

spaces around binary operators

@@ +57,3 @@
 	  if (ycount % 2)
+	    cairo_set_source_rgb (cr,
+	                          color1.red, color1.green, color1.blue);

hmm, why didn't you use gdk_cairo_set_source_rgba() here? Because alpha is undefined? define alpha?

Review of attachment 188122 [details] [review]:

Sure

Review of attachment 188123 [details] [review]:

Sure

Review of attachment 188124 [details] [review]:

Mostly looks fine, one thing I didn't quite like stylistically

::: src/ui/theme.c
@@ +1489,1 @@
+      meta_gtk_style_get_dark_color (context, flags, &dark);

why not just handle COLOR_MID: separately and fully in the switch? Saving one function call in the compiled code doesn't seem worth a complicated code flow.

Review of attachment 188125 [details] [review]:

Looks fine

Created attachment 191491 [details] [review]
theme: Add helper functions for light/dark colors

(In reply to comment #11)
> why not just handle COLOR_MID: separately and fully in the switch? Saving one
> function call in the compiled code doesn't seem worth a complicated code flow.

Sure, I did that mostly to not deviate too much from the existing code. I don't think it makes sense to move COLOR_MID into the switch and handle COLOR_TEXT_AA differently, so I moved everything into the switch statement now ...

Review of attachment 188126 [details] [review]:

Looks fine (little weirdness in that we're using a flags as an enumeration, but it's all under our control)

Review of attachment 191491 [details] [review]:

Looks good

Review of attachment 188121 [details] [review]:

Generally looks god to me, once comment:

::: src/ui/theme.c
@@ +1490,3 @@
 meta_color_spec_render (MetaColorSpec   *spec,
                         GtkStyleContext *context,
+                        GdkRGBA         *color)

Maybe add a g_assert(color->alpha == 1.0) at the end of this, since you count on that elsewhere (by using gdk_cairo_set_source_rgba()) and it's sort of "coincidentally true" when I read through the code paths.

Created attachment 191495 [details] [review]
theme: Add assert according to review

(In reply to comment #16)
> Maybe add a g_assert(color->alpha == 1.0) at the end of this, since you count
> on that elsewhere (by using gdk_cairo_set_source_rgba()) and it's sort of
> "coincidentally true" when I read through the code paths.

Sure. I think the assert needs a comment though, can you give it a quick look?

Review of attachment 191495 [details] [review]:

my concern wasn't really that the drawing code assumes opaque colors (though I guess some, like the gradient code does), but rather that the way that the code above manipulates colors assumes that they are opaque - e.g, it blends colors by only blending the RGB.

However, it occurs to me that this assert might cause mutter to crash if a GTK+ theme specified a non-opaque color for foreground/background/etc. Which is bad. So, I think the assert is just a bad idea - my idea was that the assert would be to catch bugs in our code, not cases that we know are unhandled and that just result in ugly results for weird themes.

Could just add a warning rather than an assert, but I'm not sure a warning really helps anybody - if you try to do it and it is broken, you'll file a bug, or just do nothing. So, I'd say skip this.

(In reply to comment #18)
> my concern wasn't really that the drawing code assumes opaque colors (though I
> guess some, like the gradient code does), but rather that the way that the code
> above manipulates colors assumes that they are opaque - e.g, it blends colors
> by only blending the RGB.

Ah, OK.


> However, it occurs to me that this assert might cause mutter to crash if a GTK+
> theme specified a non-opaque color for foreground/background/etc. Which is bad.
> So, I think the assert is just a bad idea - my idea was that the assert would
> be to catch bugs in our code, not cases that we know are unhandled and that
> just result in ugly results for weird themes.

That's what I was actually thinking of - I don't think the metacity theme format allows non-opaque colors, but GTK+ does. In particular stuff like trying to set a transparent background color would produce unexpected results, e.g. drawing on top of the default background rather than producing translucent frames - I thought that was what you were referring to (but yeah, crashing is rather harsh under these circumstances ...)


> Could just add a warning rather than an assert, but I'm not sure a warning
> really helps anybody - if you try to do it and it is broken, you'll file a bug,
> or just do nothing. So, I'd say skip this.

OK. I was going to suggest to warn and force the alpha component to 1.0, but the result would probably equally confusing as the current brokenness.

Attachment 188120 [details] pushed as 8199699 - gradient: Port from GdkColor to GdkRGBA
Attachment 188121 [details] pushed as ce13696 - theme: Port from GdkColor to GdkRGBA
Attachment 188122 [details] pushed as 60ee25d - theme-viewer: Port from GdkColor to GdkRGBA
Attachment 188123 [details] pushed as e16beba - mutter-window-demo: Port from GdkColor to GdkRGBA
Attachment 188125 [details] pushed as 0345702 - draw-workspace: Update to current GTK style API
Attachment 188126 [details] pushed as d95da2d - theme: Port from GtkStateType to GtkStateFlags
Attachment 191491 [details] pushed as f8d900c - theme: Add helper functions for light/dark colors

(In reply to comment #8)
> ::: src/ui/testgradient.c
> @@ +39,3 @@
> +  color1.red = 30000./65535.;
> 
> spaces around binary operators

Fixed.


> @@ +57,3 @@
>        if (ycount % 2)
> +        cairo_set_source_rgb (cr,
> +                              color1.red, color1.green, color1.blue);
> 
> hmm, why didn't you use gdk_cairo_set_source_rgba() here? Because alpha is
> undefined? define alpha?

Updated to use gdk_cairo_set_source_rgba() (with alpha == 1.0).