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

xilem_core: Provide a way to add View implementations for external types #402

Merged
merged 6 commits into from
Jun 24, 2024

Conversation

Philipp-M
Copy link
Contributor

@Philipp-M Philipp-M commented Jun 17, 2024

Since we're suffering from the orphan rule with the new xilem_core implementation, we cannot use View in downstream crates for external types such as a WidgetView implementation for &'static str in Xilem.

This PR adds an OrphanView trait which the Context of the View has to implement for the type which has an implementation in xilem_core for this. The View impl for these types is basically a proxy for OrphanView (which in turn has the same method signature as View), so downstream crates can achieve the same as with the View.

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.

It's not clear why you've added a std feature for this; none of the things behind it actually need it.

I'm also not convinced it's meaningful to have a View implementation for numbers. It's cute, but it's a bit weird imo.

xilem_core/src/views/orphan.rs Outdated Show resolved Hide resolved
xilem_core/src/views/orphan.rs Outdated Show resolved Hide resolved
impl_orphan_view_for!(String);
#[cfg(feature = "std")]
impl_as_orphan_view_for!(std::borrow::Cow<'static, str>);
// Why does the following not work, but the `Cow` impl does??
Copy link
Member

Choose a reason for hiding this comment

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

I presume this would be because we already have an impl for Arc<T>, and the compiler (for some reason) can't reason that str is not View.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, that makes sense, thanks for the hint.

@Philipp-M
Copy link
Contributor Author

It's not clear why you've added a std feature for this; none of the things behind it actually need it.

I guess I wasn't thorough enough and thought, these require std, even better if that's not necessary, I've removed the std feature.

I'm also not convinced it's meaningful to have a View implementation for numbers. It's cute, but it's a bit weird imo.

xilem_web actually already had support for numbers as Text node, apart from syntax-sugar and being slightly more optimized than using something like format!() or num.to_string(), I do think it can be useful in a more mathematical focused impl of xilem_core. I haven't looked into how viable xilem_core could be e.g. for that animatables experiment that I did in trui, but that's e.g. a use-case (i.e. animatable properties).

shows two different ways (unfortunately they aren't possible for the same type at the same time)

So at this point I'm leaning towards the more traditional approach with providing an actual View impl (in the form of OrphanView in the context of this PR), so I'm thinking about removing AsOrphanView in favor of OrphanView as it allows more control with just little bit more syntactical overhead. Maybe we can get similar behavior like as_view() somehow else in a more general way (without extra marker in View, because I don't think that gives good DX TBH).

@Philipp-M
Copy link
Contributor Author

I've removed the AsOrphanView, cleaned up a little, and added a "test" (more to show how it's used, as the implementation is trivial). When there's no objection that would be ready to merge/review now. The kurbo feature is used in the xilem_web rewrite already.

@Philipp-M Philipp-M marked this pull request as ready for review June 22, 2024 20:19
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.

The reason I really preferred the "AsView" style is that I think it's important that these views are only syntax sugar for an existing view, and are not the only way to access certain functionality. That is mostly because of extensibility, but also in terms of allowing people to make the meaning of their code clearer if needed, by migrating from the syntax sugar.

I do agree that for String specifically, that has issues around needing to clone the string for a short time, which is very unfortunate. But IMO, the answer there is to make non-'static views be supported.

That being said, this is probably a slightly more idiomatic actual implementation than the "AsView" style, and so I think we should have it this way. This is a really good trick to avoid the marker trait awkwardness the previous solution had (DJMcNab#4)

xilem_core/src/views/orphan.rs Outdated Show resolved Hide resolved
impl_orphan_view_for!(alloc::string::String);
impl_orphan_view_for!(alloc::borrow::Cow<'static, str>);

// number impls
Copy link
Member

Choose a reason for hiding this comment

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

I'm still not convinced that these are that meaningful, but I'm not going to block on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah apart from a little bit macro-expanded boilerplate (which I don't think should contribute too much to the compilation) doesn't hurt?
(In case we can also feature gate this, like the kurbo module I guess, would probably be interesting whether this meaningfully contributes to compilation time...)

I think it's cool to have something like (taken from xilem_web) p(("The answer to life is ", 42)) and it should be cheap as view, since equality checks are trivial.

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

"AsView" style is that I think it's important that these views are only syntax sugar for an existing view

Yep that's why I originally wanted to have both of these (and the full View equivalent to avoid allocs for more optimized views), but these unfortunately intersect...
I really hope we can find ways in general for something like this, because I think it's quite useful to avoid a full View impl. especially for end users not being familiar with all the specifics of the View trait.

Thanks for the review and the initial impl in DJMcNab#4 that inspired this.

@Philipp-M Philipp-M enabled auto-merge June 24, 2024 18:10
@Philipp-M Philipp-M added this pull request to the merge queue Jun 24, 2024
Merged via the queue into linebender:main with commit 5284082 Jun 24, 2024
8 checks passed
@Philipp-M Philipp-M deleted the orphan-views branch June 24, 2024 18:22
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