Created attachment 180606 [details] [review]
base-icon: Enforce the requested icon size

Mainly due to StTextureCache's way of handling fallback icons, an
implementation of BaseIcon.createIcon() may return an icon smaller
than the requested size. Given that BaseIcon is not used for isolated
elements, but rather for groups of related items (App view, Dash,
Search Results, ...), having some elements end up with the wrong
size is more annoying than having some elements end up ugly due to
scaling, so explicitly enforce the requested icon size.

This worries me if an icon in an icon theme is a few pixels smaller than the nominal size in the theme and then gets scaled up and fuzzed. Are we sure that's not going to happen?

(In reply to comment #2)
> This worries me if an icon in an icon theme is a few pixels smaller than the
> nominal size in the theme and then gets scaled up and fuzzed. Are we sure
> that's not going to happen?

Well no one really creates an icon that is lets say 46x46 instead of 48x48 .... it never made sense.

(In reply to comment #2)
> This worries me if an icon in an icon theme is a few pixels smaller than the
> nominal size in the theme and then gets scaled up and fuzzed. Are we sure
> that's not going to happen?

Yesterday I had a problem with edit-find-symbolic, which showed up as a gray rectangle. Apparently this was because the icon's dimensions were slightly off (sth like 16.08 x 16.08) and gtk-update-icon-cache refused to add it to the cache. I have no idea if that's an oddity of symbolic icons or if icon sizes are always enforced (I've quickly skimmed through the code but didn't find anything obvious which suggests this behavior) - I'm gonna check with Jakub later.

Also in the case of the dash, being a few pixels smaller is enough to break Fitts'ability for an icon.

(In reply to comment #4)
> Apparently this was because the icon's dimensions were slightly off
> (sth like 16.08 x 16.08)

You may want to mention this on bug 634023

(In reply to comment #4)
>
> Also in the case of the dash, being a few pixels smaller is enough to break
> Fitts'ability for an icon.
>

I have been running this for a while and even though the icons of the affected apps looks ugly (blurry due to being scaled up), the actual experience seems better.

The alternative would be to make the surrounding box not depend on the icon size and just center the icon in it (like it is done for alt-tab).

Created attachment 181264 [details] [review]
baseIcon: Do not make the box depend on the actual icon size

The icon returned by createIcon() might actually be smaller than
the requested icon size (for instance StTextureCache does this
for fallback icons), so make sure that atleast the surrounding
box has the requested size.

This way we are consistent with what the we do in the appSwitcher
and compared to the alternative of just scaling the icons up they do
not end up being blurry.

Review of attachment 181264 [details] [review]:

I tried something like that before, it breaks the animations of icon size changes.

I discussed this bug with Jon and Jakub yesterday, and both preferred scaling over padding. So, to push the bug forward - does the patch need further review?


(In reply to comment #5)
> (In reply to comment #4)
> > Apparently this was because the icon's dimensions were slightly off
> > (sth like 16.08 x 16.08)
> 
> You may want to mention this on bug 634023

Sorry that I didn't, but I was actually wrong - apparently the symbolic icon theme is developed in a single SVG which is split by a script to create the final icons. Due to an error in the "master" SVG, the region to create the icon in question was wrong, so it didn't have anything to do with gtk-update-icon-theme or the size of the final icon.

Review of attachment 180606 [details] [review]:

Patch looks good, given that the designers want this solution we should just go ahead and do it.

Attachment 180606 [details] pushed as a78e757 - base-icon: Enforce the requested icon size