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

Rename BusProperties::build to BusProperties::from_message and auto implement conversion for most User Facing Event Types. #181

Open
luukvanderduim opened this issue Apr 23, 2024 · 5 comments · May be fixed by #206

Comments

@luukvanderduim
Copy link
Collaborator

Our UFETs implement BusProperties (GenericEvent) which provides:

	fn build(item: ObjectRef, body: Self::Body) -> Result<Self, AtspiError>
	where
		Self: Sized;

We define per-event how the UFET is constructed from (essentially) a Message because <T as BusProperties>::build is employed in from_dbus_message! only.

StateChangedEvent::build(msg.try_into()?, msg.body().deserialize::<StateChangedEvent::Body>()?)

Observe that most UFETs do not even need a body, in that case the second argument in the signature is underscored _body:

// extract header from Message and:
// validate interface and member with this traits consts, then:

let item: ObjectRef = msg.try_into()?;
Ok( Self {  item } )

This will reduce a number of macro expansions, remove impl_from_dbus_message, remove how build eludes to a builder pattern that is not there and simplifies most conversions by not needing to define them I think.

@TTWNO
Copy link
Member

TTWNO commented Apr 23, 2024

This sounds like a great plan! Should I do it? Or would you like to?

@luukvanderduim
Copy link
Collaborator Author

You're very welcome to make this happen. It would fit nicely in your new-generic-event-trait branch.

That being said, if you feel you are somewhat overburdened, I am happy to do it. I have time this week.

@luukvanderduim luukvanderduim changed the title Rename BusProperties::build to BusProperties::from_message and auto implement conversion for most User Facing Event Typess. Rename BusProperties::build to BusProperties::from_message and auto implement conversion for most User Facing Event Types. Apr 23, 2024
@TTWNO
Copy link
Member

TTWNO commented Apr 23, 2024

All good; I got a new keyboard this week, and I need to break it in!

@luukvanderduim
Copy link
Collaborator Author

In vein of our 'pair proramming' I took a longer look at the problem.
It does not seem possible to make the default method happen because there seems no way to tell the compiler that the target type indeed has this shape. I'd love to be wrong though..

Creating a type belongs to the type itself is the main takeaway I think.

What we can have is

  • a from_message method that is implemented by each UFET.

We could still reduce duplicate code somewhat by having

  • a default implemented 'validate_message(msg: &zbus::Message) -> bool` that:
	fn validate_msg(&self, msg: &zbus::Message) -> bool {
		let header = msg.header();
                // Safety: `from_str_unchecked` is fine because these are statically known values. 
		let member = MemberName::from_str_unchecked(Self::DBUS_MEMBER);
		let interface = InterfaceName::from_str_unchecked(Self::DBUS_INTERFACE);

		header.member() == Some(&member) && header.interface() == Some(&interface)
	}

If we end up just converting build( item: ObjectRef, body: Self::Body ) to from_message(msg: &zbus::Message) is fine too. I am eager to see what you come up with.

@TTWNO
Copy link
Member

TTWNO commented Apr 26, 2024

I think that there are some other issues that are not possible to fix due to compiler limitations. Not being able to apply a blanket implementation for TryFrom<FORIEGN_TYPE> is a significant issue; the only way around is to add another macro_rules! that automatically implements the base case of BusProperties given the const identifiers.

Would that suffice?

@TTWNO TTWNO linked a pull request Jun 28, 2024 that will close this issue
2 tasks
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