Created attachment 292308 [details] [review]
gtkmodelmenuitem: force icon scaling

GtkIconTheme doesn't scale icons beyond the size specified in the theme
anymore. This can result in arbitrarily large menu items when a theme
only provides large icons.

Force icons to be always the same height as the text in the menu item,
and react to font size changes.

Review of attachment 292308 [details] [review]:

::: gtk/gtkmodelmenuitem.c
@@ +83,3 @@
+      height = (pango_font_metrics_get_ascent (metrics) +
+                pango_font_metrics_get_descent (metrics)) / PANGO_SCALE;
+      gtk_image_set_pixel_size (GTK_IMAGE (item->image), height);

I don't like the approach here.

You request a ICON_SIZE_MENU icon but then you scale it according to the font size.

This can be bad for at least two reasons:

 (a) you could end up scaling 16x16 up to 18x18 and making the icon distorted/blurry

 (b) even worse, if the font size is really large, you could end up picking the wrong-sized icon to start with


If you want to take this patch then it needs to be made more clever...

In particular for small sizes, the only right approach here is to fix the theme to provide the required sizes. scaling things with a target size of 16 or 20 pixels is never going to look good (unless you happen to start from an integer multiple).

(In reply to comment #3)
> In particular for small sizes, the only right approach here is to fix the theme
> to provide the required sizes. scaling things with a target size of 16 or 20
> pixels is never going to look good (unless you happen to start from an integer
> multiple).

Of course, but I don't think we should have unusable menus because of a buggy theme.

any concrete examples of problematic cases ?

(In reply to comment #5)
> any concrete examples of problematic cases ?

I've noticed it first with unity's icon theme, which misses some icons in the right sizes.

However, this is also an issue with application icons. For example, gvim installs an svg icon with width=height=550pt into /usr/share/pixmaps, which makes the GVim menu item in nautilus' open-with menu take the full size of my screen.  (That menu doesn't use GtkModelMenuItem yet, but unity's sound and messaging menus use it and have application icons in them as well.)

get a better vim :-)

$ rpm -ql vim-X11 | grep icons
/usr/share/icons/hicolor/16x16/apps/gvim.png
/usr/share/icons/hicolor/32x32/apps/gvim.png
/usr/share/icons/hicolor/48x48/apps/gvim.png
/usr/share/icons/hicolor/64x64/apps/gvim.png

but I accept the point that we can't show a 550 pixel 'icon' in the menu (or the dialog). I was 100% sure that I had added a pixel-size to the app chooser dialog for this very reason, but I can't find that commit now.

Created attachment 292335 [details] [review]
gtkimagemenuitem: force icon scaling

The same for GtkImageMenuItem. Nautilus still uses that for its open with
dialog.

Review of attachment 292335 [details] [review]:

::: gtk/deprecated/gtkimagemenuitem.c
@@ +1241,3 @@
+      height = (pango_font_metrics_get_ascent (metrics) +
+                pango_font_metrics_get_descent (metrics)) / PANGO_SCALE;
+      gtk_image_set_pixel_size (GTK_IMAGE (item->priv->image), height);

This looks really bad - very likely this will end up with a pixel size of 17 or 19 or something similarly horrible that will cause every single icon to be scaled off, even if it is a perfectly fine 16x16 or 20x20.

@Matthias, seems like your icons are a fedora changeset
http://pkgs.fedoraproject.org/cgit/vim.git/tree/vim.spec#n1423

would be nice to get that in upstream vim so every distro could have "a better vim" ;-)

(In reply to comment #10)

> would be nice to get that in upstream vim so every distro could have "a better
> vim" ;-)

Oh, I agree. Sadly, experience shows that pushing better app icons upstream is a suprisingly difficult challenge. Not sure if that is the case here, or what...

(In reply to comment #9)
> [...]
> This looks really bad - very likely this will end up with a pixel size of 17 or
> 19 or something similarly horrible that will cause every single icon to be
> scaled off, even if it is a perfectly fine 16x16 or 20x20.

I agree that always scaling to the font size can lead to ugly artifacts at some font sizes. (I'd argue having those is better than the current situation, though.)

I think we should always scale icons that are larger than the text. A blurry icon is preferable to menu items with inconsistent heights.

However, we could allow icons to be slightly smaller than the text if we can load them without scaling. Not sure what a good value for "slightly smaller" might be, maybe 2px?

I very much disagree that we should always scale icons to the size of the text; blurry icons suck and we should avoid them at all costs.
I think the most flexible approach would be to load icons only at sizes compatible with the theme that are near text size, and only scale icons (but still to that exact size) when the theme does not have one for that specific size. That's similar to what the Shell does e.g. with the dash. This also has the nice property that you never end up with icons larger than what the theme gives you.
A much simpler approach is to just use GTK_ICON_SIZE_MENU without any dynamic scaling at all.

(In reply to comment #13)
> I very much disagree that we should always scale icons to the size of the text;
> blurry icons suck and we should avoid them at all costs.

I'm arguing that the cost of having inconsistently sized icons (or even menu items) is too large.

> I think the most flexible approach would be to load icons only at sizes
> compatible with the theme that are near text size, and only scale icons (but
> still to that exact size) when the theme does not have one for that specific
> size. That's similar to what the Shell does e.g. with the dash. This also has
> the nice property that you never end up with icons larger than what the theme
> gives you.

If we do it on a per-icon basis, we can still get icons of different sizes in the same menu. That's why I suggested basing it on text size. But I agree, choosing a size that is an exact match but a bit smaller is better than scaling.

> A much simpler approach is to just use GTK_ICON_SIZE_MENU without any dynamic
> scaling at all.

Well, the original problem is that gtk doesn't scale icons when the theme doesn't supply them in GTK_ICON_SIZE_MENU, but shows very large (or very small)  icons instead. Obviously these are bugs in the themes and application icons, but I think such bugs shouldn't result in a different layout of the UI.

> Obviously these are bugs in the themes and application icons,
> but I think such bugs shouldn't result in a different layout of the UI.

You are treating an obvious bug (different layout) for a more subtle one (miserable, scaled icons) - which one do you think is more likely to get fixed, and which one will linger forever ?

(In reply to comment #14)
> If we do it on a per-icon basis, we can still get icons of different sizes in
> the same menu. That's why I suggested basing it on text size. But I agree,
> choosing a size that is an exact match but a bit smaller is better than
> scaling.

I disagree with this statement; assuming the text size doesn't change between menu items, I think it's possible to write an implementation that always chooses icons of the same size, regardless of whether they get scaled or not.
What I'm thinking is probably best illustrated by an example: with a text size of say 30px you would choose always icons of 24px (a size that is typically offered by the theme) and only scale (to 24px) those icons that are not available at that size natively.

I think a force-scale implementation that picks pixel sizes other than 16, 20 or 24 is unacceptable, since it will force every icon to be fuzzy.

Created attachment 292547 [details] [review]
Menu items: force loading 16x16 icons

After writing quite a complex patch trying to choose the "best" icon, I
realized that this is probably not worth making the code more complex for.

Attaching a new patch which simply scales icons to 16x16, effectively restoring
the old behavior for menu items.

Review of attachment 292547 [details] [review]:

ok

Attachment 292547 [details] pushed as b44df22 - Menu items: force loading 16x16 icons

*** Bug 742962 has been marked as a duplicate of this bug. ***

Please have a look at: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=773135
Seems like it is caused by this bug's fix.