-
Notifications
You must be signed in to change notification settings - Fork 145
xilem_core: Add a WithoutElements
ViewSequence
#608
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
xilem_core: Add a WithoutElements
ViewSequence
#608
Conversation
I haven't had chance to review this yet. One thing I'm wondering is whether we can make an alternative for |
Good point. You mean to avoid the extra wrapper, and directly use the no element views in sequences? At least something like this: impl<V: View<State, Action, Context, Message, Element = NoElement> + NoElementViewMarker, ...>
ViewSequence<State, Action, Context, Element, Message> for V {} is not possible, because of ambiguity again... (We would need something like negative trait bounds?) The other possible idea I tried, is directly implementing the I'm not sure if it's worth it, but we could remove the super trait requirement Or do you mean something else? |
I mean, I was especially saying that we should do that investigation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A docs nits, but otherwise I think it's better to land this than not.
Sorry it's taken so long to review; this hasn't been on my critical path.
Hmm now I almost forgot about this myself. I merged main into here and fixed the conflicts, as some time went by, maybe give it a quick review again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. I've not run any manual tests
@@ -72,11 +71,11 @@ where | |||
/// } | |||
/// ``` | |||
pub trait WidgetViewSequence<State, Action = ()>: | |||
ViewSequence<State, Action, ViewCtx, Pod<DynWidget>> | |||
ViewSequence<State, Action, ViewCtx, Pod<dyn Widget>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that this change has no other impacts; it seems a bit random to have in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I've included it here, as I was testing it with returning a WidgetViewSequence
but that didn't work IIRC, as sequences are using dyn Widget
instead of DynWidget
which is used only within AnyWidgetView
. I don't exactly remember anymore what did happen, but DynWidget
cause(d/s) problems.
Currently WidgetViewSequence
is not really used, which is why it doesn't seem to have an effect, and unfortunately it's not possible to mix with other view sequences like FlexSequence
.
I guess it could also be removed, as we're only using "specialized" view sequences.
I think I'll leave it in here for now, and delay further changes (removal?) to follow-up PRs, I think in this form it's slightly more correct.
Co-authored-by: Daniel McNab <[email protected]>
This is similar as the
fork
view, but it allows usingView
s/ViewSequence
s without elements within aViewSequence
context that contains elements, see #461 (comment) for the original motivation.Taking
xilem_web
as example, this should be possible (as well as something similar as in the linked comment):The
WidgetViewSequence
is "fixed" as well, by using the "right" widget type (DynWidget
is only used inAnyView
, whereas I thinkBox<dyn Widget>
is more general and correct).