GNOME Bugzilla – Bug 606257
updated look for running app indicator
Last modified: 2010-01-12 22:29:52 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.
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
Created attachment 151007 [details] [review] Update running app indicator to match latest mockup
Created attachment 151034 [details] [review] Update running app indicator to match latest mockup
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).
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.
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.
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.
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.
Review of attachment 151112 [details] [review]: No major changes since last one, I assume.
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.
(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?
Review of attachment 151194 [details] [review]: Makes sense.
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
Attachment 151194 [details] pushed as 7486c09 - Change handling of gradients