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

Support arrays ([impl ViewSequence; N]) as ViewSequences #175

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

Philipp-M
Copy link
Contributor

Adds the blanket impl
impl<const N: usize, VT: ViewSequence> ViewSequence for [VT; N]

Allows for example something like this:

fn my_tab(impl View) -> impl View {..}
h_stack(["Tab 1", "Tab 2", "Tab 3"].map(my_tab))

@Philipp-M
Copy link
Contributor Author

Well I guess I'm getting too used to using unstable Rust ([].each_ref() is unstable in 1.76). But fortunately as I've just seen, the corresponding feature was just stabilized 3 weeks ago. So we can just wait for the next Rust version (1.77) and then the feature would be supported (when we don't want to try to keep MSRV low anyhow).

Otherwise this needs I think a more dynamic solution (State = Vec<VT::State>).

@Philipp-M Philipp-M marked this pull request as draft February 16, 2024 23:52
@zoechi
Copy link

zoechi commented Feb 20, 2024

Your comment made me curious and I looked into [].each_ref(), but I wasn't able to figure out what the benefit over for example [].iter().map(..m) is, especially in this case where the created array of references isn't kept around afterwards.
What do you think is the benefit?

@Philipp-M
Copy link
Contributor Author

each_ref allows to create a different array with the same size ([T; N].each_ref() -> [&T; N].map() -> [S; N]).
The difference would be that Vec needs an allocation, while [T; N] does not. I don't expect much of a practical difference, and it could well be done with a Vec (via iter().map().collect()) instead (but I think it's a little bit cleaner with a static array).

@zoechi
Copy link

zoechi commented Feb 20, 2024

I see. Because the resulting array is not used (as far as I see), there also would be no Vec and collect() necessary.
My approach would be [].iter().for_each(|x| ...).
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2ad60536d7f403943d34758c29a01083
It's a bit more verbose though.

@Philipp-M
Copy link
Contributor Author

Philipp-M commented Feb 20, 2024

Note the function signature of ViewSequence::build(&self, cx: &mut Cx, elements: &mut Vec<Pod>) -> Self::State Which returns the view state.
So you need to create and return an owned container, it's not enough to just iterate over the child view sequences and invocate build().

It is used in every rebuild and message invocation, where it is handed by the parent/owning view sequence.

@zoechi
Copy link

zoechi commented Feb 20, 2024

Now I get it. I thought I checked 3 times if the result is returned and I didn't see it🤦🏼‍♂️

@Philipp-M Philipp-M marked this pull request as ready for review March 25, 2024 14:42
@Philipp-M
Copy link
Contributor Author

The relevant feature is now in stable rust. So this is ready. We could increase the Rust version in a separate PR as well...

@DJMcNab
Copy link
Member

DJMcNab commented Jun 17, 2024

Sorry, this probably has suffered the same fate at the hands of #310 as the other PRs

@Philipp-M Philipp-M force-pushed the array-viewsequence branch from c462657 to bdcd009 Compare June 23, 2024 17:34
@Philipp-M
Copy link
Contributor Author

I've updated this to the new xilem_core. Hmm I hope this doesn't suffer the same fate again though...
I copied some of the tests from the tuple sequence, since they're seemingly equivalent from what needs to be tested.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

A few "prose" nits, but otherwise this looks good.

I'm not really sure how useful this is, but it also doesn't cost us anything to support.

xilem_core/src/sequence.rs Outdated Show resolved Hide resolved
xilem_core/src/sequence.rs Outdated Show resolved Hide resolved
xilem_core/src/sequence.rs Outdated Show resolved Hide resolved
xilem_core/src/sequence.rs Outdated Show resolved Hide resolved
xilem_core/src/sequence.rs Outdated Show resolved Hide resolved
@Philipp-M
Copy link
Contributor Author

I'm not really sure how useful this is, but it also doesn't cost us anything to support.

Yeah I think it's mostly useful to avoid the more elaborate (and inefficient) version with Vec and small "hardcoded" arrays with the example in the comment in the PR description. But probably really not super-important to have, though I think we should support all (builtin) data structures that make sense as a ViewSequence.

@Philipp-M
Copy link
Contributor Author

Thanks for the quick review, I'm merging.

@Philipp-M Philipp-M enabled auto-merge June 24, 2024 09:37
@Philipp-M Philipp-M added this pull request to the merge queue Jun 24, 2024
Merged via the queue into linebender:main with commit fc96f06 Jun 24, 2024
8 checks passed
@Philipp-M Philipp-M deleted the array-viewsequence branch June 24, 2024 09:46
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