Created attachment 304822 [details] [review]
icontheme: don't modify symbolic SVG dimensions when recoloring

When recoloring symbolic SVG, do not modify the original width and
height of the passed-in file; the function later will scale the image
through gdk_pixbuf_new_from_stream_at_scale(), but we should still
use the original size to create the proxy SVG, or the image will
possibly be doubly-resized or blurry.

Review of attachment 304822 [details] [review]:

Whats the point here, are you using this for non-square icons ?

Yes we are.

https://github.com/endlessm/eos-desktop/blob/master/data/theme/feedback-symbolic.svg

I don't think that is something we should support

What isn't? Non-square icons? Why not?

If you look through gtkicontheme.c you'll find that there is a pretty pervasive assumption that icons just have a single size, not width/height. You might be able to make some things work with non-square items, but it will break in places, and I don't want to be on the hook for fixing all those places.

(In reply to Matthias Clasen from comment #6)
> If you look through gtkicontheme.c you'll find that there is a pretty
> pervasive assumption that icons just have a single size, not width/height.
> You might be able to make some things work with non-square items, but it
> will break in places, and I don't want to be on the hook for fixing all
> those places.

Matthias, please consider the following points:
- we have been using that icon for a long time before. This broke with the GTK 3.16 upgrade. My patch only fixes a regression.
- I think the assumptions you refer to are mostly in the named/themed icon code paths. GtkIconTheme can also be used to load arbitrary GIcons, and that's how the icon above is loaded. I think it would be reasonable not wanting to support non-square icons in theme directories.
- if you want to reject this patch, you should ensure that all the gtk_icon_theme/gtk_icon_info API asserts that only square images are passed to it, to make it clear that it's a programmer error to use the API with anything else. I would be quite sad if that was the case though.

I'm not happy, but since it caused a regression for you, this should probably go in. Two things, though:

1) Please make sure that make check still works - we do have icon loading tests nowadays

2) It would be nice to add a few testcases to cover the amount of non-square icon support that you rely on

Created attachment 305043 [details] [review]
testsuite: add a test for non-square symbolic icons

To verify the previous fix.

Created attachment 305044 [details] [review]
reftests: fix style class syntax in CSS file

Fixes reftests.

I can't think right now of any other specific behavior that we rely on.
make check was failing for unrelated reasons in a reftest for me, so the other patch fixes it.

thanks for the tests, please commit

Thanks for the review!

Attachment 304822 [details] pushed as 06df94f - icontheme: don't modify symbolic SVG dimensions when recoloring
Attachment 305043 [details] pushed as 0093b15 - testsuite: add a test for non-square symbolic icons
Attachment 305044 [details] pushed as 131abe2 - reftests: fix style class syntax in CSS file