dunst-project / dunst

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

Resolve paths for default icon #1210

Closed zappolowski closed 9 months ago

zappolowski commented 9 months ago

This (partially) fixes #1173 - support for expanding variables is done in #1215

default_icon was erroneously declared as being a plain string while it can also be a full path. Thus changing its type should solve the described issue already (without any side effects I could currently see).

fwsmit commented 9 months ago

It seems a little weird to add custom code for expanding the $HOME environment variable. This might also behave differently than people expect. I think it's better to look at the wordexp() function to see if we can use this function for expanding like a shell would. Otherwise just having the ~ expansion seems better to me.

Adding a wordexp-based expansion might also give issues with characters not accepted by the shell, though. So that makes it a little more complicated. It might be cool for certain functionality.

zappolowski commented 9 months ago

I was on the edge whether I should expand $HOME at all ... but I'm also in favor to remove it again and just expand ~/ in the beginning of a string.

wordexp could also expand commands like default_icon = `rm -rf ~`/asdf/ if I'm not mistaken and thus I would either go for a white-list appraoch of environment variables to expand (like $HOME, $XDG_CONFIG_DIR, $XDG_DATA_DIR and alike) or just leave it as is.

zappolowski commented 9 months ago

I took a closer look at wordexp and it might be worth a try. As far as I can see, expansion of commands can be disabled and referencing a non-existing variable could be turned into an error - which I consider the best approach.

zappolowski commented 9 months ago

I've implemented a version of the resolution using wordexp. One thing to decide: shall we use WRDE_UNDEF or not?

  1. If it is used, any undefined variable fails the expansion and the input is returned unaltered (with a warning).
  2. If it is not used, it is replaced with an empty string, e.g. $XDG_CONFIG_HOME/dunst just becomes /dunst. But using it allows us to use special syntax like ${XDG_CONFIG_HOME:-$HOME/.config}/dunst.
codecov-commenter commented 9 months ago

Codecov Report

Merging #1210 (ca09495) into master (19865c0) will not change coverage. The diff coverage is n/a.

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

@@           Coverage Diff           @@
##           master    #1210   +/-   ##
=======================================
  Coverage   66.03%   66.03%           
=======================================
  Files          46       46           
  Lines        7595     7595           
=======================================
  Hits         5015     5015           
  Misses       2580     2580           
Flag Coverage Δ
unittests 66.03% <ø> (ø)

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

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

fwsmit commented 9 months ago

I think it's better to keep the ~ expansion fix and the new feature of using wordexp to separate PR's. Could you create a new PR for the wordexp stuff? We can discuss it further there

fwsmit commented 9 months ago

Thanks, this makes it easy to review :)