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 606257 - updated look for running app indicator
updated look for running app indicator
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-01-06 20:19 UTC by William Jon McCann
Modified: 2010-01-12 22:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[StWidget] Change handling of gradients (8.24 KB, patch)
2010-01-07 23:47 UTC, Florian Müllner
accepted-commit_now Details | Review
Update running app indicator to match latest mockup (7.32 KB, patch)
2010-01-07 23:47 UTC, Florian Müllner
none Details | Review
Update running app indicator to match latest mockup (7.49 KB, patch)
2010-01-08 15:08 UTC, Florian Müllner
accepted-commit_now Details | Review
[StWidget] Change handling of gradients (12.45 KB, patch)
2010-01-10 03:50 UTC, Florian Müllner
reviewed Details | Review
Update running app indicator to match latest mockup (13.38 KB, patch)
2010-01-10 03:56 UTC, Florian Müllner
committed Details | Review
[StWidget] Change handling of gradients (14.37 KB, patch)
2010-01-11 21:37 UTC, Florian Müllner
committed Details | Review

Description William Jon McCann 2010-01-06 20:19:16 UTC
Jeremy and I have been struggling with the running app indicator (aka glow) for a long time.  We never really found a way to make the blue glow work well.  It never really worked at all for the multiple instance cases.

Here's our latest crack at it:
http://www.gnome.org/~mccann/screenshots/clips/20100106201448/shell-mockup-overview-test.png

I think it works a lot better.
Comment 1 Florian Müllner 2010-01-07 23:47:00 UTC
Created attachment 151006 [details] [review]
[StWidget] Change handling of gradients

Some changes to the way we handle CSS gradients:

 * draw without padding, thus interpreting gradients as part of the
   background rather than as content

 * clip to (rounded) border area

 * draw the border along the gradient instead of trying to align the
   gradient layer with the background/border layer
Comment 2 Florian Müllner 2010-01-07 23:47:04 UTC
Created attachment 151007 [details] [review]
Update running app indicator to match latest mockup
Comment 3 Florian Müllner 2010-01-08 15:08:02 UTC
Created attachment 151034 [details] [review]
Update running app indicator to match latest mockup
Comment 4 Colin Walters 2010-01-08 20:29:21 UTC
Review of attachment 151006 [details] [review]:

Looks good to me, just one small comment; you might want to add a note about the gradient implementation around line 779 to the comment (how it acts as a background, etc).

In particular you should note that gradients are clipped by the border-radius, but only to the top one (alternatively you could change the patch to support the per-corner radii, up to you).
Comment 5 Colin Walters 2010-01-08 20:36:07 UTC
Review of attachment 151034 [details] [review]:

Looks good.  One thing I notice now is that we're always changing the pseudo class even if the actor isn't visible, but that should be OK since it looks like st-widget.c skips expensive computation if we're not mapped.
Comment 6 Colin Walters 2010-01-08 20:36:07 UTC
Review of attachment 151034 [details] [review]:

Looks good.  One thing I notice now is that we're always changing the pseudo class even if the actor isn't visible, but that should be OK since it looks like st-widget.c skips expensive computation if we're not mapped.
Comment 7 Florian Müllner 2010-01-10 03:50:26 UTC
Created attachment 151111 [details] [review]
[StWidget] Change handling of gradients

Updated patch according to the review and two additional changes:

 - add support for per-corner radii on gradients
 - update the CSS3 background + borders comment
 - improve the border drawing (thanks for kindly overlooking those
   glitches in the review!)
 - use the border_image actor instead of the background_image one for
   gradients

The last point is up to debate (I'm not religious about the change), but IMHO it helps emphasizing the background charactor of gradients (see e.g. st_widget_paint()), as well as avoiding at least one nasty corner case in st_widget_real_style_changed().

We loose the ability to have both border-image and background-gradient, but as the gradient was drawn over the image anyway, I don't see much of a loss.
Comment 8 Florian Müllner 2010-01-10 03:56:03 UTC
Created attachment 151112 [details] [review]
Update running app indicator to match latest mockup

Updated the patch to remove app-well-glow.png, as well as only changing pseudo class when necessary.
Comment 9 Colin Walters 2010-01-11 19:06:32 UTC
Review of attachment 151111 [details] [review]:

::: src/st/st-widget.c
@@ +644,3 @@
                                      end->blue / 255.,
+  if (round_border)
+

I'd expect "border > 0 && round_border".

@@ +914,3 @@
+      priv->draw_border_internal = FALSE;
+    {
+  else if (priv->bg_gradient_type != ST_GRADIENT_NONE)

The use of GObject data here seems unusual; also we discussed moving all of the drawing inside StWidget itself, and if we do so we'd probably want the variables in the private structure.
Comment 10 Colin Walters 2010-01-11 19:07:45 UTC
Review of attachment 151112 [details] [review]:

No major changes since last one, I assume.
Comment 11 Florian Müllner 2010-01-11 21:37:17 UTC
Created attachment 151194 [details] [review]
[StWidget] Change handling of gradients

(In reply to comment #9)
> Review of attachment 151111 [details] [review]:
> 
> ::: src/st/st-widget.c
> @@ +644,3 @@
>                                       end->blue / 255.,
> +  if (round_border)
> +
> 
> I'd expect "border > 0 && round_border".

Not really - correct me if I'm wrong, but if round_border is true, we should
clip the background, no matter whether we draw a border later or not.


> @@ +914,3 @@
> +      priv->draw_border_internal = FALSE;
> +    {
> +  else if (priv->bg_gradient_type != ST_GRADIENT_NONE)
> 
> The use of GObject data here seems unusual; also we discussed moving all of the
> drawing inside StWidget itself, and if we do so we'd probably want the
> variables in the private structure.

OK, done.
Comment 12 Colin Walters 2010-01-11 22:40:48 UTC
(In reply to comment #11)

> > I'd expect "border > 0 && round_border".
> 
> Not really - correct me if I'm wrong, but if round_border is true, we should
> clip the background, no matter whether we draw a border later or not.

Hmm, but with your patch how are you clipping the background, if you've switched to using the border_image?
Comment 13 Colin Walters 2010-01-11 22:47:18 UTC
Review of attachment 151194 [details] [review]:

Makes sense.
Comment 14 Florian Müllner 2010-01-12 15:05:35 UTC
Comment on attachment 151112 [details] [review]
Update running app indicator to match latest mockup

Attachment 151112 [details] pushed as 33b3d05 - Update running app indicator to match latest mockup
Comment 15 Florian Müllner 2010-01-12 15:12:36 UTC
Attachment 151194 [details] pushed as 7486c09 - Change handling of gradients