i3blocks-contrib icon indicating copy to clipboard operation
i3blocks-contrib copied to clipboard

volume-pulseaudio: Process click inputs while in SUBSCRIBE mode.

Open MarijnS95 opened this issue 4 years ago • 9 comments

volume-pulseaudio was previously unable to process click inputs while being in persist mode, for valid reasons. With i3blocks now sending the key events to stdin while in this mode (i3blocks: 6e8b51d) it is possible to make the block interactable again.

This change moves the subscribe pipeline and printing to a background task since stdout remains attached by default. Input events are received on stdin which are read in a loop and passed to handle_input, that encapsulates the case satement previously used to parse values from $BLOCK_BUTTON.

On a side note, unrelated to the PR: Are there reasons to have this separate SUBSCRIBE variable, instead of basing this on $interval == "persist"? Would it be desired to base the default value (currently 0) on this comparison instead?

MarijnS95 avatar Jan 14 '20 23:01 MarijnS95

The subprocesses added by this change does not get killed when the blocket is killed, for example when you restart the window manager. At least that's the case for me (running Sway).

Adding a trap before the last if-clause fixes this for me:

trap 'pkill -P $$' SIGINT SIGTERM EXIT

Regarding your side note, I agree that $interval == "persist" should be enough to invoke this behaviour, as that is the only functioning behavior when this is set. SUBSCRIBE should probably stay though for backwards compatibility.

carlsmedstad avatar May 16 '20 11:05 carlsmedstad

@carlsmedstad That's interesting, I can't reproduce that here but it could be ZSH being smart about killing subprocesses/jobs. Manually sending a SIGINT shows the issue.

Are you sure all signals need to be handled rather than just the EXIT pseudo-signal? This seems to needlessly invoke pkill twice. Besides I see solutions involving kill 0 as well, but am not enough of a bash-guru to commend either method. The latter one should kill all subprocesses (if more nesting is added) which seems the safer solution of the two. That fits with another patch I want to push that wraps the pactl chain in a while true to account for Pulse restarts: that starts another shell to wrap the pipes and restart it on exit.

Thanks for the suggestion!

MarijnS95 avatar May 16 '20 15:05 MarijnS95

I did some more testing and I can indeed not reproduce this in i3.

For Sway though when using kill 0 in the trap and running swaymsg reload the whole window manager exits and I'm pushed back to the login screen. That's how I ended up with pkill -P $$.

Both this and the unkilled processes might have to do more with how Sway reloads itself rather than i3blocks and this blocklet.

carlsmedstad avatar May 17 '20 12:05 carlsmedstad

@carlsmedstad That's unfortunate; how about pkill $(jobs -p)?

EDIT: Though that likely doesn't work either, given that only the & shell is a job - all subprocesses will live on unless the trap is moved in there.

MarijnS95 avatar May 17 '20 12:05 MarijnS95

@MarijnS95 I think I found out why Sway behaves differently, the sway and swaybar processes share the same process group. This is not the case for i3 and i3bar. See this: Sway:

UID          PID    PPID    PGID     SID  C STIME TTY          TIME CMD
root      360370       1  360370  360370  0 15:02 ?        00:00:00 login -- carsme
carsme    360374  360370  360374  360374  2 15:02 tty1     00:03:11  \_ sway -V
carsme    360497       1  360374  360374  1 15:02 tty1     00:02:09 Xwayland :0 -rootless -terminate -listen 22 -listen 24 -wm 56
carsme    362459       1  362459  362459  0 15:05 pts/2    00:00:00 /bin/bash
carsme    363376       1  360374  360374  0 15:08 tty1     00:00:00 swaybg -o * -i /home/carsme/.wallpapers/current.png -m fill
carsme    363378       1  360374  360374  0 15:08 tty1     00:00:34 swaybar -b bar-0
carsme    363381  363378  360374  360374  0 15:08 tty1     00:00:03  \_ i3blocks
carsme    363382  363381  360374  360374  0 15:08 tty1     00:00:00      \_ /bin/bash /home/carsme/.config/i3blocks/volume-pulseaudio
carsme    363427  363382  360374  360374  0 15:08 tty1     00:00:00      |   \_ pactl subscribe
carsme    363428  363382  360374  360374  0 15:08 tty1     00:00:00      |   \_ grep change
carsme    363429  363382  360374  360374  0 15:08 tty1     00:00:00      |   \_ /bin/bash /home/carsme/.config/i3blocks/volume-pulseaudio
carsme    399378  363381  360374  360374  0 16:51 tty1     00:00:00      \_ /usr/bin/perl /home/carsme/.config/i3blocks/cpu_usage
carsme    399380  399378  360374  360374  0 16:51 tty1     00:00:00          \_ mpstat 1 1

i3:

UID          PID    PPID    PGID     SID  C STIME TTY          TIME CMD
root      400151       1  400151  400151  0 16:52 ?        00:00:00 login -- carsme
carsme    400161  400151  400161  400161  0 16:52 tty1     00:00:00  \_ /bin/sh /usr/bin/startx
carsme    400291  400161  400161  400161  0 16:52 tty1     00:00:00      \_ xinit /home/carsme/.xinitrc -- /home/carsme/.xserverrc :0 vt1 -keeptty -auth /tmp/serverauth.pVI8TCJkWk
carsme    400292  400291  400292  400161  1 16:52 tty1     00:00:00          \_ /usr/lib/Xorg -nolisten tcp :0 vt1 -keeptty -auth /tmp/serverauth.pVI8TCJkWk vt1
carsme    400302  400291  400302  400161  0 16:52 tty1     00:00:00          \_ i3 -V
carsme    400319       1  400318  400318  0 16:52 ?        00:00:00 i3bar --bar_id=bar-0 --socket=/run/user/1000/i3/ipc-socket.400302
carsme    400323  400319  400323  400318  0 16:52 ?        00:00:00  \_ i3blocks
carsme    400324  400323  400323  400318  0 16:52 ?        00:00:00      \_ /bin/bash /home/carsme/.config/i3blocks/volume-pulseaudio
carsme    400365  400324  400323  400318  0 16:52 ?        00:00:00          \_ pactl subscribe
carsme    400366  400324  400323  400318  0 16:52 ?        00:00:00          \_ grep change
carsme    400367  400324  400323  400318  0 16:52 ?        00:00:00          \_ /bin/bash /home/carsme/.config/i3blocks/volume-pulseaudio

This is why kill 0 ended up killing the whole wm.

I did find a neat work-around for this though by starting i3blocks with setsid to create a new group so I think using kill 0 would be fine!

carlsmedstad avatar May 17 '20 16:05 carlsmedstad

@carlsmedstad Sources mentioned a similar issue with kill 0 killing the entire process tree rather than only the processes below it (and itself?). According to those outputs it is indeed the matching Session ID that causes kill 0 to destroy it all. As you say wrapping all subprocesses and jobs in a new group that can easily and consistently be killed seems the safest solution :grin:

Would you mind preparing a patch that applies this to the PR for proper attribution? Since you mentioned starting i3blocks with setsid, I rather aim to start the pulse block itself (and/or sub-processes) with setsid instead to make the solution properly portable and not surprise others.

MarijnS95 avatar May 22 '20 19:05 MarijnS95

@MarijnS95 Could not get this to work without splitting the script into several files. Considering rewriting this in Python at this point as the complexity has grown a bit.

carlsmedstad avatar May 25 '20 18:05 carlsmedstad

@carlsmedstad I completely agree. As a matter of fact the first thing that was looked up when starting this modification was a poll/select/epoll in bash, to have a single thread handle buttons on stdin and output from the pactl executable simultaneously. In python one might use a DBus api and/or pulse wrapper directly.

Let's collect and decide on a way forward before implementing this. Note there's a bit of extra logic to cover but that should be easy to reimplement.

MarijnS95 avatar May 25 '20 19:05 MarijnS95

@MarijnS95 I think I will be quite busy this week, maybe I have some time this weekend. When I have the time I would be up for rewriting it and create a PR for volume-pulseaudio2. I think we should try incorporate the features of all open PRs from the get go.

If you have the time and would like to start instead I'm all for that as well :)

carlsmedstad avatar May 25 '20 20:05 carlsmedstad