tobi-wan-kenobi / bumblebee-status

bumblebee-status is a modular, theme-able status line generator for the i3 window manager.
https://bumblebee-status.readthedocs.io/en/main/
MIT License
1.21k stars 229 forks source link

No possibility to chain commands like audio and visual notification for pomodoro module #532

Closed lockejan closed 4 years ago

lockejan commented 4 years ago

Bug Report

not possible to play sounds and visual notification together

Summary

Affected module: pomodoro

Description:

I would expect the pomodoro module to accept also additional cmd's to play soundfiles besides 'notify-send' but it crashes and then falls back to the max break time.

How to reproduce

updated the notify string like so: pomodoro.notify="paplay /usr/share/sounds/gnome/default/alerts/bark.ogg & notify-send Pomodoro 'Time up!'"

trying the same with the os.system works indeed paplay causes the issue but I can't see what the actual problem is.

ghost commented 4 years ago

trying the same with the os.system works indeed

The value of pomodoro.notify is executed through bumblebee.util.execute(), which, in turn, uses subprocess.Popen() rather than os.system().

I have manually passed your pomodoro.notify value to bumblebee.util.execute() in a Python console after importing bumblebee.util and I've got a traceback indeed.

But I doubt paplay is to be blamed and here is why. subprocess.Popen() has a shell argument, which defaults to False. See the section about it here https://docs.python.org/2/library/subprocess.html#frequently-used-arguments. I'm linking to the Python 2 docs rather than the Python 3 docs at https://docs.python.org/3/library/subprocess.html#frequently-used-arguments because I really like the Python 2 version more, it goes into details:

shell=False disables all shell based features

For some reason the Python 3 version had this removed.

& in your command is a shell feature and my assumption is that the way bumblebee runs it gets this disabled, hence the traceback.

All of the above is just technical details that doesn't directly help you as end user of bumblebee-status to solve your problem. I do agree that as end user, you don't really have to know such details about the guts of subprocess Python module.

But is it actually possible to fix your problem without modifying the bumblebee-status code? I did a simple test: I extracted your pomodoro.notify command into a shell script, let's call it notify.sh. It can have the following content:

#!/bin/sh
paplay /usr/share/sounds/gnome/default/alerts/bark.ogg & notify-send Pomodoro 'Time up!'

And then your module parameter simply becomes pomodoro.notify="notify.sh" (watch the path, you will need to use a full path to make sure bumblebee-status finds the script). If I pass the script rather than the explicit command:

bumblebee.util.execute("notify.sh")

then I can get both the sound playing and the visual alert. I didn't test this directly with bumblebee-status, feel free to do it.

Does this approach suit you? If it works for you and you are happy with the result, the built-in documentation for pomodoro.notify should probably have the wrapper shell script approach for complex commands as explicit suggestion.

PS: If I'll try to play the devil's advocate, I could argue that having an additional shell script to hold the value of a module parameter goes against bumblebee's "Ease of use (no configuration files!)" philosophy. But then, we have the shell module which can be used to execute exactly external scripts, so I guess the devil's advocate argument above is invalid. It is interesting that the shell module actually has examples of complex shell commands using pipes, which are shell features as well! But after looking at the source, I notice that it uses shell=True. Which makes me wonder if we should consider this a security risk and eventually have a separate issue for it. But let @tobi-wan-kenobi get involved and decide on this.

lockejan commented 4 years ago

Thanks for your thorough feedback. I did already dig around a bit in the source code before and tested it with the param "shell=True" but with no luck. Now that you mention it has been removed for Py3.+ it makes sense why :)

However the initial error persists even with your suggested approach of having another shell-script like so:

pomodoro.notify='pomodoro-notify.sh'

#!/bin/bash

SOUND="/usr/share/sounds/gnome/default/alerts/bark.ogg"
notify-send Pomodoro "Time up!" & paplay $SOUND
ghost commented 4 years ago

Now that you mention it has been removed for Py3.+ it makes sense why

I meant that the documentation differs for Python 2 and Python 3, and I like the one for Python 2 more.

shell=True is still there in Python 3 subprocess module

pomodoro.notify='pomodoro-notify.sh'

Is the location or pomodoro-notify.sh in your PATH? Or you can try with an absolute path instead, like

pomodoro.notify='/path/to/your/script/pomodoro-notify.sh'

Do you get anything in the debug log if you run bumblebee-status with -d option (see bumblebee-status -h for details)?

ghost commented 4 years ago

I just did a real test.

My pomodoro-notify.sh script:

#!/bin/bash

SOUND="/usr/share/sounds/purple/alert.wav"
notify-send Pomodoro "Time up!" & paplay $SOUND

It's the same, I just use another sound file that is present in my system. I have this file in ~/bin, because this is where I put my user scripts, and I have this directory in PATH.

Running pomodoro-notify.sh from that directory:

$ cd ~/bin
$ ./pomodoro-notify.sh

works and produces the expected result: both sound and visual buzz.

Running the script from any other directory works as well, because, as I said above, ~/bin is in my PATH.

Now let's proceed to using it with bumblebee-status. Relevant snippet from i3 config:

bar {
        status_command python3 \
            /home/user/opt/bumblebee-status/bumblebee-status \
            -m pomodoro \
            -p pomodoro.work=1 \
               pomodoro.notify="/home/user/bin/pomodoro-notify.sh"
}

I can get both sound and visual buzz once time runs out.

Notice that:

  1. I use a full path to bumblebee-status to make sure it works and i3 finds it. I believe you have no problems with running bumblebee-status itself as i3 status bar, as I can judge from your very first post.

  2. I use pomodoro.work=1 so I only need to wait for 1 minute to observe what's going on when time runs out. I believe this is irrelevant for the result of the test.

  3. I use pomodoro.notify="/home/user/bin/pomodoro-notify.sh" - a full path to the notify script, to make sure bumblebee-status can find it.

  4. If I use pomodoro.notify="~/bin/pomodoro-notify.sh" instead, I can reproduce the crash reported by you, but this is obvious, because ~ is a shell feature which gets expanded by shell to user's home directory.

My conclusions:

  1. pomodoro module needs to explicitly tell the user to use a wrapper shell script for multiple commands, as I wrote above.
  2. Users need to be told to use full paths to the script, to avoid pitfalls like the one described in 4. above.
lockejan commented 4 years ago

Wow, thanks again for helping out.

Okay, I was so close already but I did miss a thing, that seems to cause general issues with scripts in i3.

It is that you can't put scripts in dotFolders, but I did ($HOME/.scripts/pomodoro-notify).

I had no problems executing these scripts in my terminal or via the os.system-approach mentioned earlier in a python-repl, but i3 seems to not be able to interprete 'dotFolders' in a filepath correctly and neither do other utils like rofi, even though these dir's are in my $PATH. I don't know why but I probably should avoid that in general.

I can confirm that it's working now, after I moved my notify-script into my $HOME/bin-folder.

Users need to be told to use full paths to the script, to avoid pitfalls like the one described in 4. above.

That isn't necessary if your script is in your $PATH (and not in a dotfolder) and even works like so: pomodoro.notify='pomodoro-notify'

I tried these as well:

tobi-wan-kenobi commented 4 years ago

Wow, pretty impressive to follow a discussion like that :)

I'll add the wrapper script info to the module.

Aside from that, I think it touches upon the general issue of: Spawning a subshell makes the usage more intuitive for the user in a lot of cases, but potentially opens up security risks. Exposing it to the user, on the other hand, doesn't really help, because the implication is hard to grasp.

Maybe a parameter such as "allow command chaining" might help, but I'm not sure myself.