search

MichaelAquilina / zsh-auto-notify

⏰ ZSH plugin that automatically sends out a notification when a long running task has completed.
GNU General Public License v3.0
415 stars 43 forks source link

Icon support reintroduced a previously squashed bug #58

Closed edenbynever closed 5 months ago

edenbynever commented 6 months ago

As currently implemented, the otherwise welcome introduction of support for icons has caused #45 to resurface.

https://github.com/MichaelAquilina/zsh-auto-notify/blob/452eee9454289c8a0a7269eeb1870c842685703d/auto-notify.plugin.zsh#L67

Because $icon_arg is quoted, it gets sent unconditionally and, in the case of it being the empty string, will be treated by notify-send as a third (and thus invalid) positional argument. Regrettably, the previous fix of simply removing the quotes would fail to account for space-containing icon names/paths.

I suspect we could get away with just passing "--icon=$icon", but I've only confirmed that suspicion against Dunst on X11 and mako on Wayland. The more robust solution probably involves adding another conditional and duplicating the command.

As an aside, this regression wasn't caught by the tests because zunit is perfectly happy with the following:

run notify-send too many args
assert $state equals 0

In fact, the assertion even holds with just run notify-send, whereas run false does cause it to fail, but I'm afraid I wasn't able to make much headway on that particular mystery.

NovaViper commented 6 months ago

I was wondering why I kept getting this error in my terminal! I actually went through resetting my computers thinking I accidentally messed up something with my NixOS config only to just learn that it was this particular issue 👀

MichaelAquilina commented 5 months ago

Sorry for that, would you be able to check if the latest changes (version 0.10.1) fix your issues?

NovaViper commented 5 months ago

@MichaelAquilina Can confirm it's fixed!