Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Icon fix on Linux with libnotify #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

white43
Copy link

@white43 white43 commented Dec 3, 2023

The fix is for the following issue #57

@white43 white43 changed the base branch from dev to master December 3, 2023 11:35
@white43
Copy link
Author

white43 commented Dec 3, 2023

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
Copy link
Contributor

GiorgosXou commented Dec 3, 2023

@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
Copy link
Author

white43 commented Dec 4, 2023

@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
Copy link
Contributor

@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
Copy link
Author

white43 commented Dec 4, 2023

@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
Copy link
Contributor

True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants