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

DBus reason of closing #104

Closed
antonshkurenko opened this issue Apr 28, 2021 · 11 comments · Fixed by #106
Closed

DBus reason of closing #104

antonshkurenko opened this issue Apr 28, 2021 · 11 comments · Fixed by #106

Comments

@antonshkurenko
Copy link

Please, do not ignore this value here:

if let (&MessageItem::UInt32(nid), &MessageItem::UInt32(_)) = (&items[0], &items[1]) {

According to the spec: https://specifications.freedesktop.org/notification-spec/latest/ar01s09.html

image

The second param it's the only way to handle click on notification itself

@hoodie
Copy link
Owner

hoodie commented Apr 29, 2021

oh cool, thanks for pointing this out, I hadn't noticed that yet

hoodie added a commit that referenced this issue May 2, 2021
The closure passed into
[`NotificationHandle::on_close()`](https://docs.rs/notify-rust/4.5.0/notify_rust/struct.NotificationHandle.html#method.on_close)
can now also take a parameter that will receive the reason why this Notification has been closed.

Fixes #104
Thanks for noticing @tonyshkurenko
@hoodie
Copy link
Owner

hoodie commented May 2, 2021

@tonyshkurenko how about this?

@hoodie
Copy link
Owner

hoodie commented May 2, 2021

actually, since you mention "The second param it's the only way to handle click on notification itself" what Desktop environment are you using? because the under KDE at least clicking on the notification triggers the "default" action".

@antonshkurenko
Copy link
Author

I tested on xfce4 and gnome. Thanks for your help!

@hoodie
Copy link
Owner

hoodie commented May 7, 2021

thank you for pointing this out

@antonshkurenko
Copy link
Author

antonshkurenko commented May 7, 2021

Could you please also export ActionResponse type, because it's not visible outside, if you want to use handle_action method

error[E0603]: module `xdg` is private
   --> src/notifier.rs:415:76
    |
415 |                     notify_rust::handle_action(uid, |action: &notify_rust::xdg::ActionResponse| {
    |                                                                            ^^^ private module
    |
note: the module `xdg` is defined here
   --> /home/parallels/.cargo/registry/src/github.com-1ecc6299db9ec823/notify-rust-4.5.0/src/lib.rs:153:45
    |
153 | #[cfg(all(unix, not(target_os = "macos")))] mod xdg;
    |                                             ^^^^^^^^

@hoodie
Copy link
Owner

hoodie commented May 7, 2021

could you post the code you tried please?

@hoodie
Copy link
Owner

hoodie commented May 7, 2021

also: if you like you could open a PR that fixes that

@antonshkurenko
Copy link
Author

I will file a PR then :)

@antonshkurenko
Copy link
Author

antonshkurenko commented May 11, 2021

Regarding code I've tried, I simplified it a bit, but generally it's:

notify_rust::handle_action(uid, |action: &notify_rust::xdg::ActionResponse| {
    match action {
        notify_rust::xdg::ActionResponse::Custom(key) => ...,
        notify_rust::xdg::ActionResponse::Closed(reason) => ...,
    }
});

After PR it's possible now to do:

notify_rust::handle_action(uid, |action: &notify_rust::ActionResponse| {
    match action {
        notify_rust::ActionResponse::Custom(key) => ...,
        notify_rust::ActionResponse::Closed(reason) => ...,
    }
});

@hoodie
Copy link
Owner

hoodie commented May 14, 2021

I hadn't realized this at the time, but why do you even use notify_rust::handle_action? there is .wait_for_action()
I was even thinking about deprecating the free function api, is there any good reason not to?

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 a pull request may close this issue.

2 participants