GNOME Bugzilla – Bug 637169
GtkNumerableIcon, icon with number overlay
Last modified: 2011-01-05 21:34:37 UTC
It's useful in some cases to have an icon with a number ribbon, representing a counter. See the following pages for some possible use cases: http://live.gnome.org/GnomeShell/Design/Guidelines/MessageTray/Weather http://live.gnome.org/GnomeShell/Design/Guidelines/MessageTray/Email Attached you can find a working implementation that draws everything with cairo, and fetches theming properties from the style context of the renderer widget. I had to modify GtkWidgetPath to take non-GtkWidget object types for it to work, and have a little API break.
Created attachment 176352 [details] [review] numerableicon: initial import GtkNumerableIcon is a GEmblemedIcon subclass with a fixed numeric emblem, drawn by GTK+ and themable with the icon properties.
Created attachment 176353 [details] [review] widgetpath: allow GTypes non-derived from GTK_TYPE_WIDGET This makes things like GtkCellRenderer or GtkNumerableIcon more easily themeable.
Created attachment 176354 [details] [review] numerableicon: use CSS properties and GtkStyleContext for theming Instead of regular properties. This adds a _new_with_style_context() variant, which makes the icon optionally themable.
One improvement I'd like to make is to use the background-image property to set the ribbon shape.
Two suggestions: - the code uses "%u" to format the number; this should be made translatable using C_() so translators can use "%Iu" to get i18n'd digits (btw, why unsigned instead of signed?) - I still think this should allow setting any text (and named GtkLabelledIcon or something). E.g. this could be used to set roman numerals, or short letter combinations. You mentioned on IRC that it probably looks bad with more than about 2 characters of text, but the docs could just mention that constraint (and/or the code just not show more than that). (Keeping the number property as a shortcut to set text from printf("%u") is fine, of course).
Review of attachment 176353 [details] [review]: Looks good, please commit
Review of attachment 176354 [details] [review]: I agree with Christian that it would be useful to allow short strings (1-2 chars) instead of numbers. ::: gtk/gtknumerableicon.c @@ +79,3 @@ + g_emblemed_icon_clear_emblems (G_EMBLEMED_ICON (self)); + return; + } Special cases like this should probably be documented. @@ +192,3 @@ + { + gtk_widget_path_append_type (path, GTK_TYPE_NUMERABLE_ICON); + gtk_style_context_set_path (style, path); Here you are permanently modifying the passed-in style. I think you probably need to save/restore (though I don't know if save/restore does also save the path, so you may have to do that manually. @@ +238,3 @@ + } + + g_clear_object (&self->priv->style); Always using the newest apis, thats good. :-)
Comment on attachment 176353 [details] [review] widgetpath: allow GTypes non-derived from GTK_TYPE_WIDGET Attachment 176353 [details] pushed as b792a31 - widgetpath: allow GTypes non-derived from GTK_TYPE_WIDGET
Created attachment 176601 [details] [review] numerableicon: add GtkNumerableIcon GtkNumerableIcon is a GEmblemedIcon subclass with a fixed numeric emblem, drawn by GTK+ and themable with the icon properties.
Created attachment 176606 [details] [review] numerableicon: add GtkNumerableIcon GtkNumerableIcon is a GEmblemedIcon subclass with a fixed numeric emblem, drawn by GTK+ and themable with the icon properties. ---- Wrong patch attached, sorry for the spam.
New version of the patch (squashed). Improvements over the last iteration: - fetch values for "background-image" to allow both actual images and GTK+ gradients as background for the ribbon. - if we have a style context, use the "font" CSS property to tweak font appearance. - added gtk_numerable_icon_set_label(), which takes a string. We truncate the string after two characters if it's longer than that. - turned the unsigned int into an int, and documented the behavior when passing zero. - added context for translators, and made the format string translatable I'm not sure I like to change the name of this icon to GtkLabeledIcon, because it doesn't actually let you specify a real label with all of its text properties (like GtkLabel does), just a two-chars string (that's likely a substitute or an alternative representation for a number). Actual labeled icons, which would look like [1], would be matter for a completely different class I think. [1] http://trac.adium.im/attachment/ticket/13088/Adium-Dock.jpg
Review of attachment 176606 [details] [review]: ::: docs/reference/gtk/gtk3-sections.txt @@ +2279,3 @@ +gtk_numerable_icon_get_count +gtk_numerable_icon_get_style_context +gtk_numerable_icon_set_count need to add the label setter and getter here ::: gtk/gtk.symbols @@ +1583,3 @@ +gtk_numerable_icon_new_with_style_context +gtk_numerable_icon_set_count +gtk_numerable_icon_set_style_context need to add the label getter and setter here ::: gtk/gtknumerableicon.c @@ +390,3 @@ + + buf[4] = '\0'; + retval = g_utf8_strncpy (buf, label, 2); I think buf needs to be longer to be on the safe side here. The docs of g_utf8_strncpy are unfortunately silent on the issue... @@ +422,3 @@ + + self->priv->count = count; + self->priv->rendered_string = g_strdup_printf (C_("Number format", "%d"), count); This probably needs a translators comment in addition to the context to be useful. @@ +621,3 @@ + * Note that this is meant for displaying short labels, such as roman numbers, + * or single letters, and strings longer than two characters will be truncated. + * The docs need to say something about the priority between label and count. @@ +661,3 @@ + * + * Sets the currently displayed value of @self to @count. + * Note that setting a count of zero removes the emblem. The docs need to say something about the priority between label and count, and it should be mentioned that the value will be clamped at 99.
Feel free to land this after addressing these comments.
Created attachment 176952 [details] [review] icon-theme: support GtkNumerable icon emblem sizing
Created attachment 176953 [details] [review] numerableicon: squashed patch GtkNumerableIcon is a GEmblemedIcon subclass with a fixed numeric emblem, drawn by GTK+ and themable with the icon properties.
New version of the patch - the biggest difference is the introduction of two new properties, "background-icon" (GIcon) and "background-icon-name" (string), which allow to specify an icon name (or a GIcon) as background for the rendered emblem. This allows the use of named icons defined by the theme, which can vary their appearance according to the rendered size (the separate icon theme patch is needed for it to work properly). Feedback welcome on this approach.
Some more mockups by Jimmac on how this would be used http://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/app-switcher.png http://git.gnome.org/browse/gnome-shell-design/plain/mockups/static/notifications-summary.png
Review of attachment 176953 [details] [review]: Is this patch missing some gtkicontheme.c part to make using numerable icons as emblems actually work ? ::: gtk/gtknumerableicon.c @@ +876,3 @@ + * is %NULL, @self will go back using style information or default theming + * for its background image. + * Should document the priorities between gicon and icon_name here. ::: gtk/gtknumerableicon.h @@ +54,3 @@ + + /* padding for future class expansion */ + gpointer padding[16]; This seems a tad excessive, even GtkWidget only has 8 slots of padding atm. I think we generally stick to 8 now. @@ +81,3 @@ +void gtk_numerable_icon_set_background_icon_name (GtkNumerableIcon *self, + const gchar *icon_name); +const gchar * gtk_numerable_icon_get_background_icon_name (GtkNumerableIcon *self); Should line up the arguments here, to make things look neat. @@ +85,3 @@ +/* private */ +void _gtk_numerable_icon_set_background_icon_size (GtkNumerableIcon *self, + gint icon_size); I've started to keep private apis out of the public headers now, and moved to separate gtkfooprivate.h headers.
(In reply to comment #18) > Review of attachment 176953 [details] [review]: > > Is this patch missing some gtkicontheme.c part to make using numerable icons as > emblems actually work ? Yes, the gtkicontheme.c part of the patch was attached in comment #14. I fixed the other comments in the upcoming new patch, thanks for the review.
Created attachment 177122 [details] [review] numerableicon: squashed patch GtkNumerableIcon is a GEmblemedIcon subclass with a fixed numeric emblem, drawn by GTK+ and themable with the icon properties.
Created attachment 177123 [details] [review] icon-theme: support GtkNumerable icon emblem sizing
Review of attachment 177123 [details] [review]: What I am missing here is a GTK_IS_NUMERABLE_ICON case in gtk_icon_theme_lookup_by_gicon
Review of attachment 177122 [details] [review]: Almost there. Still missing: include the docs section in gtk-docs.sgml, and add SECTION:gtknumerableicon docs in gtknumerableicon.c ::: gtk/gtknumerableicon.c @@ +747,3 @@ + * automatically be reset to zero before rendering the label, i.e. the last + * method called between #gtk_numerable_icon_set_label and + * #gtk_numerable_icon_set_count has always priority. References to function in gtk-doc look like this: gtk_numerable_icon_set_label() @@ +871,3 @@ + * gtk_numerable_icon_set_background_gicon: + * @self: a #GtkNumerableIcon + * @icon: (allow-none): a #GIcon Should say @icon: (allow-none): a #GIcon, or %NULL here (ie repeat the annotation in human-digestible form ::: gtk/gtknumerableicon.h @@ +76,3 @@ + +void gtk_numerable_icon_set_background_gicon (GtkNumerableIcon *self, + GIcon *icon); Indentation is not quite up to GTK+ standards yet, but I can fix that up later.
+static gchar * +sanitize_label (const gchar *label) +{ + gchar buf[128]; + gchar *retval; + + if (!g_utf8_validate (label, -1, NULL)) + return NULL; Why validate? All strings in gtk are UTF-8, we don't generally put too much extra validation in. + if (g_utf8_strlen (label, -1) <= 2) + return g_strdup (label); + + buf[127] = '\0'; + retval = g_utf8_strncpy (buf, label, 2); + + return g_strdup (retval); +} This doesn't look right. You may be correct in limiting the shown text to 2 glyphs, but that does not correspond to just 2 utf-8 codepoints. The code above will not work correctly, since it e.g. will cut off combining accent characters. Also, since you explicitly mention roman numerals in the docs, making it impossible to actually put 3 (III) or 7 (VII) on the emblem (which in glyph size is probably not longer than most 2 letter sequences), i.e. not even supportging all numbers from 1 to 10, seems suboptimal :)
Actually I forgot about U+2060..U+216B, so the last point can be worked around by using these real roman numbers instead of faking them with ascii :) The point about combining characters still stands, however.
Committed after a round of cleanups and changes.