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

Resolve paths for default icon #1210

Merged

Conversation

zappolowski
Copy link
Member

@zappolowski zappolowski commented Oct 14, 2023

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

fwsmit commented Oct 17, 2023

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

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

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

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

codecov-commenter commented Oct 18, 2023

Codecov Report

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

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

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

@fwsmit
Copy link
Member

fwsmit commented Oct 20, 2023

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

Currently is always treated as a plain string so no expansion of `~`
happens. The real resolution (name of icon vs. path to icon) is done
later when the notification is about to be rendered.
@fwsmit
Copy link
Member

fwsmit commented Oct 24, 2023

Thanks, this makes it easy to review :)

@fwsmit fwsmit merged commit d5cc29b into dunst-project:master Oct 24, 2023
18 checks passed
@zappolowski zappolowski deleted the resolve-paths-for-default-icon branch October 24, 2023 18:01
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.

Can't set default_icon path with environment variable ($HOME or ~)
3 participants