GNOME Bugzilla – Bug 633865
Add St.Icon and use for named icons
Last modified: 2010-11-13 03:30:02 UTC
This patch series imports MxIcon as StIcon, adapts it to our purposes, then uses it for named icons. The main effect is that we now properly pick up CSS colors for symbolic icons.. There are some directions that this patch series goes that I haven't fully explored - that probably need to be left for later. - Make St.Icon use a CoglTexture rather than a ClutterActor child; this would allow us not to queue a relayout when the icon finishes loading but just a repaint. - Make St.Icon handle icons other than named icons or add St.Image for those. This would allow more comprehensive use of CSS for sizing, and prevent unbalanced code where we either create a St.Icon or call St.TextureCache.load_...
Created attachment 173718 [details] [review] Import MxIcon as StIcon
Created attachment 173719 [details] [review] StIcon: Remove content-image capability The ability to set a "content image" on an icon relies on the ability to have custom theme properties of a "border image" (9-slice) type. We don't have this, and the capability of a bordered image specified by the theme can be achieved more naturally with standard CSS facilities.
Created attachment 173720 [details] [review] Move StIconType to st-types.h from st-texture-cache.h StIconType will be used by a new StIcon class, so move it to the header file of common enumerations. Including st-types.h which had the ST single-include check revealed that st-texture-cache.h didn't have that check and seeral places were including that directly. Fix that up.
Created attachment 173721 [details] [review] Port StIcon from MX framework to ST framework Make StIcon compile and work in ST. Changes: * ::icon-type and st_icon_set_icon_type are added to allow specifying SYMBOLIC/FULLCOLOR for an icon. * Ability to set the icon name from the theme is removed; it wouldn't easily fit into our framework and two levels of abstraction between code and image doesn't seem that useful. * size CSS property is renamed from x-st-icon-size to icon-size to correspond to what we are doing elsewhere. * CSS and property based icon sizing are cleanly layered - if you set the icon-size property, the CSS size is ignored. * Add a simple JS test of StIcon.
Created attachment 173722 [details] [review] StIcon: Always request a square icon size We don't want the layout to change when we say, change from battery-full to battery-full-charging, so we should request a square based on the icon size unconditionally and not try to adapt to the size of the texture we loaded. This also means that our layout is independent of the loaded texure which, if we switch away from using a ClutterActor child will allow us not queue a relayout when the icon finishes loading.
Created attachment 173723 [details] [review] StIcon: Center the icon rather than scaling it up Scaling up icons from the loaded size to a larger size is uniformly ugly and results in a long series of "fuzzy icon" bugs. It's better to just load at the specified size and center. (Centering can be overridden by packing not-fill in the parent container.)
Created attachment 173724 [details] [review] Add StIconColors object, compute in StThemeNode A new StIconColors object is used to efficiently track the colors we need to colorize a symbolic icon. st_theme_node_compute_icon_colors() is added to compute the StIconColors for a theme node. (Refcounting of StIconColors means that we'll typically share the colors object of the parent node.)
Created attachment 173725 [details] [review] StTextureCache: support loading a named icons with colors from a theme node Add st_texture_cache_load_icon_name_for_theme() which, when loading a symbolic icon, gets a #StIconColors from the theme node and uses that to colorize the icon.
Created attachment 173726 [details] [review] Add st_widget_peek_theme_node() Sometimes it's useful to get the theme node if there is one and do nothing and wait for the ::style-changed signal if there is no theme node. Add st_widget_peek_theme_node() that just gets the current theme node if available. The caller must handle a %NULL return.
Created attachment 173727 [details] [review] StIcon: pass in the StThemeNode to get colorized symbolic icons Use st_texture_cache_load_icon_name_for_theme() so that we get the right colors for symbolic icons. The code refactoring to achieve this also avoids constantly starting a new icon load each time we set a property on initialization ... the icon is loaded only after we have a #StThemeNode assigned.
Created attachment 173728 [details] [review] Use St.Icon for named icons Switch from St.TextureCache.load_named_icon() to using St.Icon for named icons. Along with the advantage of getting colorization right for symbolic icons, this allows moving some icon sizes into the CSS.
Comment on attachment 173718 [details] [review] Import MxIcon as StIcon everything obviously wrong with this patch seems to be fixed by later patches...
Comment on attachment 173719 [details] [review] StIcon: Remove content-image capability >@@ -514,9 +454,7 @@ st_icon_set_icon_name (StIcon *icon, > priv = icon->priv; > > /* Check if there's no change */ >- if ((!priv->icon_name && !icon_name) || >- (priv->icon_name && icon_name && >- g_str_equal (priv->icon_name, icon_name))) >+ if (g_strcmp0 (priv->icon_name, icon_name)) this does not belong with this patch, although if you're going to squash it all together at the end it doesn't matter much. except for the part where you forgot the "!", though apparently that gets fixed later on...
Comment on attachment 173720 [details] [review] Move StIconType to st-types.h from st-texture-cache.h >have that check and seeral places were including that directly. *several
Comment on attachment 173721 [details] [review] Port StIcon from MX framework to ST framework >Make StIcon compile and work in ST. "ST"? Really? I always say "St"... > * Written by: Thomas Wood <thomas.wood@intel.com>, > * Chris Lord <chris@linux.intel.com> >- * >+ * Owen Taylor <otaylor@redhat.com> IMHO, "Written by" sections are an antipattern, because they just get out of date. >- if (g_strcmp0 (priv->icon_name, icon_name)) >+ if (g_strcmp0 (priv->icon_name, icon_name) == 0) and there's that fix >+st_icon_set_icon_type (StIcon *icon, >+ StIconType icon_type) need an extra space in there between types and arguments >+ > gint > st_icon_get_icon_size (StIcon *icon) missing documentation
Comment on attachment 173722 [details] [review] StIcon: Always request a square icon size >We don't want the layout to change when we say, change from >battery-full to battery-full-charging The icons are all *supposed* to be exactly 16x16, although apparently they aren't (bug 634023). But anyway, I don't think you'd be able to detect that, and with the battery icons, the real problem is that battery-full is centered in its 16x16 box and battery-full-charging is flush left (bug 633991). The patch itself is fine though.
Comment on attachment 173724 [details] [review] Add StIconColors object, compute in StThemeNode >+ * st-icon-colors.c: Colors for colorzizing a symbolic icon "colorzizing" sounds like something you'd see an infomercial about... >+ * Atomically increments the reference count of @shadow by one. s/shadow/colors/ >+ * if you were planning to change colors in the result; s/;/./ >+GType >+st_icon_colors_get_type (void) use G_DEFINE_BOXED_TYPE
Comment on attachment 173725 [details] [review] StTextureCache: support loading a named icons with colors from a theme node >Subject: [PATCH] StTextureCache: support loading a named icons with colors from a theme node "A named iconS" ? >Add st_texture_cache_load_icon_name_for_theme() which, when loading a >symbolic icon, gets a #StIconColors from the theme node and uses that >to colorize the icon. Shouldn't we just add an StThemeNode arg to st_texture_cache_load_icon_name()? Is there any situation where we *wouldn't* want to use theme colors?
Comment on attachment 173728 [details] [review] Use St.Icon for named icons >+.popup-menu-icon { >+ icon-size: 16px; >+} >+.system-status-icon { >+ icon-size: 24px; >+} you might as well just fix these while you're here.
Created attachment 173880 [details] [review] test case fixes put a border around the "16px icon in 48px icon widget" test, to verify that the icon is being centered correctly add spacing and fix alignment for general prettiness
(In reply to comment #15) > (From update of attachment 173721 [details] [review]) > >Make StIcon compile and work in ST. > > "ST"? Really? I always say "St"... I read it "ess-tee", but a grep through our ChangeLog reveals 'St' being far more common than ST, and I'm happy to accommodate. > > * Written by: Thomas Wood <thomas.wood@intel.com>, > > * Chris Lord <chris@linux.intel.com> > >- * > >+ * Owen Taylor <otaylor@redhat.com> > > IMHO, "Written by" sections are an antipattern, because they just get out of > date. Removed (along with all the the Written by sections, bug 634550) > >+st_icon_set_icon_type (StIcon *icon, > >+ StIconType icon_type) > > need an extra space in there between types and arguments Thought about that when writing it, but never have been clear in my mind whether you need a full blank column or lack of collision is good enough. It comes up surprisingly rarely. Blank column restored. > >+ > > gint > > st_icon_get_icon_size (StIcon *icon) > > missing documentation Guess if I add it for set_icon_size() to clarify a point I'm on the hook for the getter too ;-). Added.
(In reply to comment #18) > >Add st_texture_cache_load_icon_name_for_theme() which, when loading a > >symbolic icon, gets a #StIconColors from the theme node and uses that > >to colorize the icon. > > Shouldn't we just add an StThemeNode arg to st_texture_cache_load_icon_name()? > Is there any situation where we *wouldn't* want to use theme colors? The reason for not doing this is that that forces either a gigantic katamari patch, or updating a lot of JS code that is going to get removed anyways. Or making intermediate versions that are a bisection hazard. No perfect solution. I think I'll add a cleanup patch to the end of the patch-set that reconsolidates the two functions. Which isn't perfect either...
Attachment 173718 [details] pushed as 839492f - Import MxIcon as StIcon Attachment 173719 [details] pushed as a9a8b1e - StIcon: Remove content-image capability Attachment 173720 [details] pushed as aed6375 - Move StIconType to st-types.h from st-texture-cache.h Attachment 173722 [details] pushed as 3540023 - StIcon: Always request a square icon size Attachment 173723 [details] pushed as 3ca86b2 - StIcon: Center the icon rather than scaling it up Attachment 173724 [details] pushed as 04da2a6 - Add StIconColors object, compute in StThemeNode Attachment 173726 [details] pushed as 8d6ab6f - Add st_widget_peek_theme_node() Attachment 173727 [details] pushed as af7ba00 - StIcon: pass in the StThemeNode to get colorized symbolic icons
Comment on attachment 173880 [details] [review] test case fixes Looks good; I pushed so I can close this bug