search

dunst-project / dunst

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

[experimental icon lookup] Improve config options #992

Closed fwsmit closed 2 years ago

fwsmit commented 2 years ago

This only applies to the experimental icon lookup. This should be fixed before the experimental icon lookup is enabled by default.

Currently icon_size is only used as the default icon size for looking up icons in a theme. It does not affect the scaling of said icon or other icons.

There are 3 main ways an icon can be passed to dunst:

Via icon name

The icon is searched in the selected theme. The icon is only found if the index.theme says the icon is suitable for icon_size. The icon should then be scaled to that size (this is necessary when the icon is scalable).

Via a path

Now dunst cannot choose what size icon is passed to it. In the old implementation the icon was scaled between min_icon_size and max_icon_size. If this is kept the same, there are 3 settings for icon sizes. We might just scale everything to icon_size, but that will perhaps result in some badly scaled icons. It will be a good starting point, though.

Raw icon data via dbus

Configuration can be done the same as for icons passed via a path.

Priorities

  1. The rule "new_icon"
  2. "image-data"
  3. "image-path"
  4. app_icon parameter
  5. for compatibility reason, "icon_data"
  6. The rule "default_icon"

TODO

imsamuka commented 2 years ago

Test if icons passed via path get scaled correctly.

This can be tested already? :confused:

We might just scale everything to icon_size, but that will perhaps result in some badly scaled icons.

Maybe we could create a new global option scale_non_svg, that disable and enable scaling for other formats. That could be used on a application/match basis too. This way we can remove min_icon_size and max_icon_size, right?

fwsmit commented 2 years ago

Test if icons passed via path get scaled correctly.

This can be tested already? :confused:

It should work as intended now. All icons are scaled to icon_size iirc.

We might just scale everything to icon_size, but that will perhaps result in some badly scaled icons.

Maybe we could create a new global option scale_non_svg, that disable and enable scaling for other formats. That could be used on a application/match basis too. This way we can remove min_icon_size and max_icon_size, right?

Yeah that's a good idea.

imsamuka commented 2 years ago

Raw icon data is supposed to scale as well when using scale_non_svg, right? The notifications from mpDris2 doesn't respect icon_size yet, only max_icon_size.

Changing icon_size on matches/applications is working well.

I took a while to figure out why a icon wasn't scaling up, but i got it: the file .../96x96/apps/enjoy-music-player.svg exists and is used, but it is actually a 48x48 :face_exhaling:. Since it's a SVG, dunst should scale these up too, i think?

fwsmit commented 2 years ago

Raw icon data is supposed to scale as well when using scale_non_svg, right? The notifications from mpDris2 doesn't respect icon_size yet, only max_icon_size.

I see, I can reproduce that. I'll take a look at it.

Changing icon_size on matches/applications is working well.

I took a while to figure out why a icon wasn't scaling up, but i got it: the file .../96x96/apps/enjoy-music-player.svg exists and is used, but it is actually a 48x48 face_exhaling. Since it's a SVG, dunst should scale these up too, i think?

Yes, I'll take a look at that one as well.

fwsmit commented 2 years ago

One problem I can see occurring with the proposed setup: When you set you icon size to 64 and search the icon "audio-volume-medium" in the Adwaita theme it will find the icon Adwaita/256x256/legacy/audio-volume-medium.png, which is not scalable, but can be scaled to a size of 64 according to the index.theme. If you then would set scale_non_svg to false, the icon will be a huge 256x256 size. While you would probably want to scale it down to a size of 64 or use the icon "audio-volume-medium-symbolic", which is actually a SVG format.

imsamuka commented 2 years ago

or use the icon "audio-volume-medium-symbolic", which is actually a SVG format.

That's rough... Using *-symbolic as a fallback, at least, shouldn't be active by default imho.

Maybe we could create something like scale_index_theme, as when active, it would respect the scalability described in index.theme. Also maybe all icons could be down-scaled, without losing too much quality (at least compared to up-scaling).

If someone activates scale_index_theme but have disabled scale_non_svg, that's probably his intended behavior, something like "i trust index.theme but not everyone else".

fwsmit commented 2 years ago

I think we should just respect the scalability of index.theme in all cases

imsamuka commented 2 years ago

Ah, yeah, i don't think someone would disable this after all

fwsmit commented 2 years ago

1003 should fix the issues you brought up of icons not being the right size.

imsamuka commented 2 years ago

I would love to test it, but I don't know how to "clone" a pull request on my machine yet :monkey:

fwsmit commented 2 years ago

You can clone my fork of dunst and git checkout the branch. Or you can download the github command line tool and do gh pr checkout 1003 (or something similar)

imsamuka commented 2 years ago

You can clone my fork of dunst

Oh, well, i didn't though of that, it's very simple then. Thank you, i will test it tomorrow!

imsamuka commented 2 years ago

I tested and all these succeeded to scale up and down:

The scale_non_svg will be implemented in this PR too?

fwsmit commented 2 years ago

Than

I tested and all these succeeded to scale up and down:

* Raw icon data (from `mpDris2` album arts)

* Icon passed directly via path, on `new_icon` rule

* `.../Papirus-Dark/96x96/apps/enjoy-music-player.svg` (48x48 file), on `-i`

* `.../Adwaita/24x24/legacy/zoom-in.png`, on `-i` (of course, with a bad upscaling)

Thanks for testing, and sorry for the late reply. Looks good.

The scale_non_svg will be implemented in this PR too?

No not yet. I still have to decide if I'm gonna include that option.

fwsmit commented 2 years ago

For now I will close this issue until real configuration issues come up