Move `xilem` onto a new `xilem_core`, which uses a generic View trait #310

DJMcNab · 2024-05-20T15:51:49Z

This:

Renames the current/old xilem_core to xilem_web_core and moves it to the xilem_web/xilem_web_core folder
Creates a new xilem_core, which does not use (non-tuple) macros and instead contains a View trait which is generic over the Context type
Ports xilem to this xilem_core, but with some functionality missing (namely a few of the extra views; I expect these to straightforward to port)
Ports the mason and mason_android examples to this new xilem, with less functionality.

This continues ideas first explored in #235

The advantages of this new View trait are:

Improved support for ad-hoc views, such as views with additional attributes.
This will be very useful for layout algorithms, and will also enable native good multi-window (and potentially menus?)
A lack of macros, to better enable using go-to-definition and other IDE features on the traits

Possible disadvantages:

There are a few more traits to enable the flexibility
It can be less clear what Self::Element::Mut is in the rebuild function, because of how the resolution works
When implementing View, you need to specify the context (i.e. impl<State, Action> View<State, Action, [new] ViewCtx> for Button<State, Action>.

xilem_core2/src/element.rs

DJMcNab

Some thoughts which might be helpful for reviewers

masonry/src/action.rs

xilem/src/any_view.rs

xilem/src/view/button.rs

xilem/src/view/flex.rs

DJMcNab · 2024-05-24T20:43:28Z

xilem/src/view/flex.rs

+        let ret = f(child);
+        self.element.remove_child(self.ix);
+        // Also remove the implicit spacer
+        // TODO: Make the spacers be explicit?


I wonder if we want two forms of this?

I lean towards only one, and more explicit, we may want some good helper views/types then probably.

This will probably be a follow-up PR

DJMcNab · 2024-05-24T20:50:48Z

xilem_core/src/message.rs

-    /// state remained the same. It is expected that this type won't be used very
-    /// often.
-    #[allow(unused)]
+    /// This allows for sub-sections of your app to use an elm-like architecture


How useful is this really?

I'm not sure yet about it exactly, but I guess it (also) allows to modularize views, possibly in external libraries. Maybe we don't need it, when we have found a satisfying solution for local/modularized state.

DJMcNab · 2024-05-24T20:51:45Z

xilem_core/src/message.rs

-            MessageResult::Stale(event) => MessageResult::Stale(event),
-            MessageResult::Nop => MessageResult::Nop,
-        }
+impl<T> Message for T


This has some footguns around e.g. Box(Message) also implementing these same methods

Yes that's indeed suboptimal...

Having Message as generic View param has certainly some advantages, since I have the feeling that View will not really be used directly unless for implementing it for types maybe worth exploring? But yeah, yet another additional generic parameter...

DJMcNab · 2024-05-24T20:52:21Z

xilem_core/src/sequence.rs

@@ -1,363 +1,650 @@
-// Copyright 2023 the Xilem Authors
+// Copyright 2024 the Xilem Authors


Not sure about these year changes; they're mostly accidental

I mean most of the code here is rewritten, so 2024 makes sense? But maybe we also want to have year-ranges to be more accurate.

xilem_core/src/sequence.rs

Philipp-M

I've added a first round of comments, I'll go deeper into this later.

Philipp-M · 2024-05-25T10:42:45Z

xilem_core/src/message.rs

-    /// state remained the same. It is expected that this type won't be used very
-    /// often.
-    #[allow(unused)]
+    /// This allows for sub-sections of your app to use an elm-like architecture


I'm not sure yet about it exactly, but I guess it (also) allows to modularize views, possibly in external libraries. Maybe we don't need it, when we have found a satisfying solution for local/modularized state.

Philipp-M · 2024-05-25T10:43:10Z

xilem_core/src/sequence.rs

@@ -1,363 +1,650 @@
-// Copyright 2023 the Xilem Authors
+// Copyright 2024 the Xilem Authors


I mean most of the code here is rewritten, so 2024 makes sense? But maybe we also want to have year-ranges to be more accurate.

xilem_core/src/sequence.rs

Philipp-M · 2024-05-25T10:44:41Z

xilem_core/src/sequence.rs

+        prev: &Self,
+        seq_state: &mut Self::SeqState,
+        ctx: &mut Context,
+        elements: &mut impl ElementSplice<Element>,


I suspect this makes the trait "object-unsafe".
Just noting, I see ElementSplice is already not object-safe, I remember working around that, to make it possible to use dyn, but maybe it's not necessary to have this object-safe.

Yeah - it would be nice to have an AnyViewSequence (and we'll probably need it for hot reloading?), but it should still be a relatively straightforward adaptation.

PoignardAzur

I don't think we've really nailed "React architecture in Rust" yet, but this PR gets us one step closer.

xilem_core/src/sequence.rs

xilem_core/src/views/memoize.rs

xilem_core/examples/filesystem.rs

xilem/examples/flex.rs

xilem/examples/mason.rs

xilem_core/src/views/memoize.rs

xilem_core/examples/filesystem.rs

Blocking merge on tests; will need review again once those are added

Philipp-M

Phew... not the easiest to digest code, but indeed better than #235 (when it can cover the functionality introduced there without the reborrowing)
I do think the added complexity is worth it though to avoid the macros and being more flexible with different view contexts.
And it seems, that not every contributor of xilem does need to understand all this code for (non-core) contributions...
I hope, that due to all the added typing, composing views will not let the compiler explode (in compilation time mostly).

Great, well-thought work!

I don't think we've really nailed "React architecture in Rust" yet

I'm not sure we even want that, having to deal with a lot of React ugliness in my job currently (e.g. useEffect or magic (unwanted) reconciliations).
I'm not the biggest fan of distributed "reactive" state as it's done in React (nor the "centralized state" solutions that exist there)...
We still want to find a satisfying and simple solution for modularized views and state anyway, to support convenient library crates for (composed?) views. (but that's orthogonal to this PR)

xilem/src/lib.rs

xilem/src/any_view.rs

Philipp-M · 2024-05-30T11:17:27Z

xilem/src/view/flex.rs

+        let ret = f(child);
+        self.element.remove_child(self.ix);
+        // Also remove the implicit spacer
+        // TODO: Make the spacers be explicit?


I lean towards only one, and more explicit, we may want some good helper views/types then probably.

xilem_core/src/views/memoize.rs

xilem_core/src/element.rs

xilem_core/examples/user_interface.rs

xilem_core/src/element.rs

Philipp-M · 2024-05-31T15:13:06Z

xilem_core/src/message.rs

-            MessageResult::Stale(event) => MessageResult::Stale(event),
-            MessageResult::Nop => MessageResult::Nop,
-        }
+impl<T> Message for T


Yes that's indeed suboptimal...

Having Message as generic View param has certainly some advantages, since I have the feeling that View will not really be used directly unless for implementing it for types maybe worth exploring? But yeah, yet another additional generic parameter...

xilem_core/src/view.rs

Philipp-M · 2024-05-31T15:42:51Z

xilem_core/src/view.rs

+/// In order to support the open-ended [`DynMessage`] type, this trait requires an
+/// allocator to be available.


Another reason to make it generic then I guess.

The allocator story isn't great, but a lot of views need it - i.e. sequences and AnyView

I see, in that case we may want to wait for Allocator traits etc. anyways before committing to something like that.
I don't think there are good ways for these cases without allocators?

xilem_core/src/sequence.rs

Philipp-M · 2024-06-02T09:44:49Z

xilem_core/src/sequence.rs

+#[doc(hidden)]
+pub struct WasAView;
+
+impl<State, Action, Context, V, Element> ViewSequence<State, Action, Context, Element, WasAView>


Suggested change

impl<State, Action, Context, V, Element> ViewSequence<State, Action, Context, Element, WasAView>

impl<State, Action, Context, V, Element> ViewSequence<State, Action, Context, Element, fn() -> WasAView>

Otherwise it's e.g. not possible to Clone a ViewSequence (I just ran into that).
Alternatively we could also do

#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Default, Copy)] pub struct WasAView;

It's not clear to me how this impacts Clone. Can you explain please?

Consider all the views would implement Clone (which I have tested with either xilem_web and xilem right now, they currently do not, probably something for a follow-up PR, because I don't see a reason against implementing all these derive traits).

This doesn't work without this addition:

fn my_clonable_view() -> impl WidgetView<()> + Clone { flex((button(...), button(...))) }

Oh, so it's a perfect derive issue?
I'd say that you should just implement Clone for Flex manually; it's a logical mistake that Flex: Clone would require Marker: Clone, since that implementation is never used - working around it here isn't very principled.

That the same issue arises with Debug is unfortunate, and was a hazard I didn't see before. Overall, I'm happy to add the Debug derive here, but I'm going to push back on the fixes to make Clone work like that here.

Philipp-M · 2024-06-02T11:40:26Z

xilem_core/src/sequence.rs

+    /// Insert a new element at the current index in the resulting collection.
+    fn push(&mut self, element: Element);


Do we still need this, when there's with_scratch (which I'm not super happy with the naming, maybe it should be called append or this should be the new push instead)?
If we do keep this, it should either push at the end (this was the behavior before IIRC) or be called insert as the doc comment already implies IMO.

I think it's needed to avoid additional e.g. allocations when they're not needed.

It's also a bit simpler to use. Although really, manual implementation of ViewSequence seem so unlikely that it wouldn't be unreasonable to remove this

Philipp-M · 2024-06-02T16:34:51Z

xilem_core/src/message.rs

+// The `View` trait could have been made generic over the message type,
+// primarily to enable flexibility around Send/Sync and avoid the need
+// for allocation.
+pub trait Message: 'static + Send {


I'm currently running into the issue while rewriting xilem_web event listeners, that this requires Send. web_sys::Event doesn't implement Send.

So the Send bound should be optional (another reason to make it generic?).

Philipp-M · 2024-06-02T19:32:20Z

Hmmm, I just discovered one major issue because of the orphan rule (in hindsight it's obvious):
The trait can not be implemented for foreign types in downstream crates, and I think this is somewhat a dealbreaker for nice API design, following e.g. is not possible:

impl<State, Action> View<State, Action, ViewCtx> for &'static str {..}

Philipp-M · 2024-06-02T20:53:28Z

The trait can not be implemented for foreign types in downstream crates

I mean I'm honest, I really don't like it, as it convolutes everything with wrapper types such as text("text") or number(123), but I guess it will be difficult to have different implementations in one project anyway (e.g. we have a masonry text node and a web text node and want to use it in the same project (after a potential merger of those crates), e.g. in an embedded web-view).

I'm asking myself, if there's a general way to provide such an implementation in core and have different contexts define which implementation/element is used or something like that... But I'm not sure if that's possible... That makes this even more relevant

DJMcNab · 2024-06-03T15:15:24Z

Those general implementations can be added, as shown in DJMcNab#4

They do lead to some jankiness however. In particular, you can't have:

fn test()->impl WidgetView<...> {
    "my string"
}

because of the new required marker

Continues linebender#235

* xilem: Port rest of examples and add `Memoize` to `xilem_core` * cargo fmt * Address review comments * Add License header

…iewElement::Mut` It's not possible to modify the element otherwise, after descendent `View`s have used the element in `View::rebuild`

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

xilem_core: return element in `View::rebuild`

Philipp-M

Great work, certainly a big improvement in code quality.
Nice job also with all those exhaustive tests (seems like they surfaced some issues as well?)

xilem_core/src/any_view.rs

Philipp-M · 2024-06-05T18:02:26Z

xilem_core/src/message.rs

@@ -17,6 +17,8 @@ pub enum MessageResult<Action> {
    /// This allows for sub-sections of your app to use an elm-like architecture
    Action(Action),
    // TODO: What does this mean?
+    /// This message's handler needs a rebuild to happen.
+    /// The exact semantics of this method haven't been determined.


Well, it has been, but that was when we had async in xilem, it was used when a future has completed, sent a message and in View::message a rebuild was requested (e.g. when a list view entry in the async list has finished loading)

But my question is how is that different from the implied result of an action handler at the root?

The semantics here are clearly not very principled

You mean via MessageResult:Action?

Yeah, exactly.

DJMcNab mentioned this pull request May 22, 2024

xilem_masonry: Create a generic View trait, and use it to complete the Flex view #235

Closed

DJMcNab force-pushed the core-two branch from 24b482e to 095439f Compare May 23, 2024 08:17

richard-uk1 reviewed May 23, 2024

View reviewed changes

xilem_core2/src/element.rs Outdated Show resolved Hide resolved

DJMcNab marked this pull request as ready for review May 24, 2024 16:33

DJMcNab force-pushed the core-two branch from 4d1a28a to a79141b Compare May 24, 2024 16:42

DJMcNab changed the title ~~Investigation into a more generic view trait~~ Move xilem onto a new xilem_core, which uses a generic View trait May 24, 2024

DJMcNab commented May 24, 2024

View reviewed changes

Philipp-M reviewed May 25, 2024

View reviewed changes

DJMcNab mentioned this pull request May 26, 2024

xilem: Port the rest of the existing widgets and the mason example DJMcNab/xilem#1

Merged

PoignardAzur previously approved these changes May 30, 2024

View reviewed changes

xilem_core/src/sequence.rs Outdated Show resolved Hide resolved

xilem_core/src/views/memoize.rs Show resolved Hide resolved

xilem_core/examples/filesystem.rs Show resolved Hide resolved

DJMcNab commented May 31, 2024

View reviewed changes

xilem/examples/flex.rs Show resolved Hide resolved

xilem/examples/mason.rs Outdated Show resolved Hide resolved

xilem_core/src/views/memoize.rs Show resolved Hide resolved

xilem_core/examples/filesystem.rs Show resolved Hide resolved

Philipp-M approved these changes May 31, 2024

View reviewed changes

Philipp-M reviewed Jun 2, 2024

View reviewed changes

xilem_core/src/sequence.rs Outdated Show resolved Hide resolved

Philipp-M reviewed Jun 2, 2024

View reviewed changes

DJMcNab added 10 commits June 3, 2024 16:21

Investigation into a more generic view trait

39f1bc5

Continues linebender#235

Start some documentation

343f790

Push in-progress non-compiling changes

f7e528a

Finish the non-sequential part of xilem_core2

4c5a3ec

Add missing copyright header

0b38488

Doc improvements

540d0b5

Add an example using a filesystem

5428768

Start porting ViewSequence

c67aba9

Add the sequence implementation

a8ae1c7

Clippyification

95317c4

DJMcNab and others added 4 commits June 3, 2024 16:24

Fix docs in element.rs

afc7c7e

xilem: Port rest of examples and add Memoize to xilem_core (#2)

7e4e73d

* xilem: Port rest of examples and add `Memoize` to `xilem_core` * cargo fmt * Address review comments * Add License header

Add and use Pod::new

7c56c5b

Address review comments

e9ea5f9

DJMcNab force-pushed the core-two branch from 1030a67 to e9ea5f9 Compare June 3, 2024 15:25

DJMcNab and others added 11 commits June 3, 2024 16:34

Fix changes murdered by poor merge conflicts UI

0ef7621

Add some super basic docs

1c17017

Add some initial tests

01107f5

Start working on sequence tests

f404fd5

Move comment onto its own line

e3adff4

xilem_core: return element in View::rebuild, and Mut alias for `V…

62a7e05

…iewElement::Mut` It's not possible to modify the element otherwise, after descendent `View`s have used the element in `View::rebuild`

wording

708b9ab

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

better doc comment for View::rebuild

d478bf2

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Consistent naming for Mut<'el, Self::Element>

1bc36c1

typo

c763e09

Merge pull request #3 from Philipp-M/xilem_core-rebuild-return-element

6fa0c55

xilem_core: return element in `View::rebuild`

Philipp-M mentioned this pull request Jun 4, 2024

Provide orphan rules compatible impls DJMcNab/xilem#4

Closed

DJMcNab added 5 commits June 5, 2024 10:54

Some sequences

aeab9e3

Add tests for non-vec sequences

afbddf6

Finish sequence tests and fix Vec behaviour

1874d5d

Don't change copyright years

bbbb7cd

Fix missing docs issues

8315312

Philipp-M approved these changes Jun 5, 2024

View reviewed changes

DJMcNab mentioned this pull request Jun 6, 2024

Add support for impl View for Arc/Rc<impl View> #164

Closed

DJMcNab added this pull request to the merge queue Jun 6, 2024

Merged via the queue into linebender:main with commit 86d9592 Jun 6, 2024
8 checks passed

DJMcNab mentioned this pull request Jun 17, 2024

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

Merged

DJMcNab deleted the core-two branch August 5, 2024 13:13

RagibHasin mentioned this pull request Aug 6, 2024

Fix non-standard textbox behavior in Windows #489

Merged

		@@ -1,363 +1,650 @@
		// Copyright 2023 the Xilem Authors
		// Copyright 2024 the Xilem Authors

		/// In order to support the open-ended [`DynMessage`] type, this trait requires an
		/// allocator to be available.

	impl<State, Action, Context, V, Element> ViewSequence<State, Action, Context, Element, WasAView>
	impl<State, Action, Context, V, Element> ViewSequence<State, Action, Context, Element, fn() -> WasAView>

		/// Insert a new element at the current index in the resulting collection.
		fn push(&mut self, element: Element);

Move xilem onto a new xilem_core, which uses a generic View trait #310

Move xilem onto a new xilem_core, which uses a generic View trait #310

Conversation

DJMcNab commented May 20, 2024 • edited Loading

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philipp-M left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoignardAzur left a comment

Choose a reason for hiding this comment

Philipp-M left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philipp-M commented Jun 2, 2024

Philipp-M commented Jun 2, 2024

DJMcNab commented Jun 3, 2024

Philipp-M left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Move `xilem` onto a new `xilem_core`, which uses a generic View trait #310

Move `xilem` onto a new `xilem_core`, which uses a generic View trait #310

DJMcNab commented May 20, 2024 •

edited

Loading