search

ms7m / notify-py

:speech_balloon: | A simple Python Module for sending cross-platform desktop notifications on Windows, macOS and Linux
MIT License
262 stars 26 forks source link

Icon fix on Linux with libnotify #58

Open white43 opened 11 months ago

white43 commented 11 months ago

The fix is for the following issue https://github.com/ms7m/notify-py/issues/57

white43 commented 11 months ago

Readme states I need to open a merge request to the dev branch, though it's outdated. So, I have changed target branch to master.

GiorgosXou commented 11 months ago

@ms7m I can verify this PR is ok and it works just fine. Althought @white43 it seems weird to me that you experience this issue, what notification-daemon you use (I personally use dunst and I don't experience this issue)

Dunst - A customizable and lightweight notification-daemon 1.9.2 (2023-04-20)
white43 commented 11 months ago

@GiorgosXou I'm on Xfce. I see the following notification daemon is installed:

extra/xfce4-notifyd 0.9.3-1 (xfce4-goodies) [installed]
    Notification daemon for the Xfce desktop

Do you think the problem is deeper than libnotify command line arguments?

GiorgosXou commented 11 months ago

@white43 While I am pretty confident that this PR will address the issue, I do have a slight concern about other notification-deamons that might result into issues like: capturing spaces (in a path) as different commands eg. ~/folder/folder space/file.png

I'm on Xfce...

Just in case, what ldconfig -p | grep libnotify outputs? (Mine outputs version 4)

Do you think the problem is deeper than libnotify command line arguments?

Yes, because we both use Arch (btw), and In my case I tested Dunst in both of my machines (with & without the PR) and works just fine, probably meaning it has to do with how the notification-deamon handles libnotify command line arguments.


PS. I use xfce on my FreeBSD machine, I might later check the issue there too.

white43 commented 11 months ago

@GiorgosXou

Mine outputs version 4

Same for me:

ldconfig -p | grep libnotify
    libnotify.so.4 (libc6,x86-64) => /usr/lib/libnotify.so.4
    libnotify.so (libc6,x86-64) => /usr/lib/libnotify.so

I understand your concerns, and this issue has something to do with handling input by xfce4-notifyd. Nevertheless, it does not change that subprocess.Popen (which is used internally by notify-py) documentation (https://docs.python.org/3/library/subprocess.html#popen-constructor) clearly suggests splitting arguments and their values before sending them to Popen. This library does the opposite.

GiorgosXou commented 11 months ago

True