GNOME Bugzilla – Bug 671656
Empty icons for applications without icons
Last modified: 2012-03-17 05:47:55 UTC
- Install xterm - In the dock, alt-tab dialog and Applications icon view, it will be displayed as a transparent empty icon - If you run it, in the top panel you will get the (right) generic app icon instead. The same icon should be used everywhere else an icon is displayed. I am pretty sure this regressed recently.
Created attachment 209278 [details] [review] st-texture-cache: Don't implicitly return a fallback icon In the case that we don't have an icon corresponding to the gicon, it's more than likely that the code calling load_gicon will know more about what it wants as a fallback than the texture-cache itself. In fact - we had a whole lot of dead code that would try to fall back, but never did because we always returned a valid actor. This was causing certain applications with invalid icons to not get the fallback icon because an icon couldn't be found. Fix up the one place where we don't have an explicit fallback icon codepath, and then stop doing what we were doing.
Review of attachment 209278 [details] [review]: Looks good to me, if you agree with the below comment. ::: js/ui/iconGrid.js @@ +127,3 @@ + + let icon = this.createIcon(this.iconSize); + if (icon == null) Couldn't this cause the same problem described in this bug somewhere else in the future? Maybe it's fine though - this code should never get run in practice, since BaseIcon subclasses are required to implement createIcon() and those usually have their fallbacks specified already (so this is really a "let's put an actor here just so that we don't crash if somebody assume there is one" code path).
Review of attachment 209278 [details] [review]: Besides StIcon, load_gicon is used by placeDisplay.js to load the mount and volume icons. If some of those are missing, what should we put in instead?
(In reply to comment #3) > Review of attachment 209278 [details] [review]: > > Besides StIcon, load_gicon is used by placeDisplay.js to load the mount and > volume icons. If some of those are missing, what should we put in instead? A generic drive-harddisk icon maybe?
Created attachment 209962 [details] [review] st-texture-cache: Don't implicitly return a fallback icon In the case that we don't have an icon corresponding to the gicon, it's more than likely that the code calling load_gicon will know more about what it wants as a fallback than the texture-cache itself. In fact - we had a whole lot of dead code that would try to fall back, but never did because we always returned a valid actor. This was causing certain applications with invalid icons to not get the fallback icon because an icon couldn't be found. Fix up the one place where we don't have an explicit fallback icon codepath, and then stop doing what we were doing. Looking into it, it seems that the icons in placeDisplay.js already had explicit fallbacks, so this isn't needed either.
Created attachment 209963 [details] [review] st-texture-cache: Remove unused St.IconType.APPLICATION/DOCUMENT No reason to have special handling for fallbacks that we can do (and do do) in the JS. And for the hell of it...
Review of attachment 209962 [details] [review]: Looking at the code in StTextureCache, the ST_ICON_SYMBOLIC case in st_texture_cache_load_icon_name() assumes load_gicon_with_colors() will return a ClutterActor, since it will cast its return value to it. I think a better approach would be to move this fallback code there instead of removing it.
Review of attachment 209963 [details] [review]: Makes sense.
Created attachment 209967 [details] [review] st-texture-cache: Don't implicitly return a fallback icon In the case that we don't have an icon corresponding to the gicon, it's more than likely that the code calling load_gicon will know more about what it wants as a fallback than the texture-cache itself. In fact - we had a whole lot of dead code that would try to fall back, but never did because we always returned a valid actor. This was causing certain applications with invalid icons to not get the fallback icon because an icon couldn't be found. Fix up the one place where we don't have an explicit fallback icon codepath, and then stop doing what we were doing. Like this?
Comment on attachment 209963 [details] [review] st-texture-cache: Remove unused St.IconType.APPLICATION/DOCUMENT Attachment 209963 [details] pushed as 21e8097 - st-texture-cache: Remove unused St.IconType.APPLICATION/DOCUMENT
Review of attachment 209967 [details] [review]: ::: src/st/st-texture-cache.c @@ +1281,3 @@ + } + + themed = g_themed_icon_new ("image-missing"); Since we don't have a symbolic version of image-missing (and I don't know if it's even a good idea to have it), I was actually thinking to just move this code into the symbolic case and leave everything else unchanged. if (texture == NULL) { /* gah, the icon doesn't exist. Return a blank texture that will never load */ texture = CLUTTER_ACTOR (create_default_texture (cache)); clutter_actor_set_size (texture, size, size); } return CLUTTER_ACTOR (texture);
Created attachment 209978 [details] [review] st-texture-cache: Don't implicitly return a fallback icon In the case that we don't have an icon corresponding to the gicon, it's more than likely that the code calling load_gicon will know more about what it wants as a fallback than the texture-cache itself. In fact - we had a whole lot of dead code that would try to fall back, but never did because we always returned a valid actor. This was causing certain applications with invalid icons to not get the fallback icon because an icon couldn't be found. Fix up the one place where we don't have an explicit fallback icon codepath, and then stop doing what we were doing. Like this?
Review of attachment 209978 [details] [review]: Yes, this looks good, thanks!
Attachment 209978 [details] pushed as 0aad74a - st-texture-cache: Don't implicitly return a fallback icon