Created attachment 166577 [details] [review]
[StThemeNode] Add text-shadow property

Reorganize the existing code which parses the -st-shadow property
to allow parsing different shadow properties and add support for
the text-shadow property.

(The series looks scarier than it is - it mostly moves code around to make the existing shadow code usable outside of StThemeNode)

Created attachment 166578 [details] [review]
[StThemeNode] Make shadow helper functions semi-public

Move shadow helper functions from st-theme-node-drawing to st-private
to make them available to widgets which want to add shadows to internal
actors.
Also add a new helper function for creating the shadow material from a
ClutterActor.

Created attachment 166580 [details] [review]
[StLabel] Implement text-shadow property

Use the existing shadow code to add a drop shadow to the underlying
ClutterText actor if the text-shadow property is specified.

This is obviously the main use case for the new text-shadow property - there are currently two other widgets which have internal ClutterText actors: StEntry and StButton. Not sure if we'd ever want to add shadows to the former, and the latter is a little tricky as the ClutterText is optional (and there exists the obvious workaround of setting the button's child to an StLabel instead of using the label property). So for now I skip implementing the property for those - we can still add it when the need arises.

Created attachment 167508 [details] [review]
[StThemeNode] Make shadow helper functions semi-public

Rebased on master.

Review of attachment 166580 [details] [review]:

The obvious problem here is that this doesn't catch the case where what the label paints changes without changing size. I think I'm OK with that limitation as long as we get that right when the text is changed through st_label_set_text() or the StLabel:text property. Other than that it looks good to me.

Review of attachment 167508 [details] [review]:

I only see one issue here, otherwise looks good.

::: src/st/st-private.c
@@ +529,3 @@
+      offscreen = cogl_offscreen_new_to_texture (buffer);
+
+      if (offscreen != COGL_INVALID_HANDLE)

It looks like if the offscreen is COGL_INVALID_HANDLE you return out a texture with undefined contents? (I'm not actually sure what the cogl_texture initialization guarantees are.) Would it be better to return COGL_INVALID_HANDLE?

The advantage of returning a real handle, of course, is that the caller doesn't have to handle it specially when caching the result. But we should make sure (by checking the COGL code and if necessary clearing the buffer) that the returned texture is transparent if we do that.

[ For GNOME Shell FBO usage is pretty much mandatory - it might be better *not* to handle it cleanly, so we can catch if problems show up - like I think COGL_PIXEL_FORMAT_ANY might cause problems on Radeons. (That would be a clutter/radeon driver bug if it did.) Still, I don't really buy that argument - if cogl_offscreen_new_to_texture() can fail, we should handle it. ]

(In reply to comment #6)
> It looks like if the offscreen is COGL_INVALID_HANDLE you return out a texture
> with undefined contents? (I'm not actually sure what the cogl_texture
> initialization guarantees are.) Would it be better to return
> COGL_INVALID_HANDLE?


It should - shadow_material is initialized to COGL_INVALID_HANDLE and only modified if creating the offscreen buffer succeeds.


> Still, I don't really buy that argument - if cogl_offscreen_new_to_texture()
> can fail, we should handle it.

Agreed - callers of _st_create_shadow_material() must be updated to check for COGL_INVALID_HANDLE.

Created attachment 168937 [details] [review]
[StThemeNode] Make shadow helper functions semi-public

Check arguments in _st_paint_shadow_with_opacity()

Created attachment 168938 [details] [review]
[StLabel] Implement text-shadow property

(In reply to comment #5)
> I think I'm OK with that limitation as long as we get that right when the
> text is changed through st_label_set_text() or the StLabel:text property.

Right.

Review of attachment 168938 [details] [review]:

Looks good

Review of attachment 168937 [details] [review]:

OK, I just misread the logic for creating the texture/offscreen pair. Looks fine to me.

Review of attachment 166577 [details] [review]:

Mostly OK, few details

::: src/st/st-theme-node.c
@@ +2586,3 @@
+
+  if (!result && inherit && node->parent_node)
+    result = st_theme_node_get_text_shadow (node->parent_node, inherit);

You should grab a reference to he shadow and set computed here (add reference counting to StShadow if necessary). Otherwise each time this function is called on a node that's inheriting the shadow, it will search through the properties and strcmp all the property names.

::: src/st/st-theme-node.h
@@ +169,3 @@
 StShadow      *st_theme_node_get_shadow       (StThemeNode *node);
+StShadow      *st_theme_node_get_text_shadow  (StThemeNode *node,
+                                               gboolean     inherit);

Hmm, having 'inherit' here is a bit odd to me. It's there for the generic things like get_length because we don't know the inheritance behavior. But text-shadow is supposed to be inherited:

 http://www.w3.org/TR/css3-text/#text-shadow

So it doesn't need to be passed in in the function signature.

Created attachment 169324 [details] [review]
[StShadow] Add reference counting

If a shadow property is inherited from a parent, multiple StThemeNodes
share a common StShadow. It would be possible to use st_shadow_copy()
for this purpose, but proper reference counting is nicer.

Created attachment 169325 [details] [review]
[StThemeNode] Add text-shadow property

Reorganize the existing code which parses the -st-shadow property
to allow parsing different shadow properties and add support for
the text-shadow property.

Created attachment 169326 [details] [review]
[StThemeNode] Make shadow helper functions semi-public

Reattaching to maintain patch order.

Created attachment 169327 [details] [review]
[StLabel] Implement text-shadow property

Adjust to changed parameters of st_theme_node_get_text_shadow().

Review of attachment 169324 [details] [review]:

Looks good to me. Don't particularly see a reason to use thread-safe atomic reference counting, but doesn't hurt either.

Review of attachment 169325 [details] [review]:

One trivial style commit, otherwise good to commit.

::: src/st/st-theme-node.c
@@ +2586,3 @@
+      if (result)
+        node->text_shadow = st_shadow_ref (result);
+      node->text_shadow_computed = TRUE;

This is duplicated between the branches. Probably just slightly cleaner to do:

if (!result && node->parent_node)
  {
     result = st_theme_node_get_text_shadow (node->parent_node);
     if (result)
        st_shadow_ref (result);
  }

 node->text_shadow = result;
 node->text_shadow_computed = TRUE;

 return result;

Attachment 169324 [details] pushed as e942444 - [StShadow] Add reference counting
Attachment 169325 [details] pushed as 29781b2 - [StThemeNode] Add text-shadow property
Attachment 169326 [details] pushed as ba1da9d - [StThemeNode] Make shadow helper functions semi-public
Attachment 169327 [details] pushed as a243ba6 - [StLabel] Implement text-shadow property