Make recursive icon lookup work with size == 0

[bynect]

Only works with scalable icons, but at least it works.

@fwsmit do you think this is a good enough solution for #1094?

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 65.52%. Comparing base (e5f188c) to head (41ffe7c). Report is 4 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1302   +/-   ##
=======================================
  Coverage   65.52%   65.52%           
=======================================
  Files          48       48           
  Lines        7997     7997           
=======================================
  Hits         5240     5240           
  Misses       2757     2757           

Flag	Coverage Δ
unittests	65.52% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[bynect]

Looking back at it maybe it's better to print an error if recursive lookup is enabled with min_size = 0?

[fwsmit]

Yeah, i think it's best to get rid of min size. It doesn't make much sense with the new icon lookup, since it only uses one size.

[bynect]

Yeah, i think it's best to get rid of min size. It doesn't make much sense with the new icon lookup, since it only uses one size.

Honestly I don't know if it is better to ignore the min size for icon lookup. Because at the moment it is used for determining the size of non scalable icons.

In this pr I made it so that if minsize is 0 we get whatever size we found. Which seems sensible... but i don't know if some warning should be issued or something 🤔

[fwsmit]

What I mean is that having a max and min size doesn't make much sense right now, since only one size is used for the lookup. So it's best to rename this setting or somehow use both sizes again

[bynect]

What I mean is that having a max and min size doesn't make much sense right now, since only one size is used for the lookup. So it's best to rename this setting or somehow use both sizes again

proposal = make a icon_size a length so that you can say (min,max) or size

[fwsmit]

Yeah, but what are you doing with scalable icons? Are you making them as big as possible? In practice most icons are scalable, so only the biggest size is used. For consistency of the notifications it's better for all icons to be the same size. For some notifications, like album art for music, you might want bigger icons. But that should be done with a rule.

So in what case would you want a min and max size?

[bynect]

Yeah, but what are you doing with scalable icons? Are you making them as big as possible? In practice most icons are scalable, so only the biggest size is used. For consistency of the notifications it's better for all icons to be the same size. For some notifications, like album art for music, you might want bigger icons. But that should be done with a rule.

So in what case would you want a min and max size?

I guess you are referring to making an icon_size length value.

The option is to have a value (min, max) like for width. Or alternatively a single value that makes min =max and in practice would mean that the icon has a fixed size. This latter part could be achieved by setting icon_max_size = icon_min_size but that would change the semantics of those in another way.

So the idea is to implement this more versatile single option that has also a sane behavior if you want fixed size icons and is tailored to work with recursive lookup.

The old options can obviously be deprecated at a later date.

[bynect]

I will have to redo this based on #1365