add notification reading support #125
Conversation
…n integration test to make sure it works
…s interface, which I thought was there because I was misreading the spec
…ng notifications, which is again a signatures mismatch
…geStream type already implements FusedStream because the library shows that it maintains the required invariants itself by implementing that trait on the type exposed through its public API, we can safely assume that we don't need to call that method our selves, in fact we could even be introducing overhead of our own by doing that, so both the call and the comment was removed.
…g code is easier to write and understand this made the code in the latter half of the function more readable, however now there is a problem with esthetics, because the code to obtain the message from the Arc<Message> involves a function from the standard library, and furthermore it can fail, but not with the error type I expect, so I had to unwrap it for the moment, which is not good because I use the ? operator everywhere else the code contains a comment about this, as well as a reminder to fix it later, if I have the ability or an idea to do so
…different files this makes it easy to implement future refactorings, because the code is properly scoped
…g reason the test was hanging because a while loop was used to fetch all the notifications indefinitely, even if the assertions were successful the test would still not terminate furthermore, the call to the freedesktop notifications proxy is commented out, because it made the test fail due to incorrect signatures, even though the notification was shown by my desktop environment's notification daemon. this is pending on taking a dev dependency for a crate which sends notifications correctly
we apparently don't get the name lost signal in the stream, perhaps because we register the rule early enough, I'm not sure. If anyone uses a different dbus implementation and the test failes for you, do notify me of that, as in that case it's an implementation difference and more investigation would be required As such, skipping one *valid* notification signal from the stream, thinking it's not valid, does nothing more than cause the tests to hang because two notifications are required for the stream to return the first item, so that call is no longer there
…ify the test accordingly the notify-rust crate is now a transitive dependency of odilia-notify, used for the integration test furthermore, the test now correctly sends a notification and should recieve it properly
before, the test panicked because the show method tryed to show a notification by starting an async executor, at least on linux and bsd, where this is relevant. now, show_async is being called and awaited, ensuring that the same executor the test is started in is being used for the notifications as well
- Adjusts `Action` fields to correspond with what I understand specs say about
`Notify` method call arguments.
See: [Notification specifications]():
Actions are sent over as a list of pairs. [...] the identifier for the
action and he localized string that will be displayed to the user.
Previous `Actions` fields (name, method) are meant for the invocation
of the `Action` (?).
- Remove `RawNotifySignature<'a>`
- Introduce `NotifyArgs` struct that holds and names the arguments to
the `Notify` method call we are monitoring for.
- Make sure to derive `Type` on `NotifyArgs`.
- Adjust `TryFrom<Message> for Notification` to
simplify deserialize and deconstruct the `Message::body`.
Since we have the `NotifyArgs` struct and `NotifyArgs` has the
`Type + Deserialize` traits, we can now simply
```Rust
let notify_args = msg.body::<NotifyArgs>body()?;
```
and since we are interested in just a few fields, we can selectively
decunstruct in one go:
```Rust
let NotifyArgs { app_name, summary, body, actions, .. } = msg.body()?;
```
(- Format lib.rs)
Test passes. Cherry-pick whatever you like.
Seems to work.
Make things lean. - Removes `Action`s altogether. - Remove Notify, - Simplifies `Notification` - Simplifies `TryFrom<Merssage> for Notification` - Adjust test, since the actions field is no longer there. The types we do not need are references in our signature in the hope that, should Serde deserialize these into a temporary, these are zero-copy, borrowed types before these are dropped.
the following things were done with this merge since it was opened, which deviated at my request from the original purpose * simplify the whole API * remove cruft from the features I thought were needed at the beginning of making this crate
…he other way around there is now a new type, called NotifyError, which will unite all the errors encountered in the operation of the function. For now, it only has variants for dbus specific errors, in this case zbus::Error and zbus::fdo::Error the signature of the function inside the library has been changed, from `impl Stream<Item=Result<Notification...>>`, to `Result<impl Stream<Item=Notification>, NotifyError> some code comments are around the return value of the function though, because there is something strange going on with pinning currently, and tests would error out, so basically the API, as given, would be unusable without that. This is only a temporary solution, hopefully. Even if not, the cost of pinning that stream in memory is relatively small all things considered
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
==========================================
+ Coverage 14.30% 16.08% +1.78%
==========================================
Files 17 20 +3
Lines 1559 1616 +57
==========================================
+ Hits 223 260 +37
- Misses 1336 1356 +20 ☔ View full report in Codecov by Sentry. |
|
apparently, the clippy test is failing because of something this pr didn't touch at all, so other prs should have failed on this as well. Should I merge this now? opinions? |
… fix spelling and make code more idioma * Remove need for `Deref` by using `.as_str()` because it's clearer to the reader, and more rust idiomatic * Avoid extraction from `Arc` by `TryFrom<Messages>` into `TryFrom<Arc<Message>>`. As a bonus, this will return a notification even if the strong count inside the `Arc` is greather than one, mitigating the possibility that we might lose notifications if the bus is under heavy contention * 'Recieve' is spelt 'receive': fix comment and rename test file.
merge a pull request for odilia-notify and reflect it here as well
this allows the clippy CI check to pass, and is therefore required.
main.rs imported std::string::String, even though that type was subsequently not used. Therefore, I removed that import, in order to get clippy to not error
most applications don't provide anything in the app_name field, so it is preferable that odilia doesn't try to make something up, which is mostly the empty string in my experience the long-term goals are to provide such information, but for now that's not possible of interest is the org.gtk.notifications interface, which provides an application id, but that's internal to gnome for one, and for another, I think most of the notifications recieved that way are not ment for public consumption, and are ment more like a publisher/subscriber system for gtk apps to talk to one another. Research needed in this area
|
Should this really be implemented by sniffing the bus? I was under the assumption that notification daemons will put information in a live area that screen readers will pick up? Advantage of this approach is you don't need a notification daemon (or rather, that your screen reader becomes the notification daemon), but the downside is that if you're using an existing (and accessible) notification daemon, then you will be getting data from both sources. Do you have plans to mitigate this problem? |
|
@TTWNO well, it's not precisely a specific accessible region where demons put their notifications. In stead, it's another orca hack, which, as far as I was able to read, envolves monitoring if a new window appeared and its role is alert, or notification, or something like that, I'm a bit fuzzy on the details |
Oh I'm not worried. For better or worse, DBus, and XDG portals are becoming the standard. So fair enough, I get your point. I never knew this was another Orca hack 😆 Tried it out on my local machine, it it works fine... I want to see a better criticality and app indicators (since right now it does not do either). Thoughts? I don't need anything advanced, but it might be nice to get the app name, criticality level (if supplied), then also say the text. |
this makes this pr mergeable into main again, and furthermore adds cli options because of the work already merged on main
|
@TTWNO if you want, you can look at my previous commits, where I tryed doing that. The commit where I changed it is clearly labeled, and the description says that most apps, especially gtk ones, don't provide anything but an empty string in the app_name argument, so we don't know exactly where a notification comes from, maybe try app_icon instead? more experimentation is needed, and feel free to reintroduce that bit of code if you feel like testing with different toolkits. About urgency, I dk, is that even listed in the spec? |
…orkspace configuration * a new unit test was added inside the notification module of odilia-notify. This checks that for a given manually built message, the expected answers are returned * codecov was complaining before because the tests inside odilia-notify were never ran inside CI, which would cause it to conclude that I introduced coverage wholes. Even though I added another test, this wouldn't have been fixed anyway * therefore, I added odilia-notify to the default-members list of the root workspace file, insuring that tests run for this one as well * I also changed the resolver of the workspace to the newer one, because cargo was warning me every time, so that's fixed as well * II added afew common dependencies to the workspace proper, replacing them from odilia-notify. Hopefully, now build times should be significantly smaller
pulls in the relevant CI, readme and contributing changes from upstream
… all tests successfully as well
this uses the odilia-notify crate in order to provide odilia with the ability of monitoring the bus, and then hooks the result in the speech framework
the code isn't currently the most robust, but it does the job relatively well
for example, it seemns that many apps don't provide a non-empty string for app_name, a thing this currently doesn't handle