Fix macros to allow an arbitrary number of properties in Xilem views #1161
Conversation
|
Philip suggested using type composition instead, which we might adopt. This is more of a short-term fix. See discussion on zulip: #xilem > Refactor property system to use a composition property view. |
There was a problem hiding this comment.
This looks good - I'd like to see better handling of the new public API which this creates.
I think that having PropertyTuple still is reasonable - it ensures that the new methods have at least some basic docs and consistent semantics. It does need have its docs changed, at least, in this PR, and probably also to be renamed.
| type $Props = ($(Option<$Type>,)+); | ||
|
|
||
| #[expect(missing_docs)] | ||
| pub struct $Props($(Option<$Type>,)+); |
There was a problem hiding this comment.
Why is this pub? I don't think this type is going to be part of the public API of any widget, nor have any stability guarantees.
I think it should at least have some basic docs if you're going to make it so.
Or it should be expect(unreachable_pub), if it's for the associated type on Style
(it probably should have some basic docs anyway, even just mentioning that you should call rebuild_properties in rebuild and build_properties in build, and initialise it with Default)
There was a problem hiding this comment.
I think this comment still stands - I'd like to understand why this type is pub before approving. I see that you've added the requested docs - thanks!
There was a problem hiding this comment.
Oh, right, forgot to answer. Sorry.
It needs to be pub because it's used as an associated type for the various implementations of the Style trait, and you're not allowed to do that with a private type. We could do a pub-in-private trick or something similar, but I think just making it a pub item works for now.
Fixes the issues encountered in #1099.