Created attachment 163400 [details] [review]
Add support for fallback icons

Changes icon resolution to take account for fallbacks, meaning that
you can ask for whatever-symbolic and get whatever if that is not
available.

Review of attachment 163400 [details] [review]:

This patch looks like it doesn't compile ("default_fallbacks" not "defaults_fallbacks")

The slightly larger question in my head is whether using g_themed_icon_new_with_default_fallbacks() is considered right, stylistically, or whether it would be better to to explicitly use new_from_names("<name>-symbolic", "<name>")

(sorry the added s was just a mistake)

How are you proposing to use new_from_names?
Do you think there should be a St.TextureCache.load_icon_names along with St.TextureCache.load_icon_name?
Or do you propose to always ask for the symbolic version (and only that)?
In first case, what should PopupImageMenuItem use?

I believe new_with_default_fallbacks solves the general problem, and solves it fine.

I'm going to admit to being confused here. I talked to Matthias Clasen (who added the symbolic icon support to GTK+, and he agreed that with my feeling that using  g_themed_icon_new_with_default_fallbacks() is bad style, and it was better to be specific.

I tried to research the issue a bit more, and followed a link from the GTK+ docs to http://www.freedesktop.org/wiki/SymbolicIcons, it has:

===
Therefore, there should be a mechanism for developers to request a symbolic variation of an icon, such that it will gracefully fall back to the non-symbolic equivalent if — whether intentionally or unintentionally — no symbolic variation has been provided. 

[...]

This text should be added to the Icon Naming Specification:

  Beneath the lowest level of specificity, there is a -symbolic namespace
  for all icon names. If a theme does not include a requested -symbolic icon,
  it falls back to the more generic version of the icon from the same theme.
===

And the IconNaming specification (http://standards.freedesktop.org/icon-naming-spec/) has:

===
The dash “-” character is used to separate levels of specificity in icon names, for all contexts other than MimeTypes. For instance, we use “input-mouse” as the generic item for all mouse devices, and we use “input-mouse-usb” for a USB mouse device. However, if the more specific item does not exist in the current theme, and does exist in a parent theme, the generic icon from the current theme is preferred, in order to keep consistent style.

The “Applications” context should not use this method of falling back to more generic icons. An application must either use a generic application icon name provided by this specification, or install an icon named the same as the executable for running the application. A generic desktop application included with the desktop suite, such as the calculator or terminal application, should use the generic names provided by this specification, as described above in the “Applications” context description. 
===

*That* implies, apparently, that we are supposed to always do fallbacks except when looking up anything but mime types and applications, and not allowed to do falbacks when looking up mime types and applications.

Note that the "Context" here is basically the subdirectory within the icon theme (as I understand it), and isn't part of the information provided to the lookup procedure for GTK+.

And then the "-symbolic" suffix is part of that generic fallback mechanism? Does that mean you can't have symbolic applications / MIME types?

I think the implication is that the shell needs two ways to load icons, one with fallbacks, and one without.

Separate issue - colorization; GTK+ 3 has:

GdkPixbuf *gtk_icon_info_load_symbolic (GtkIconInfo   *icon_info,
                                        GdkColor      *fg,
                                        GdkColor      *success_color,
                                        GdkColor      *warning_color,
                                        GdkColor      *error_color,
                                        gboolean      *was_symbolic,
                                        GError       **error);

Which does a complicated librsvg+CSS-style dance to recolor SVG icons. We should be using that eventually, but I don't think it's possible with the current API (it's going to need some sort of StImage/Icon actor so it can load a new CoglTexture when the CSS applied to it changes.) So you can ignore it for now.

OK, so, what does this all mean? I *think* what I'd like to see is a StIconType enumeration, for now:

typedef enum {
   ST_ICON_APPLICATION,
   ST_ICON_DOCUMENT,
   ST_ICON_SYMBOL
} StIconType;

And add that as an extra parameter to st_texture_cache_load_icon_name(). Behavior is that for APPLICATION and DOCUMENT, we just do as currently.
For SYMBOL we tack on -symbolic and g_themed_icon_new_with_default_fallbacks().

The reason for this approach is to take the policy out of the JS code - the JS code describes what sort of icon it is, and we figure out what that means to display it.

Created attachment 165930 [details] [review]
Add symbolic icons to TextureCache's load_icon_name()

This adds an argument to load_icon_name which needs to be one of the items in the enumeration Owen described above.  Right now, I actually don't know if this works.  My build of gnome shell doesn't seem to want to load any svg icons right now, which the symbolic icons are.  If anyone knows symbolic icons work on their system, please try this out and let me know what madness I've wrought!

(In reply to comment #5)
> Right now, I actually don't know if this
> works.  My build of gnome shell doesn't seem to want to load any svg icons
> right now

Edit ~/.jhbuildrc and change

    modules = [ 'gnome-shell' ]

to

    modules = [ 'gnome-shell-full' ]

and then "jhbuild build"

Comment on attachment 165930 [details] [review]
Add symbolic icons to TextureCache's load_icon_name()

>Icon's can be loaded as St.Icon.SYMBOL, DOCUMENT, or APPLICATION

you don't want an apostrophe there

also, I feel like SYMBOLIC would be better than SYMBOL...

>+++ b/js/ui/notificationDaemon.js
>-                return textureCache.load_icon_name(this._icon, size);
>+                return textureCache.load_icon_name(this._icon, St.IconType.APPLICATION, size);

hm... this is not necessarily an application icon. The app might be using a stock icon appropriate to the particular notification.

Based on Owen's quotes from the spec, it seems like we actually need an St.IconType.GENERIC, meaning to use g_themed_icon_new_with_default_fallbacks() but NOT append "-symbolic".

It's not obvious how we'd choose between APPLICATION, DOCUMENT, and GENERIC here though. I guess DOCUMENT icons have names of a particular form that can be recognized? For APPLICATION I guess we'd have to compare the icon name against the ShellAppInfo's icon name (and just assume they're not using some *other* application's name...).

>+            return textureCache.load_icon_name(stockIcon, St.IconType.APPLICATION, size);

this one would always be GENERIC

>+++ b/js/ui/popupMenu.js
>+            let img = St.TextureCache.get_default().load_icon_name(this._iconName, St.IconType.SYMBOL, this._size);

Not sure we want symbolic icons for all PopupMenus, but we can make that a property of the menu later on if we need to.

>+++ b/js/ui/telepathyClient.js
>+            iconBox.child = textureCache.load_icon_name('stock_person', St.IconType.SYMBOL, iconBox._size);

Not sure if we want SYMBOL or GENERIC here; other chat icons will be avatar images, which will not be symbolic, so having the fallback icon be non-symbolic might be more thematically appropriate?

>                                  const char        *name,
>+                                 gint              icon_type, 
>                                  gint               size)

fix alignment of "icon_type" (in both the .c and the .h)

(In reply to comment #7)
> Based on Owen's quotes from the spec, it seems like we actually need an
> St.IconType.GENERIC, meaning to use g_themed_icon_new_with_default_fallbacks()
> but NOT append "-symbolic".

(but you should probably wait for Owen to reply to this before implementing that :)

Created attachment 168083 [details] [review]
Add symbolic icons to TextureCache's load_icon_name()

git bz seems to have forgotten my comment for that patch...

It's just adding the GENERIC type (pending owen's approval - if not it's simple enough to change back) and uses it in the two cases mentioned.  It also fixes the argument alignment in the function definition in st-texture-cache.[ch].

(In reply to comment #7)
> (From update of attachment 165930 [details] [review])
> >Icon's can be loaded as St.Icon.SYMBOL, DOCUMENT, or APPLICATION
> 
> you don't want an apostrophe there
> 
> also, I feel like SYMBOLIC would be better than SYMBOL...
> 
> >+++ b/js/ui/notificationDaemon.js
> >-                return textureCache.load_icon_name(this._icon, size);
> >+                return textureCache.load_icon_name(this._icon, St.IconType.APPLICATION, size);
> 
> hm... this is not necessarily an application icon. The app might be using a
> stock icon appropriate to the particular notification.
> 
> Based on Owen's quotes from the spec, it seems like we actually need an
> St.IconType.GENERIC, meaning to use g_themed_icon_new_with_default_fallbacks()
> but NOT append "-symbolic".
> 
> It's not obvious how we'd choose between APPLICATION, DOCUMENT, and GENERIC
> here though. I guess DOCUMENT icons have names of a particular form that can be
> recognized? For APPLICATION I guess we'd have to compare the icon name against
> the ShellAppInfo's icon name (and just assume they're not using some *other*
> application's name...).

This misses the point of my suggestion. My API was meant to make the choice something that someone writing the API could actually know with certainty without understanding the whim of the day for icon behavior.

If you have to choose between SYMBOLIC and GENERIC, you are screwed as a coder, unless the mockup you are following happens to show a stock icon that clearly could have been symbolic drawn in a non-symbolic fashion.

And my understanding is also basically that we *always* want to draw icons with a symbolic variant as symbolic. When I questioned the designers by this they didn't come up with any exceptions to this rule.

If we have an icon name where there's no context as to whether it's and application/document or not, then I think you have to use SYMBOLIC and hope that the fallback code doesn't cause a false match in the icon database. (The docs probably should mention the fact you should use SYMBOLIC if you have no way of knowing.)

If we *do* have places in the API where we need non-symbolic icons, then I'd much rather the enum option was called "NONSYMBOLIC" than "GENERIC", since "symbolic" is the general case as far a I understand it.

(In reply to comment #11)
> If you have to choose between SYMBOLIC and GENERIC, you are screwed as a coder,
> unless the mockup you are following happens to show a stock icon that clearly
> could have been symbolic drawn in a non-symbolic fashion.

Mockups are never sufficiently detailed that you can just implement them without any further communication with the designers.

> And my understanding is also basically that we *always* want to draw icons with
> a symbolic variant as symbolic. When I questioned the designers by this they
> didn't come up with any exceptions to this rule.

http://www.freedesktop.org/wiki/SymbolicIcons explicitly says otherwise, repeatedly.

Unless you just mean *the shell* always wants symbolic icons, but still, if we're merging with Mx someday, non-shell users will want non-symbolic icons in some contexts. (And non-symbolic icons, as I understand it, are still the GENERIC version, and SYMBOLIC the special case.)

I certainly was thinking about the shell, and asking the designers about that, rather than all possible uses on the desktop. (For all purposes on the desktop, we need vastly better docs than that freedesktop page which is "might" this and "might" that. Going through that, the only clear example I can find of non-symbolic icons is that _some_ toolbars might use non-symbolic icons.)

But there's also a general point here that APIs need to be one or the other:

 - Obvious from the docs what you use in a given situation - this is an application icon, so I choose APPLICATION.

 - Obvious from the docs what the end result is given an input - I want a fullcolor icon, so I choose FULLCOLOR

I was trying for the first, but as soon as you add GENERIC, you end up with an unhappy compromise - an API mess. If the axis here is symbolic icons versus fullcolor icons, we should have exactly two choices:

 SYMBOLIC
 FULLCOLOR

(With presumably being documented that it's best-effort, and if you specify one you might get the other if that's all that is available.)

Now if we go this way, we are actually tackling a different problem then the problem I was trying to get at. The problem I was trying to get at was not symbolic vs. fullcolor but rather "fallbacks" vs. "no fallbacks."

The icon naming specification (as I read it)

 - Requires the chop off -suffixes behavior for most types of icons
 - Forbids it for application and mime-type icons

Now, we could actually go with flags rather than an enumeration to cover both:

 FULLCOLOR | NO_FALLBACKS

But I think that puts a lot of stress on the API user to read docs and understand when NO_FALLBACKS should be used. And beyond that apparently in the case you are trying to handle here you don't actually know. You just have a name provided by the notification source?

So, maybe we need to just ignore the whole fallbacks thing in the spec and always do fallbacks, and hope that doesn't cause bad results.

We aren't following the freedesktop wiki page necessarily.  Our guidelines are evolving here:
http://live.gnome.org/GnomeShell/Design/Whiteboards/SymbolicIcons
http://live.gnome.org/GnomeShell/Design/Whiteboards/SymbolicIcons/UseGuidelines

If something isn't addressed there then we should add it.  Let us know.

Created attachment 170422 [details] [review]
gnome-shell.modules: add gnome-icon-theme, gnome-icon-theme-symbolic

FIXME: something needs to run gtk_update_icon_cache after building
these...

Created attachment 170423 [details] [review]
Add symbolic icons to TextureCache's load_icon_name()

This patch divides icons into SYMBOLIC, COLOR, APPLICATION, and
DOCUMENT, with the former two doing fallback and the latter two not.
(There is no distinction between APPLICATION and DOCUMENT actually.)

load_gicon (implicitly) uses the APPLICATION/DOCUMENT behavior. The
other uses in the overview (eg device icons) currently use COLOR, but
could be changed.

The message tray mostly uses COLOR, because the icons that we don't
create ourselves--app icons and avatar images--will be color, and so
it makes sense that if a notification requests an image by name it
should get a color icon as well. Jon says that notifications from
"the system" should have symbolic icons though, so we'll need to add
something for that at some point.

Looking Glass and the status area/user menu use SYMBOLIC, not that it
matters since we don't currently have symbolic versions of any of the
icons they use.

(In reply to comment #11)
> And my understanding is also basically that we *always* want to draw icons with
> a symbolic variant as symbolic. When I questioned the designers by this they
> didn't come up with any exceptions to this rule.

Jon agreed (or at least, failed to disagree) that since most avatar icons will be colorful, and most notification source icons will be app icons (which will be full color), that the fallback avatar icon and default source icon should also be full color, to avoid implying a distinction that is not actually there.

(In reply to comment #13)
> So, maybe we need to just ignore the whole fallbacks thing in the spec and
> always do fallbacks, and hope that doesn't cause bad results.

I left it in (behind an abstraction layer that doesn't require the user to read the spec, and with a note saying "use COLOR if you don't know what kind of icon it is"), but I'm willing to take it out and always do fallbacks. Really, it seems like the API is the wrong place to put this; the icon theme itself should be specifying "this icon is a fallback for these other icons".

Review of attachment 170423 [details] [review]:

One typo I noticed (in the class of things that a gobject-introspection-js-lint could catch...) Otherwise, looks fine to me.

::: js/ui/messageTray.js
@@ +346,3 @@
         if (Gtk.IconTheme.get_default().has_icon(id)) {
             button.add_style_class_name('notification-icon-button');
+            button.child = St.TextureCache.get_default().load_icon_name(id, St.IconType.SYMBOL, BUTTON_ICON_SIZE);

SYMBOLIC not SYMBOL

::: src/st/st-texture-cache.c
@@ +1133,3 @@
+ * StIconType:
+ * @ST_ICON_SYMBOLIC: a symbolic (ie, mostly monochrome) icon
+ * @ST_ICON_COLOR: a full-color icon

FULLCOLOR makes more sense to me, since there is color in symbolic icons too. But don't feel strongly.

@@ +1135,3 @@
+ * @ST_ICON_COLOR: a full-color icon
+ * @ST_ICON_APPLICATION: a full-color icon, which is expected
+ *   to be an application icon

As described earlier, this feels messy an non-orthogonal, especially since there are cases where we don't know. But I'm OK with it too, on the theory that St doesn't have to have the world's most beautiful API. 

We discussed whether the fallbacks-or-no should be pushed down to the code inside GTK+, since that knows the categories. But I'm not sure that works since it knows categories *after* it has looked up icons. If it encounters gnome-color-picker, looks that up, fails, looks up gnome-color, then maybe gnome-color is a toolbar icon, while gnome-color-picker was an app icon that shouldn't have gotten fallbacks.

Review of attachment 170422 [details] [review]:

OK with the addition, but:

> FIXME: something needs to run gtk_update_icon_cache after building
> these...

Does this FIXME need to be fixed before commit?
Why don't these modules run gtk_update_icon_cache in their install?
If the FIXME doesn't need to be fixed, needs a real commit message that explains the why

(In reply to comment #19)
> Does this FIXME need to be fixed before commit?
> Why don't these modules run gtk_update_icon_cache in their install?

Yes and I don't know, respectively. Filed bug 630465.

The gnome-icon-theme bug is now fixed, so I pushed this. Requires a
re-build (or at least a buildone of gnome-icon-theme and
gnome-icon-theme-symbolic) to have any effect. Currently I think the
a11y menu is the only icon we use that we actually have a symbolic
version of.