Always check /usr/share/icons in recursive lookup

[bynect]

As the name says. It may be good since /usr/share/icons is so ubiquitous to check it anyway. Especially if XDG_DATA_DIRS is changed unknowingly from the user (like it happened to me #1270).

[codecov-commenter]

Codecov Report

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

Comparison is base (2fcea84) 65.45% compared to head (f5f3497) 65.45%.

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

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #1271 +/- ## ======================================= Coverage 65.45% 65.45% ======================================= Files 46 46 Lines 7749 7750 +1 ======================================= + Hits 5072 5073 +1 Misses 2677 2677 ``` | [Flag](https://app.codecov.io/gh/dunst-project/dunst/pull/1271/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/1271/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | `65.45% <100.00%> (+<0.01%)` | :arrow_up: | 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.

[zappolowski]

According to the spec an unset or empty XDG_DATA_DIR should be regarded as being set to /usr/local/share:/usr/share - as it was implemented before.

If the user explicitly sets XDG_DATA_DIR incorrectly we should not try to fix it. Maybe the intention was to get rid of /usr/share in that case? We don't know.

IMHO: I consider the problem described in #1270 a user configuration issue and thus nothing which should be changed by dunst.

[bynect]

According to the spec an unset or empty XDG_DATA_DIR should be regarded as being set to /usr/local/share:/usr/share - as it was implemented before.

If the user explicitly sets XDG_DATA_DIR incorrectly we should not try to fix it. Maybe the intention was to get rid of /usr/share in that case? We don't know.

IMHO: I consider the problem described in #1270 a user configuration issue and thus nothing which should be changed by dunst.

This could be valid if dunst didn't also check /usr/share/pixmaps on its own accord. Also if you check /usr/share/icons always even if XDG_DATA_DIRS is empty the behavior is the exact same. The only difference would be the order of resolution in this case, but I can invert the line of share/bitmaps and share/icons.

I don't see how making a 2 line change to improve dunst ease of use is bad honestly

[zappolowski]

It's hard to judge from the description of the original issue how the messed up XDG_DATA_DIR came to be. If it's installed from a package, it's the package maintainer to blame. XDG_DATA_DIR should be augmented and not replaced in such cases. (Just out of curiosity: how did you install wine in such a way that it needs to adjust XDG_DATA_DIR?)

even if XDG_DATA_DIRS is empty the behavior is the exact same

If I actually want to use an adjusted XDG_DATA_DIR now (e.g. with /usr/share explicitly removed), it's plain impossible after your changes - the behavior changed in a breaking way.

This could be valid if dunst didn't also check /usr/share/pixmaps on its own accord.

It does this in accordance with this spec.

[bynect]

It's hard to judge from the description of the original issue how the messed up XDG_DATA_DIR came to be. If it's installed from a package, it's the package maintainer to blame. XDG_DATA_DIR should be augmented and not replaced in such cases. (Just out of curiosity: how did you install wine in such a way that it needs to adjust XDG_DATA_DIR?)

even if XDG_DATA_DIRS is empty the behavior is the exact same

If I actually want to use an adjusted XDG_DATA_DIR now (e.g. with /usr/share explicitly removed), it's plain impossible after your changes - the behavior changed in a breaking way.

This could be valid if dunst didn't also check /usr/share/pixmaps on its own accord.

It does this in accordance with this spec.

I just did emerge wine and it set XDG_DATA_DIRS which was previously unset. After being set dunst wasn't working anymore, but I didn't notice at first. Now, since /usr/share/icons is quite ubiquitous I don't why having an extra check is problematic.

If you want the user to be able to set a different icon path they can simply set xdg_data_dirs and it will have precedence in the resolution order. The same problem could be said anyway for the /usr/share/bitmaps path, which is searched even if the user didn't set it in the environment. So in the end if a power user wants to change the icon path they can and normal user can just gain from this extra check.

[fwsmit]

Changing you XDG_DATA_DIRS probably causes more issues than just dunst icons, so it's better to report this issue to the gentoo wine package

[bynect]

Changing you XDG_DATA_DIRS probably causes more issues than just dunst icons, so it's better to report this issue to the gentoo wine package

To be fair XDG_DATA_DIRS was unset before and I didn't think that it was used by dunst for icon resolution. Maybe it should be made clearer when the theme is not found for easier troubleshooting

[zappolowski]

Maybe it should be made clearer when the theme is not found for easier troubleshooting

I don't understand this one. When icon_theme in dunstrc references an unknown icon theme (or a icon theme used therein inherits from another icon theme which cannot be found), dunst already issues a warning. Shouldn't this be enough for troubleshooting?

To be fair XDG_DATA_DIRS was unset before and I didn't think that it was used by dunst for icon resolution.

Maybe I'm misunderstanding you, but XDG_DATA_DIR was obviously used ... you even changed the line using it.

I just did emerge wine and it set XDG_DATA_DIRS which was previously unset.

I don't have Gentoo myself, but asked a friend to run emerge wine and from what I could see it sets up XDG_DATA_DIRS to /usr/local/share:/usr/share:/etc/eselect/wine/share, which seems correct and does not need your fix, as /usr/share is still present. How does your "messed up" version look like?

After being set dunst wasn't working anymore, but I didn't notice at first. Now, since /usr/share/icons is quite ubiquitous I don't why having an extra check is problematic.

My main point is, that no fix should be needed when XDG_DATA_DIRS is set correctly and thus solving the root issue (incorrect XDG_DATA_DIRS) seems to be the better approach. Also, although this fix might work for dunst, all other tools adhering to the Icon Theme Specification in combination with the XDG Base Directory Specification would still be broken and would need fixing as well.

If you want the user to be able to set a different icon path they can simply set xdg_data_dirs and it will have precedence in the resolution order. The same problem could be said anyway for the /usr/share/bitmaps path, which is searched even if the user didn't set it in the environment.

/usr/share/bitmaps is not searched, but /usr/share/pixmaps and as already stated, that's because it's mandated in the Icon Theme Specification.

So in the end if a power user wants to change the icon path they can

That's incorrect, as there's no way to get rid of /usr/share/icons now, as that one is enforced.

[bynect]

Maybe it should be made clearer when the theme is not found for easier troubleshooting

I don't understand this one. When icon_theme in dunstrc references an unknown icon theme (or a icon theme used therein inherits from another icon theme which cannot be found), dunst already issues a warning. Shouldn't this be enough for troubleshooting?

To be fair XDG_DATA_DIRS was unset before and I didn't think that it was used by dunst for icon resolution.

Maybe I'm misunderstanding you, but XDG_DATA_DIR was obviously used ... you even changed the line using it.

I just did emerge wine and it set XDG_DATA_DIRS which was previously unset.

I don't have Gentoo myself, but asked a friend to run emerge wine and from what I could see it sets up XDG_DATA_DIRS to /usr/local/share:/usr/share:/etc/eselect/wine/share, which seems correct and does not need your fix, as /usr/share is still present. How does your "messed up" version look like?

After being set dunst wasn't working anymore, but I didn't notice at first. Now, since /usr/share/icons is quite ubiquitous I don't why having an extra check is problematic.

My main point is, that no fix should be needed when XDG_DATA_DIRS is set correctly and thus solving the root issue (incorrect XDG_DATA_DIRS) seems to be the better approach. Also, although this fix might work for dunst, all other tools adhering to the Icon Theme Specification in combination with the XDG Base Directory Specification would still be broken and would need fixing as well.

If you want the user to be able to set a different icon path they can simply set xdg_data_dirs and it will have precedence in the resolution order. The same problem could be said anyway for the /usr/share/bitmaps path, which is searched even if the user didn't set it in the environment.

/usr/share/bitmaps is not searched, but /usr/share/pixmaps and as already stated, that's because it's mandated in the Icon Theme Specification.

So in the end if a power user wants to change the icon path they can

That's incorrect, as there's no way to get rid of /usr/share/icons now, as that one is enforced.

I didn't set XDG_DATA_DIRS anywhere. I installed wine and it set automatically that var to /etc/eselect/wine. So the icon themes that worked normally before (Adwaita) now spammed Icon theme not found in the console. I enabled debug but since I didn't know that XDG_DATA_DIRS was used I had to see the code that loads the icon. I even changed the icon_path (which is ignored if recursive icons are enabled and I think this should be changed).

But I guess that's on me, let's just stick to the protocols...

[fwsmit]

Let's stick to the spec. I'll close this PR.