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
404 stars 41 forks source link

Unquote $icon in `notify-send` command #57

Open Grafcube opened 4 months ago

Grafcube commented 4 months ago

If AUTO_NOTIFY_ICON_SUCCESS or AUTO_NOTIFY_ICON_FAILURE is not set, the command fails with Invalid number of options since $icon is empty.

edenbynever commented 4 months ago

This approach of simply removing the quotes solved #45 because $transient would never contain any spaces, but the same can't be guaranteed for $icon_arg; some users really do put spaces in their filenames.

Thankfully, it looks like we can replace "$icon_arg" with ${icon_arg:+"$icon_arg"}, which will expand to the quoted value if it exists (accounting for spaces) and nothing (not even the empty string) otherwise. Doing that would cause the following test to fail, but only because it now contains an extraneous space at the end.

https://github.com/MichaelAquilina/zsh-auto-notify/blob/452eee9454289c8a0a7269eeb1870c842685703d/tests/test_auto_notify_send.zunit#L92

Grafcube commented 4 months ago

I've changed it to use ${icon_arg:+"$icon_arg"}. It didn't even occur to me that icon names could contain spaces.

edenbynever commented 4 months ago

It didn't even occur to me that icon names could contain spaces.

As I understand it, icon names are essentially just basenames of filesystem paths, which shouldn't but certainly can contain spaces. I've no doubt someone somewhen would've been bitten by the unquoted form, so thanks for updating.

MichaelAquilina commented 4 months ago

Thanks for the fix. I'm not around a laptop to test this, but I should be able to give it a look sometime today. The failing tests need to be looked at as they could indicate a problem