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

Implement AnyView in xilem_masonry #206

Merged
merged 3 commits into from
Apr 25, 2024
Merged

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Apr 25, 2024

xilem_masonry_anyview.webm

) -> ChangeFlags {
// TODO: Does this need to have a custom view id to enable events sent
// to an outdated view path to be caught and returned?
// Should we store this generation in `element`? Seems plausible
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really decoupled from the widgets then, is it? (Which AFAIK is necessary, for views not being wired up directly with widgets but being able to receive events from somewhere else, async list is an example for that (via the waker that stores the current view id path)).
I think it's one of the reasons why View::build returns a ViewId, and rebuild is able to change it (e.g. in this method, because there's a new view).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow how the suggestion in here would break views not directly hooked up to widgets.

You are right that this does need to resolved before merging though. The correct place for this state to go is actually in View::State, but that doesn't exist yet. The element is a Xilem specific widget, so it's fine-ish to include a simple counter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this does need to happen in View::State.

This isn't actually problematic because we don't have async wiring yet, but the current state is wrong

crates/xilem_masonry/src/any_view.rs Outdated Show resolved Hide resolved
crates/xilem_masonry/src/any_view.rs Outdated Show resolved Hide resolved
@DJMcNab
Copy link
Member Author

DJMcNab commented Apr 25, 2024

This is in a far from perfect state, and it has confirmed that we need to bring back the State field/type on MasonryView (entirely because of our future async plans, but doing so later would cause a lot more pain).

But we're merging now despite that to unblock work

@DJMcNab DJMcNab added this pull request to the merge queue Apr 25, 2024
Merged via the queue into linebender:main with commit 25d4a8c Apr 25, 2024
7 checks passed
@DJMcNab DJMcNab deleted the masonany branch April 25, 2024 16:20
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.

3 participants