odilia-app / atspi

A fast, zbus-based, permissively licensed AT-SPI library written in pure Rust!
Apache License 2.0
28 stars 6 forks source link

`from_message_parts` into `from_message` #183

Open luukvanderduim opened 5 months ago

luukvanderduim commented 5 months ago

Builds on 'new-generic-event-trait'

from_message takes just one argument, &zbus::Message and converts into the BusProperties implementing type.

This avoids crreating a body in the cases we do not need one.

This also introduces a MessageExt trait that provides matches_event This allows users who have / prefer a MessageStream over the event_stream to do something like this:

    if msg.matches_event::<AbsEvent>()? {
        AbsEvent::from_message()?
    } else {
        Err(AtspiError::EventMisMatch)
    };

or simply use:

    AbsEvent::try_from(msg)?

Which now uses matches_event.

Lastly, this has a few little optimizations that avoid clones in a conversion.

TTWNO commented 4 months ago

Does it make sense to abstract away the nature of the mismatch? In my mind, it'd be much more useful to have specific descriptions of the mismatch, instead of having library users needing to add a bunch of debugging to figure it out.

Just my thoughts. Let me know.

TTWNO commented 4 months ago

Otherwise, this is a welcome change. I'm glad that a rename and redesign was considered.

luukvanderduim commented 4 months ago

Does it make sense to abstract away the nature of the mismatch? In my mind, it'd be much more useful to have specific descriptions of the mismatch, instead of having library users needing to add a bunch of debugging to figure it out.

Just my thoughts. Let me know.

I lean towards that a mismatch is not an error at all. If it matches 'false', nothing 'bad' happened, the outcome is just not what you want. The user registers for particular event(s) and deliberately msg.matches_event::<AttributesChangedEvent>(), right? Maybe you want to match against a different type or use a catch-all like msh.matches_event::<ObjectEvents>().

TTWNO commented 4 months ago

In the case of matches_event, I agree with you. My question is about try_from_message which only tells the user that a mismatch occurred, but not why it happened.

TTWNO commented 4 months ago

Hmmmm.... It looks like this breaks my goal of keeping common WASM compatible. I'm still thinking about this to see if it's strictly necessary, and if it is, how can we still integrate this.

I'm in favour of fewer lines of boilerplate code :)

luukvanderduim commented 4 months ago

Hmmmm.... It looks like this breaks my goal of keeping common WASM compatible.

Constructing errors breaks WASM? What rule should I not have broken?

I'm still thinking about this to see if it's strictly necessary, and if it is, how can we still integrate this.

What do 'it' and 'this' refer to? Greetings from the disambiguation dept!

I'm in favour of fewer lines of boilerplate code :) For the user?

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 96.44444% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 86.90%. Comparing base (280f3b2) to head (6833743).

Files Patch % Lines
atspi-common/src/macros.rs 82.05% 7 Missing :warning:
atspi-common/src/error.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #183 +/- ## ========================================== + Coverage 86.69% 86.90% +0.20% ========================================== Files 39 39 Lines 3338 3421 +83 ========================================== + Hits 2894 2973 +79 - Misses 444 448 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.