phuhl / notify-send.py

A python-script like libnotify but with improved functionality
Other
96 stars 13 forks source link

--replaces-process broken in Python 3.9? #13

Closed fiskhest closed 3 years ago

fiskhest commented 3 years ago

After patching my system and receiving a newly updated version of python (3.9), I noticed that some of my indication notifications weren't working any longer.

I traced it back to the usage of --replaces-process, which produces the following traceback:

❯ ${HOME}/.local/bin/notify-send.py "Volume" "$VOLUME/100" \
                         --hint string:image-path:$ICON boolean:transient:true \
                         --replaces-process "volume-popup")
Traceback (most recent call last):
  File "/home/johan/.local/lib/python3.9/site-packages/notify_send_py/notify_send_py.py", line 118, in notify
    conn = Client(pidf.read())
  File "/usr/lib/python3.9/multiprocessing/connection.py", line 507, in Client
    c = SocketClient(address)
  File "/usr/lib/python3.9/multiprocessing/connection.py", line 635, in SocketClient
    s.connect(address)
OSError: [Errno 22] Invalid argument

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/johan/.local/bin/notify-send.py", line 8, in <module>
    sys.exit(main())
  File "/home/johan/.local/lib/python3.9/site-packages/notify_send_py/notify_send_py.py", line 205, in main
    NotifySendPyCLI()
  File "/home/johan/.local/lib/python3.9/site-packages/notify_send_py/notify_send_py.py", line 187, in __init__
    n_id = NotifySendPy().notify(
  File "/home/johan/.local/lib/python3.9/site-packages/notify_send_py/notify_send_py.py", line 124, in notify
    pidf.write(listener.address)
TypeError: write() argument must be str, not bytes

When I do the same using a python3.8 venv/binary, I get a working notification.

I modified line 123 to open the file with the binary flag (with open('/tmp/notify-send.py.address', 'wb') as pidf:) and notifications started working again, but something else broke in the flow around this line as I have to ctrl-c my script execution to get terminal control back. Traceback:

❯ ${HOME}/.local/bin/notify-send.py "Volume" "$VOLUME/100" \
                         --hint string:image-path:$ICON boolean:transient:true \
                         --replaces-process "volume-popup")
^CTraceback (most recent call last):
  File "/home/johan/.local/lib/python3.9/site-packages/notify_send_py/notify_send_py.py", line 118, in notify
    conn = Client(pidf.read())
  File "/usr/lib/python3.9/multiprocessing/connection.py", line 507, in Client

    c = SocketClient(address)
  File "/usr/lib/python3.9/multiprocessing/connection.py", line 635, in SocketClient
    s.connect(address)
ConnectionRefusedError: [Errno 111] Connection refused

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/johan/.local/bin/notify-send.py", line 8, in <module>
    sys.exit(main())
  File "/home/johan/.local/lib/python3.9/site-packages/notify_send_py/notify_send_py.py", line 205, in main
    NotifySendPyCLI()
  File "/home/johan/.local/lib/python3.9/site-packages/notify_send_py/notify_send_py.py", line 187, in __init__
    n_id = NotifySendPy().notify(
  File "/home/johan/.local/lib/python3.9/site-packages/notify_send_py/notify_send_py.py", line 130, in notify
    conn = listener.accept()
  File "/usr/lib/python3.9/multiprocessing/connection.py", line 468, in accept
    c = self._listener.accept()
  File "/usr/lib/python3.9/multiprocessing/connection.py", line 614, in accept
    s, self._last_accepted = self._socket.accept()
  File "/usr/lib/python3.9/socket.py", line 293, in accept
    fd, addr = self._accept()
KeyboardInterrupt

I tried to modify line 117 to also use the binary flag when reading file but there is no change in traceback.

Data from the file being read/written to:

❯ cat /tmp/notify-send.py.address                            
listener-1745943-0%
phuhl commented 3 years ago

Thanks for reporting.

On the first run of notify-send.py with --replaces-process, notify-send might block. This is known and documented. I guess, your fix works, then... Would you mind, creating a PR?

fiskhest commented 3 years ago

Alright, I see. Wouldn't mind creating a PR, but I noticed another error that I currently don't have the time or knowledge to pursue, namely:

Running

notify-send.py a --hint boolean:deadd-notification-center:true \
               string:type:clearPopups

now creates a new notification containing the letter a (see attached image), whereas it previously did it's intended function (cleared the most recent popup notification). Screenshot_2020-12-09-59_124x125

phuhl commented 3 years ago

Hm, that's intresting. I will look into this. Not sure if this is a deadd-notification-center issue or notify-send.py... Doesn't happen on my machine afaict...

fiskhest commented 3 years ago

Thanks. Let me know if there's anything I can do on my end to assist.

phuhl commented 3 years ago

Very strange, but I don't have any issue at all on my system. I, too run on python 3.9 and neither did I have to apply your patch from above, nor does the notification contain an "a". Did you change anything else? Did you try a fresh install of notify-send.py via pip3? Don't know, how to debug this, as I can't reproduce.

fiskhest commented 3 years ago

To my knowledge nothing else was changed and I did try a fresh install using pip3 install --force notify-send.py. I will double check my environment and do an actual purge on the package. Apologies for potentially wasting your time.

phuhl commented 3 years ago

Ok, I noticed, that I could not reproduce the original issue, because I am an idiot. Did not use the --replaces-process param. Your fix does work though. I also added a b to the read function because, it probably should both be binary.

Your second issue I still cannot reproduce, though. Your exact command works for me with python 3.9

fiskhest commented 3 years ago

That makes things a little more reasonable on my end, thanks for reporting back :smile: . I suggest we track that issue separately then, I'll create one when I have more details to share.

phuhl commented 3 years ago

This one should be closed by my last commit anyways

fiskhest commented 3 years ago

@phuhl :+1:

for clarity I figured out what was causing the second issue, and I am definitely also an idiot. :smile:

In case anyone ever stumbles upon this issue because they run into clearPopups producing an a instead of clearing the popup, make sure you don't have any other notification daemons running. (In my case I had commented out dunst in my startup .rc-file, without realising I also had it running as a systemctl unit enabled on boot...)

phuhl commented 3 years ago

Alright, glad you could fix it :+1: