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

Reserve ability to create Pod to ViewCtx #597

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Conversation

PoignardAzur
Copy link
Contributor

This is another intermediary PR for the "restrict widget creation to Mutate pass" feature.

The basic idea is that, in the near future, it will be impossible to create a WidgetPod without a handle to the arena. In my current WIP code, that handle is passed through ViewCtx. Therefore, this PR changes all the sites in xilem that create a Pod and has them use a ViewCtx method instead.

I've tested most xilem examples and they all worked (except for variable_clock, which currently panics in the last main commit).

@PoignardAzur
Copy link
Contributor Author

Note for reviewers: the most interesting changes are in:

Everything else is plumbing those API changes into method implementations.

@Philipp-M
Copy link
Contributor

Oh, well, I wanted to start a (zulip-)topic already about having the ViewCtx "everywhere", good to see that we're converging on similar ideas I guess.
I have started something very similar here already (I haven't access to my PC where I've implemented it, I forgot to push it, I'll link to it when I have access later), it's basically about:

Have access to ViewCtx everywhere where the element is forwarded/created in Xilem (that means in SuperElement as done here, and as well in ElementSplice which I think will be useful for keyed viewsequences as well as more efficient tree updates in xilem_web)

Copy link
Contributor

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

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

So this is my branch, looks quite similar, different ordering of the parameters, and access to the ViewCtx in SuperElement::with_downcast_val, I guess that can be merged somewhat.
But since your PR is more targeted, probably yours first, I'll do that afterwards.
I think generally it makes sense to also have access to the context in SuperElement::with_downcast_val, but it introduces a little bit complexity, though I expect this to be useful (I'm experimenting with this currently in xilem_web).

except for variable_clock, which currently panics in the last main commit

I think you should open an issue then, it works for me (on this branch as well as main).

Otherwise looks good to me.

@PoignardAzur
Copy link
Contributor Author

I think you should open an issue then, it works for me (on this branch as well as main).

It's fixed now.

@PoignardAzur PoignardAzur added this pull request to the merge queue Sep 18, 2024
Merged via the queue into main with commit 2521829 Sep 18, 2024
17 checks passed
@PoignardAzur PoignardAzur deleted the remove_pod_new branch September 18, 2024 20:59
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