dunst-project / dunst

Lightweight and customizable notification daemon
https://dunst-project.org
Other
4.42k stars 338 forks source link

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

Open bynect opened 4 months ago

bynect commented 4 months ago

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

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

codecov-commenter commented 4 months ago

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 ```diff @@ Coverage Diff @@ ## master #1302 +/- ## ======================================= Coverage 65.52% 65.52% ======================================= Files 48 48 Lines 7997 7997 ======================================= Hits 5240 5240 Misses 2757 2757 ``` | [Flag](https://app.codecov.io/gh/dunst-project/dunst/pull/1302/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/dunst-project/dunst/pull/1302/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | `65.52% <100.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project#carryforward-flags-in-the-pull-request-comment) to find out more.

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

bynect commented 3 months ago

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

fwsmit commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.