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

New message notifications using notify-rust #19

Merged
merged 5 commits into from
Nov 27, 2020
Merged

Conversation

cleeyv
Copy link
Contributor

@cleeyv cleeyv commented Sep 27, 2020

The initial version is just a generic notification every time a message is received. I'm currently testing this with gnome-shell and it is working well. As suggested in the readme, I agree that these notifications should be optional. I imagined doing that with a config in gurk.toml but haven't tried it out yet. I'll probably submit the PR once that part is working.

@cleeyv cleeyv marked this pull request as ready for review October 1, 2020 16:52
@cleeyv
Copy link
Contributor Author

cleeyv commented Oct 1, 2020

This is ready for review! In the end it is not configured through gurk.toml at all, instead it is disabled by default and is optionally enabled at build time with the cargo argument --feature notifications. It is implemented with rust-notify, which in turn depends on the dbus-rs. I implemented it as an optional cargo build feature because dbus-rs still depends on the C library libdbus for both building and running this functionality. This is also why I had to add some build libdbus-related dependencies to the CI config for the builds to complete successfully.

As far as I can tell, there are two possible alternatives to implementing it as an opt-in cargo feature:

  • Include notify-rust by default, and list libdbus-1-dev and pkg-config as build dependencies of the project. This is currently the only viable alternative, and I thought making it optional at compile time was better than requiring more external dependencies (especially for the server use case where there a higher chance that the dbus libraries are not already installed).
  • For the future, use a rust-only notifications stack, without external C dependencies. This is being worked on but apparently isn't fully usable yet, see libdbus-sys: consider a Rust-native implementation diwic/dbus-rs#100.

For both of these alternatives, the new message notifications being optional would require it to be configurable at run time (probably through gurk.toml).

I found this PR from the starship repo to be informative for background info starship/starship#1019 because they were also implementing dbus notifications with notify-rust and ran into many of the same issues that I did.

@boxdot
Copy link
Owner

boxdot commented Nov 27, 2020

Thanks for the PR and the investigation! LGTM

@boxdot boxdot merged commit f5d63e8 into boxdot:master Nov 27, 2020
@cleeyv cleeyv deleted the notify branch March 4, 2021 04:32
@zeenix
Copy link

zeenix commented Feb 1, 2022

  • For the future, use a rust-only notifications stack, without external C dependencies. This is being worked on but apparently isn't fully usable yet, see

Just FYI if you still have the need/desire for a pure D-Bus crate, zbus is at your service. We recently rolled out 2.0, which is primarily async API, The upcoming release (~1 week) will also support Windows platform, provide tighter tokio integration and support for TCP sockets with more auth mechanisms.

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.

3 participants