After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 624384 - text-shadow support
text-shadow support
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
2.29.x
Other All
: Normal enhancement
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-07-14 20:17 UTC by Jakub Steiner
Modified: 2010-09-02 20:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[StThemeNode] Add text-shadow property (8.55 KB, patch)
2010-07-26 13:02 UTC, Florian Müllner
needs-work Details | Review
[StThemeNode] Make shadow helper functions semi-public (19.78 KB, patch)
2010-07-26 13:03 UTC, Florian Müllner
none Details | Review
[StLabel] Implement text-shadow property (3.72 KB, patch)
2010-07-26 13:12 UTC, Florian Müllner
needs-work Details | Review
[StThemeNode] Make shadow helper functions semi-public (19.79 KB, patch)
2010-08-10 13:13 UTC, Florian Müllner
needs-work Details | Review
[StThemeNode] Make shadow helper functions semi-public (19.89 KB, patch)
2010-08-28 09:47 UTC, Florian Müllner
accepted-commit_now Details | Review
[StLabel] Implement text-shadow property (4.09 KB, patch)
2010-08-28 09:48 UTC, Florian Müllner
accepted-commit_now Details | Review
[StShadow] Add reference counting (4.20 KB, patch)
2010-09-02 07:20 UTC, Florian Müllner
committed Details | Review
[StThemeNode] Add text-shadow property (8.35 KB, patch)
2010-09-02 07:21 UTC, Florian Müllner
committed Details | Review
[StThemeNode] Make shadow helper functions semi-public (19.89 KB, patch)
2010-09-02 07:21 UTC, Florian Müllner
committed Details | Review
[StLabel] Implement text-shadow property (4.09 KB, patch)
2010-09-02 07:23 UTC, Florian Müllner
committed Details | Review

Description Jakub Steiner 2010-07-14 20:17:20 UTC
A very powerful tool for a graphics designer is being able to use text-shadow on text labels. Please let me have my pony!

http://www.w3.org/TR/css3-text/#text-shadow
Comment 1 Florian Müllner 2010-07-26 13:02:58 UTC
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)
Comment 2 Florian Müllner 2010-07-26 13:03:13 UTC
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.
Comment 3 Florian Müllner 2010-07-26 13:12:50 UTC
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.
Comment 4 Florian Müllner 2010-08-10 13:13:47 UTC
Created attachment 167508 [details] [review]
[StThemeNode] Make shadow helper functions semi-public

Rebased on master.
Comment 5 Owen Taylor 2010-08-27 17:26:29 UTC
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.
Comment 6 Owen Taylor 2010-08-27 17:33:39 UTC
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. ]
Comment 7 Florian Müllner 2010-08-28 08:58:56 UTC
(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.
Comment 8 Florian Müllner 2010-08-28 09:47:42 UTC
Created attachment 168937 [details] [review]
[StThemeNode] Make shadow helper functions semi-public

Check arguments in _st_paint_shadow_with_opacity()
Comment 9 Florian Müllner 2010-08-28 09:48:32 UTC
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.
Comment 10 Owen Taylor 2010-08-30 17:22:26 UTC
Review of attachment 168938 [details] [review]:

Looks good
Comment 11 Owen Taylor 2010-08-30 17:25:51 UTC
Review of attachment 168937 [details] [review]:

OK, I just misread the logic for creating the texture/offscreen pair. Looks fine to me.
Comment 12 Owen Taylor 2010-08-30 18:38:51 UTC
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.
Comment 13 Florian Müllner 2010-09-02 07:20:25 UTC
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.
Comment 14 Florian Müllner 2010-09-02 07:21:00 UTC
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.
Comment 15 Florian Müllner 2010-09-02 07:21:24 UTC
Created attachment 169326 [details] [review]
[StThemeNode] Make shadow helper functions semi-public

Reattaching to maintain patch order.
Comment 16 Florian Müllner 2010-09-02 07:23:18 UTC
Created attachment 169327 [details] [review]
[StLabel] Implement text-shadow property

Adjust to changed parameters of st_theme_node_get_text_shadow().
Comment 17 Owen Taylor 2010-09-02 18:47:39 UTC
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.
Comment 18 Owen Taylor 2010-09-02 18:51:03 UTC
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;
Comment 19 Florian Müllner 2010-09-02 20:07:09 UTC
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