GNOME Bugzilla – Bug 614711
support 'symbolic' themed icons
Last modified: 2011-02-22 22:25:43 UTC
Created attachment 157787 [details] [review] patch See http://live.gnome.org/GnomeShell/Design/Whiteboards/SymbolicIcons The main point here is that these icons are supposed to be rendered with the foreground/background from the surrounding context. Here is a proposal for an implementation that relies on CSS to change the color of svg elements. It adds a single new API, GdkPixbuf * gtk_icon_info_load_symbolic (GtkIconInfo *icon_info, GdkColor *fg, GdkColor *bg, GError **error); which is like gtk_icon_info_load_icon(), but takes a pair of colors to use.
Depending on whether the shell continues to use status icons or changes to shell-drawn icons, we also need a way to obtain the 'shell colors' for use in the status icon.
Created attachment 157851 [details] [review] second patch This patch make tray icons look for a _NET_SYSTEM_TRAY_COLORS property containing the fg and bg colors to use as a six cardinals. You can play with this by using xprop -id <manager window id> -f _NET_SYSTEM_TRAY_COLORS 32cccccc -set _NET_SYSTEM_TRAY_COLORS 0xffff,0xffff,0xffff,0x0,0x0,0x0 where <manager window id> is the ID of the manager window for your status icon (I believe the patch still has the debug printf to spit it out).
Created attachment 157852 [details] [review] third patch Here is a patch that adds support to notification-area-applet for setting the _NET_SYSTEM_TRAY_COLORS property. For fun, it hardwires the fg color to magenta. The patch is against Fedora gnome-panel packages and may not apply cleanly against master.
I'm not sure the tray-specific bits will end up being useful for the shell, because we still aren't totally sure whether the tray in the panel is going to use the systray protocol or not. (It's possible that we'll end up deciding that the symbolic panel icons should all be in-process.)
(In reply to comment #0) > Created an attachment (id=157787) [details] [review] > patch > > See http://live.gnome.org/GnomeShell/Design/Whiteboards/SymbolicIcons > The main point here is that these icons are supposed to be rendered with the > foreground/background from the surrounding context. FWIW, this will be great for Sugar. We are now applying a regex to the svgs to set the colors, then rendering with rsvg and cairo to a pixbuf.
It seems like, in general, two colors wouldn't always be enough. For instance you could want a highlight color or a warning color to be consistent across all of the icons. Would it not be better to have these be properties set in the GTK Theme's gtkrc file rather than as X properties? That way the symbolic icon would match the theme in use, which seems the overall goal.
If you look at the shell, the style of the panel in which the icons should integrate is quite different from the style of regular GTK+ apps. That is why it is necessary to transfer the style information from the tray to the clients (as long as you are using client-side status icons, at least). And X properties are the way to do that.
*** Bug 616180 has been marked as a duplicate of this bug. ***
As I mentioned, you definitely want to pass GtkStyle and GtkState along, so that you can theme the icons properly. This would allow changing the display based on state, as well as passing along symbolic colours from the GTK theme to the icon (eg. the warning colour for the batter icon).
We cannot pass the GtkStyle to the systray, and we also cannot generate one, as the symbolic colours are private to the GtkStyle. So we'd need to serialise all the 10 (5 bg, 5 fg) symbolic colours actually in use in GTK+ for the systray, and the other uses of gtk_icon_info_load_symbolic(). My thinking is: - Passing a GHashTable of strings/GdkColors to gtk_icon_info_load_symbolic() - Adding a function to create that GHashTable from a GtkStyle - Modifying both implementations of _NET_SYSTEM_TRAY_COLORS to pass/read the 12 required colours
Created attachment 159595 [details] [review] Support 'symbolic' themed icons Add gtk_icon_info_load_symbolic() to load symbolic icons, and theme their background/foreground colours to match the colours used in the theme. Adds the gtk_icon_info_load_symbolic() function.
Some notes: - the CSS needs tweaking to override the colour in the loaded SVG file - we probably don't need a "bg" colour - we might need a "positive" colour (eg. a green)
Created attachment 159607 [details] [review] Support 'symbolic' themed icons Add gtk_icon_info_load_symbolic() to load symbolic icons, and theme their background/foreground colours to match the colours used in the theme. Adds the gtk_icon_info_load_symbolic() function.
Created attachment 159617 [details] [review] Support 'symbolic' themed icons Add gtk_icon_info_load_symbolic() to load symbolic icons, and theme their background/foreground colours to match the colours used in the theme. Adds the gtk_icon_info_load_symbolic() function.
Created attachment 159681 [details] test file SVG file to test theming
Review of attachment 159617 [details] [review]: Would be good to also have a testcase for the cell renderer somewhere in gtk-demo. And the new function needs to be added to gtk.symbols and gtk-sections.txt. ::: gtk/gtkcellrendererpixbuf.c @@ +549,3 @@ + success_ptr = NULL; + else + success_ptr = &success_color; Did we want to do some kind of fallback here, if the symbolic colors are not defined ? Like we do in gtk_info_bar_update_colors()... @@ +551,3 @@ + success_ptr = &success_color; + cellpixbuf->pixbuf = gtk_icon_info_load_symbolic (info, + &style->fg[GTK_STATE_NORMAL], didn't we want to go to tracking widget state here ? ::: gtk/gtkicontheme.c @@ +3095,3 @@ + * @success_color: a #GdkColor representing the warning color of the icon + * @warning_color: a #GdkColor representing the warning color of the icon + * @error_color: a #GdkColor representing the error color of the icon Need to say "or %NULL to use the default foo color". And probably add a may-be-null annotation (whatever the syntax for that is). @@ +3100,3 @@ + * success, warning and error colors provided. + * + * This allows loading symbolic icons that will match the system theme. There should ideally be some link here to a symbolic icon spec, or somesuch. Also, would be good to make it clear that it is fine to call this function even if the icon is not symbolic. In this case, it will function just like gtk_icon_info_load() @@ +3141,3 @@ + GdkColor warning_default_color = { 0, 0xf500, 0x7900, 0x3e00 }; + css_warning = gdk_color_to_css (&warning_default_color); + } Oh, I see, the fallback is here. Nevermind the other comment then. ::: gtk/gtkimage.c @@ +1738,3 @@ + + image->data.name.pixbuf = gtk_icon_info_load_symbolic (info, + &style->fg[GTK_STATE_NORMAL], Here too: did we want to track widget state ?
(In reply to comment #16) > Review of attachment 159617 [details] [review]: > > Would be good to also have a testcase for the cell renderer somewhere in > gtk-demo. Done. > And the new function needs to be added to gtk.symbols and gtk-sections.txt. Done. <snip> > @@ +551,3 @@ > + success_ptr = &success_color; > + cellpixbuf->pixbuf = gtk_icon_info_load_symbolic (info, > + > &style->fg[GTK_STATE_NORMAL], > > didn't we want to go to tracking widget state here ? Not here, but in the render function. Done. > ::: gtk/gtkicontheme.c > @@ +3095,3 @@ > + * @success_color: a #GdkColor representing the warning color of the icon > + * @warning_color: a #GdkColor representing the warning color of the icon > + * @error_color: a #GdkColor representing the error color of the icon > > Need to say "or %NULL to use the default foo color". And probably add a > may-be-null annotation (whatever the syntax for that is). It's "allow-none". Done. > @@ +3100,3 @@ > + * success, warning and error colors provided. > + * > + * This allows loading symbolic icons that will match the system theme. > > There should ideally be some link here to a symbolic icon spec, or somesuch. Done. > Also, would be good to make it clear that it is fine to call this function even > if the icon is not symbolic. In this case, it will function just like > gtk_icon_info_load() Done. > ::: gtk/gtkimage.c > @@ +1738,3 @@ > + > + image->data.name.pixbuf = gtk_icon_info_load_symbolic (info, > + > &style->fg[GTK_STATE_NORMAL], > > Here too: did we want to track widget state ? No, the GtkImage already follows the state and transforms the image accordingly. It would apply 2 levels of changes if we did that.
Created attachment 159693 [details] [review] Support 'symbolic' themed icons Add gtk_icon_info_load_symbolic() to load symbolic icons, and theme their background/foreground colours to match the colours used in the theme. Adds the gtk_icon_info_load_symbolic() function, explicit support in GtkImage and GtkCellRendererPixbuf, and test cases for those 2 widgets. With help from Bastien Nocera <hadess@hadess.net>
Review of attachment 159693 [details] [review]: ::: demos/gtk-demo/list_store.c @@ +227,3 @@ + COLUMN_SENSITIVE, + "follow-state", + COLUMN_SENSITIVE, I think the demo would actually be nicer/clearer if we just g_object_set (renderer, "follow-state", TRUE.. and get rid of the sensitive column. ::: gtk/gtkimage.c @@ +1741,3 @@ + image->data.name.pixbuf = gtk_icon_info_load_symbolic (info, + &style->fg[GTK_STATE_NORMAL], + success_ptr, Replace GTK_STATE_NORMAL by gtk_widget_get_state (GTK_WIDGET (image)) here @@ +1821,3 @@ + + image->data.gicon.pixbuf = gtk_icon_info_load_symbolic (info, + &style->fg[GTK_STATE_NORMAL], And here too...
Review of attachment 159693 [details] [review]: Please commit with these changes
(In reply to comment #19) > Review of attachment 159693 [details] [review]: > > ::: demos/gtk-demo/list_store.c > @@ +227,3 @@ > + COLUMN_SENSITIVE, > + "follow-state", > + COLUMN_SENSITIVE, > > I think the demo would actually be nicer/clearer if we just g_object_set > (renderer, "follow-state", TRUE.. > and get rid of the sensitive column. OK. The sensitive column is actually used for one of the test cases. > ::: gtk/gtkimage.c > @@ +1741,3 @@ > + image->data.name.pixbuf = gtk_icon_info_load_symbolic (info, > + > &style->fg[GTK_STATE_NORMAL], > + success_ptr, > > Replace GTK_STATE_NORMAL by gtk_widget_get_state (GTK_WIDGET (image)) here > > @@ +1821,3 @@ > + > + image->data.gicon.pixbuf = gtk_icon_info_load_symbolic (info, > + > &style->fg[GTK_STATE_NORMAL], > > And here too... As I mentioned, in both those cases, the GtkImage will modify the pixbuf itself, and we won't re-render the pixbuf. Otherwise we'll end up changing the pixbuf twice (once through state, once through GtkImage itself).
> No, the GtkImage already follows the state and transforms the image > accordingly. It would apply 2 levels of changes if we did that. There is some truth to that. So, what we should do is return from ensure_pixbuf_for_icon_name whether the state was taken into account or not, and set needs_state_transform accordingly.
- Added was_symbolic argument to gtk_icon_info_load_symbolic() so that we know whether the colours were applied to it - Fix introspection keywords use - Add missing error declaration in the API docs - Made GtkImage remember what the last state that was rendered, and whether it was a symbolic icon, so as to avoid unnecessary re-renders when it's not the case - Use the normal state when rendering insensitive, otherwise the theme colours don't end up re-colorised (when insensitive, the fg would change, but the theme colours wouldn't, so dark gray became light gray, but sharp red stayed sharp red, see attached screenshot) - Removed the follow-state column attribute in the list store demo
Created attachment 159776 [details] insensitive weird looking
Created attachment 159777 [details] [review] Support 'symbolic' themed icons Add gtk_icon_info_load_symbolic() to load symbolic icons, and theme their background/foreground colours to match the colours used in the theme. Adds the gtk_icon_info_load_symbolic() function, explicit support in GtkImage and GtkCellRendererPixbuf, and test cases for those 2 widgets. With help from Bastien Nocera <hadess@hadess.net>
Comment on attachment 159777 [details] [review] Support 'symbolic' themed icons Committed as 6b939d57c762f58a9f8d529024b7171ff70b6986 on master
Created attachment 159846 [details] [review] panel patch updated to more symbolic colors
Comment on attachment 157851 [details] [review] second patch I've committed a variant of this patch that deals with the extra symbolic colors.
So, can this be closed now?
(In reply to comment #30) > So, can this be closed now? Missing the panel support for the status icons.
Review of attachment 159846 [details] [review]: ::: gnome-panel-2.30.0/applets/notification_area/main.c @@ +287,3 @@ + error.red = 0xffff; error.green = 0; error.blue = 0; + warning.red = 0xffff; warning.green = 0xffff; warning.blue = 0; + success.red = 0; success.green = 0xffff; success.blue = 0; Are those colors? Or masks? If colors, why are we hardcoding them? And not using tango colors? @@ +686,3 @@ + GdkDisplay *display; + Atom atom; + gulong data[6]; 6? And later... @@ +706,3 @@ + data[9] = manager->success.red; + data[10] = manager->success.green; + data[11] = manager->success.blue; ... we use 12 slots here. @@ +762,3 @@ na_tray_manager_set_visual_property (manager); na_tray_manager_set_padding_property (manager); + na_tray_manager_set_colors_property (manager); na_tray_manager_set_padding_property() is not in git.
> Are those colors? Or masks? > If colors, why are we hardcoding them? And not using tango colors? They are colors, and the patch is missing the part where you pull the right colors for your theme out of the hat and set them. 6? And later... Ugh, indeed. Originally it was just fg/bg... na_tray_manager_set_padding_property() is not in git. Thats from a patch in another bug...
Created attachment 179731 [details] [review] tray: add _NET_SYSTEM_TRAY_COLORS support, for symbolic icons Here's an updated version of the na-tray-manager part of the patch, fixing the 6-vs-12 bug and updating for current gtk. I've tested this with gnome-shell, but not gnome-panel. See also bug 641059, and note that (for reasons I didn't bother to look into), the excess g_object_thaw_notify()s there actually cause the trayicon to crash, so you don't want to apply this until that fix is in.
Just tried in gnome-panel, with the rest from attachment 159846 [details] [review], after rebuilding gtk+ from master. With "fg.red = fg.green = fg.blue = 0xffff;", I get a blue icon. Not sure who is to blame, though :-)
*** Bug 583272 has been marked as a duplicate of this bug. ***
Well, Owen pointed out a problem with this patch in bug 641060, although that should just result in the property not getting set, not in it being set to the wrong value... While debugging, I had it printf the xid of manager->window so that I could use xprop from the command line to see if the property was getting set to the right value, so I could figure out which bugs were tray-side and which were icon-side.
Created attachment 179824 [details] [review] na: remove unneeded includes gnome-panel and gnome-shell differed as to whether these files should include g18n.h or g18n-lib.h, but there are no translated strings in them anyway, so it doesn't matter.
Created attachment 179825 [details] [review] na: warn if trying to set tray properties before realizing Previously, na_tray_manager_set_orientation_property() and na_tray_manager_set_visual_property() would silently no-op if you called them before realizing the tray. Make them g_return_if_fail() instead.
Created attachment 179826 [details] [review] na: add support for _NET_SYSTEM_TRAY_COLORS This adds support for setting _NET_SYSTEM_TRAY_COLORS value, but does not include a call to set it to match the theme/panel colors.
with that patch (and different default color values) and latest gtk3, I got the right colors
Comment on attachment 179824 [details] [review] na: remove unneeded includes Committed in gnome-panel, but not in gnome-shell. (not sure I setting the status to committed is right, but well...)
Comment on attachment 179825 [details] [review] na: warn if trying to set tray properties before realizing Committed in gnome-panel, but not in gnome-shell. (not sure I setting the status to committed is right, but well...)
Comment on attachment 179826 [details] [review] na: add support for _NET_SYSTEM_TRAY_COLORS Committed in gnome-panel, but not in gnome-shell. (not sure I setting the status to committed is right, but well...)
Thanks! I tested things again, and now it works. Maybe I did something wrong earlier, or maybe that was because my gtk3 wasn't fully up-to-date. I'm not sure if we need additional review for gnome-shell, or if the patches can go in there too. The only missing piece now, in gnome-panel at least, is calling na_tray_set_colors() with information from the theme... Shouldn't be too hard, though.
gnome-shell has its own version of the patches, in bug 641060 (which also includes syncing up a few other miscellaneous differences between the two copies)
Created attachment 180307 [details] [review] na: copy a fix from gnome-shell gdk_window_set_background_pattern()'s behavior has changed; use cairo drawing functions instead.
Comment on attachment 180307 [details] [review] na: copy a fix from gnome-shell Thanks!
We've added the code to fetch colors from the theme now.