Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make recursive icon lookup work with size == 0 #1302

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bynect
Copy link
Member

@bynect bynect commented Mar 3, 2024

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

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

@bynect bynect requested a review from fwsmit March 3, 2024 17:28
@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bynect
Copy link
Member Author

bynect commented Apr 7, 2024

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

@bynect bynect requested a review from zappolowski April 7, 2024 20:13
@fwsmit
Copy link
Member

fwsmit commented Apr 8, 2024

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
Copy link
Member Author

bynect commented Apr 8, 2024

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 🤔

@bynect bynect linked an issue Apr 9, 2024 that may be closed by this pull request
@bynect bynect marked this pull request as draft April 9, 2024 19:25
@fwsmit
Copy link
Member

fwsmit commented Apr 10, 2024

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
Copy link
Member Author

bynect commented Apr 10, 2024

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
Copy link
Member

fwsmit commented Apr 10, 2024

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
Copy link
Member Author

bynect commented Apr 10, 2024

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
Copy link
Member Author

bynect commented Jul 15, 2024

I will have to redo this based on #1365

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some icons can't be found
3 participants