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

from_message_parts into from_message #183

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

luukvanderduim
Copy link
Collaborator

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.

Base automatically changed from new-generic-event-trait to main May 8, 2024 16:45
@TTWNO
Copy link
Member

TTWNO commented May 8, 2024

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
Copy link
Member

TTWNO commented May 8, 2024

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

@luukvanderduim
Copy link
Collaborator Author

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
Copy link
Member

TTWNO commented May 9, 2024

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
Copy link
Member

TTWNO commented May 16, 2024

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
Copy link
Collaborator Author

luukvanderduim commented May 16, 2024

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?

Copy link

codecov bot commented May 17, 2024

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 ⚠️
atspi-common/src/error.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

luukvanderduim and others added 8 commits May 23, 2024 12:44
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:

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

or simply use:

```Rust
	AbsEvent::try_from(msg)?
```

Which now uses `matches_event`.

Lastly, this has a few little optimizations that avoid clones
in a conversion.
We tried to re-export zbus for convenience, if the user wished so but did not h ave the corresponding feature in Cargo.toml.

This fixes that.
This thanges `BusProperties::from_message` to `try_from_message` to better
hint at what it returns.

Also fixes a dependency problem Clippy found in crate atspi, where we tried
to re-export zbus (but failed) because we did not have the `zbus` feature
present in that crate. This add zbus as optional dependency to that crate.
…nition

This introduces more verbose specific & verbose on why the conversion from
`Message` failed.

Only disadvantage is perhaps that we need to use the error type `Owned`,
because we have to construct the error from runtime gathered parts.
We cannot compile zbus in WASM context, so this branch's use of `Message` in
the `BusProperties::try_from_message` method is problematic.

Because `BusProperties` is used to blanket-implement some other traits, this
commit tries to be least intrusive and avoids having `try_from_message`
in the trait when zbus is not set as feature.

The obvious downside is that `BusProperties` now has two sets of behaviour,
depending on the "zbus" feature.
That being said, `try_from_message` is not relevant anyway without zbus support.
Fixes `ObjectRef` type path in `atspi-common`
Fix docs
@TTWNO TTWNO force-pushed the new-generic-event-trait-ext branch from 08e6f49 to 6833743 Compare May 23, 2024 18:45
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.

2 participants