-
Notifications
You must be signed in to change notification settings - Fork 252
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
chore(363): replace notify2 by py-notifier #365
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review is ok except one note related to my overall comment which is:
I will investigate whether dbus can be removed and probably open a second PR to cleanup the dependencies.
I'd rather we do this now since this is closely related.
We should avoid unnecessary dependencies. If we don't crash when dbus is missing and notify when it is present then I think we should remove the dependency.
Just pinging @obilodeau in case you missed the notification. I removed all DBUS references, this should be ready for merge whenever you have time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we can't get rid of notify-osd
, I mean if it's required by libnotify-bin
to work, it'll be pulled by it, no? Also, if I have a desktop with dbus and notifications working then it means I have something which provides notify-osd
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last little thing. See comment below. Also, I'm off to test in a Ubuntu server system now w/o a GUI and see if my assumptions about notify-osd
are correct.
This replaces notify2 by py-notifier.
I think (but would need further testing) that this allows us to get rid of dbus dependencies since it uses
libnotify-bin
. Which has been added to the README and docker files (not relevant to the CI pipeline AFAIK)I will investigate whether dbus can be removed and probably open a second PR to cleanup the dependencies.
This also paves the road to eventually supporting windows notifications once win10toast is fixed. (see jithurjacob/Windows-10-Toast-Notifications#86)
Closes #363.