Add fine-grained tree-structure tracking in xilem and effcient tree-updates in xilem_web

[Philipp-M]

This is based on top of #159 (which is a requirement for this PR, and the reason why this is a draft PR)

This implements fine-grained tree-structure tracking described in #54 using a TreeTrackerSplice (which implements the ElementsSplice introduced in #159).
Currently it's only replacing the bloom filter which is only used for accessibility. But I expect this to be more useful later.

It's inspired by the lasagna branch in druid.

[PoignardAzur]

As I mentioned in the zulip thread, I'm of two minds on this.

On the one hand, I kinda hate VecSplice and I think it's a very confusing abstraction, and I think this PR compounds the things I don't like.

On the other hand, this is good quality code, and it's an incremental improvement.

I'd be a lot more comfortable with this PR if it included unit tests. I'm honestly feeling extremely nervous about the existing VecSplice code, because after reading it, I notice that a lot of its inner complexity isn't currently used by our web and widget code. Like, skip isn't even called anywhere.

So, given that how VecSplice is supposed to work is undocumented, I'm guessing that whether it actually works can only be known by those who can peer into the thoughts of Raph Levien.

Anyway, tests and documentation aside, my other sticking point is that I'd like the splicing code to stay out of the backend side. Since that backend is soon going to be replaced by a different crate, your changes would be easier to integrate with the Masonry merge if they interacted with the backend as little as possible. Essentially, TreeStructure is the only datastructure I'd want the backend to be aware of.

If the backend-related concerns are addressed, I'd be okay to merge this PR. Consider everything else extra credit.

[Philipp-M]

I kinda hate VecSplice and I think it's a very confusing abstraction
...
So, given that how VecSplice is supposed to work is undocumented, I'm guessing that whether it actually works can only be known by those who can peer into the thoughts of Raph Levien.

I still think it's mostly because it's not really documented that well (and it's not the easiest to digest code code admittedly, but with good documentation it should probably not be necessary to dive that deep into the details). The general idea how it's supposed to work can be extracted from the code though?

I don't want to defend it as I also generally like a more mutation-log based approach as discussed on zulip.
But I think it makes sense in the way the architecture is currently built (every view/view sequence manages their corresponding element(s) and recursively traverses their children). I don't see currently how it could be done significantly better otherwise.

Well I can write some tests as this could also well serve as documentation (but I don't like to do unnecessary work when you think the VecSplice or generally this approach should go away any time soon).

Anyway, should #159 be just merged with/as this PR or does it make sense to do these separately?

[PoignardAzur]

Anyway, should #159 be just merged with/as this PR or does it make sense to do these separately?

I think we can close #159, so the discussion is centralized in this PR.

[PoignardAzur]

Well I can write some tests as this could also well serve as documentation (but I don't like to do unnecessary work when you think the VecSplice or generally this approach should go away any time soon).

Fair enough. It's far outside my current priorities that I don't expect this code to go away for at least 6 months, but I do want to eventually refactor it, so I get your reluctance. I think tests would be better, but Xilem current has virtually none, so it would be unfair to hold your PR to a higher standard. Consider tests optional for getting this PR accepted.

[Philipp-M]

I think I remember why I put the code of the tree-structure changes into the widget part of the codebase. I think I once had it within the views, until I recognized that the type of the ids were different between view/widget...

This makes stuff more complicated because of the Pod (which holds the widget id) that is currently only constructed by the view sequences (more specifically by impl ViewSequence for impl View) (and the root pod within app.rs). I need the id of the element/pod though, and I can't access that within View::rebuild as it's done by the parent context.

I'm currently brainstorming how to get around that without shuffling a lot of code (or ugly hacks).

[Philipp-M]

So one idea I have, is extending the ElementsSplice methods with a parameter cx: &mut Cx to record the changes (or apply them directly in the view context).

So taking the content of the ViewSequence::build() for the view:

let (id, state, element) = <V as View<T, A>>::build(self, cx);
elements.push(Pod::new(element), cx); // applys children tree-structure changes to the newly created Pod
(state, id)

Does that sound feasible?

[Philipp-M]

I've done that, I'm not extremely happy about it, it introduces some kind of accidental complexity I think, and may be slightly brittle (as it requires a clean 1:1 relation of Widget <-> Pod, which is planned anyway I think).
At this point I'm also considering rethinking the way the View(Sequence) architecture/traits (towards less inplace mutations). But i think this requires quite a bit of thought to be an actual improvement.

Anyway how I have "solved" this for now is by having a Cx::mark_children_tree_structure_mutations(&mut self, mutations: Vec<SpliceMutation>)

which is called by the view that owns the children elements. And then register them with the above function.
TreeTrackerSplice::push(&mut self, element: Pod, cx: &mut Cx) looks like this then:

self.mutations.push(SpliceMutation::Add(element.id()));
cx.apply_children_tree_structure_mutations(element.id());
self.splice.push(element);

Maybe there's a better way (open for suggestions)

[PoignardAzur]

Can you push the commits where you implemented that? I think I don't see them.

[Philipp-M]

Yeah I wanted to get general feedback first, but since the changes were mostly done, you're probably right.

I've pushed them (and added a few doc comments, such that it's hopefully comprehensible). I've also added tests for the actual tree-structure mutations, since they were a low hanging fruit, and is probably more likely to survive. (I think automated tests for the actual ElementsSplice implementations are a little bit more work as it involves the whole widget/view architecture).

[PoignardAzur]

Anyway how I have "solved" this for now is by having a Cx::mark_children_tree_structure_mutations(&mut self, mutations: Vec<SpliceMutation>)

which is called by the view that owns the children elements. And then register them with the above function. TreeTrackerSplice::push(&mut self, element: Pod, cx: &mut Cx) looks like this then:

self.mutations.push(SpliceMutation::Add(element.id()));
cx.apply_children_tree_structure_mutations(element.id());
self.splice.push(element);

Maybe I'm missing something obvious, but is SpliceMutation even necessary a this point? The TreeStructureTrackingSplice::push() method takes a Cx and a WidgetPod; TreeStructure::append_child() takes a parent id and a child id. It seems like you could easily call one from the other and skip all lot of intermediary steps.

Right now, it looks like you're using Cx::mark_children_tree_structure_mutations and Cx::apply_children_tree_structure_mutations as a workaround to the fact that Cx doesn't have the parent id. EDIT: Actually, no, you can get it from the id path.

Anyway, if think your code would get a lot more robust if you skipped the mutation list and applied the mutations directly as they come.

(Also, quick question, have you tried running some Xilem apps, eg the Taffy example, and checked that the child tracking actually worked?)

[PoignardAzur]

I feel much better about this code now. I definitely like the added unit tests.

If we can resolve the bit with mark_children_tree_structure_mutations then we're good to merge.

[PoignardAzur]

Having an issue/PR number associated with TODOs is good, assuming the linked issue/PR includes enough info for an outsider contributor reading it to know how to contribute.

Since it's awkward to create issues for a PR that hasn't been merged yet, I think a good compromise would be to edit the top-level post of this PR with:

• A list of all the TODOs it adds.
• Enough info to operationalize resolving those TODOS (eg a short description of the problem and how to solve it). I know this is a lot to ask, so it's okay if this info is light, as long as it's immediately visible.

[Philipp-M]

Hmm, not sure about that, as I don't think a lot of people will look it up (vs the effort of creating such).
And when the TODO isn't clear in the code context (and/or is generally worth an issue) IMO it should really be an issue with a clear description after the PR has been merged.
After the recent update, I don't think such TODOs have survived (I don't think the TODO comment above is worth an issue TBH, I'll probably add a simple PR directly targeting that after this PR, to not further bloat this one).

[Philipp-M]

I'm going a bit in more detail over the review later (when I got more time), thanks so far for the detailed review.

One thing though:

Actually, no, you can get it from the id path.

That's unfortunately not the case, as it's the ViewId (we should really rename it (see also #168), I forgot about it when I did the changes again). While we need the WidgetId.

This is why it's pretty tricky currently to do this cleanly I think (without reshuffling the whole code-base).

[PoignardAzur]

That's unfortunately not the case, as it's the ViewId

Ah, right, I forgot. Okay, so what I suggest is:

• Add a parent_widget: Option<widget::Id> field to Cx, with a getter method.
• Add a parent_widget: widget::id argument to Cx::with_id() and Cx::with_new_id()

This adds some cruft to the codebase (we're basically tracking the same thing twice, if I understand correctly), but we'll deal with that later.

(we should really rename it (see also #168)

Two renames (Id -> ViewId, Id -> WidgetId) would be welcome, though probably not in this PR.

[Philipp-M]

Ok, I've updated this and applied all the review comments.

Maybe I'm missing something obvious, but is SpliceMutation even necessary a this point?
...
Anyway, if think your code would get a lot more robust if you skipped the mutation list and applied the mutations directly as they come.

Yeah you're right, having access to Cx in the splice methods makes all of this quite a bit simpler indeed.
The idea to use something like with_id is certainly cleaner, made the code more compact.

I've implemented that though similarly as the id_path in Cx (via a stack). Just having an option is not enough, as children setting this, overwrite it for other children. So I've added new context methods named with_pod and with_new_pod so that access to the element id is always valid within the views (via Cx::element_id). This is also necessary (as described below) since there's no direct ViewId <-> WidgetId relation (there are more ViewIds than WidgetIds).

Also, quick question, have you tried running some Xilem apps, eg the Taffy example, and checked that the child tracking actually worked?

Sure, I don't don't want to merge untested code (did it via the good ol' println debugging).

Two renames (Id -> ViewId, Id -> WidgetId) would be welcome, though probably not in this PR.

Yeah agree, we should keep PRs small and focused.

we're basically tracking the same thing twice, if I understand correctly

No that's not necessarily the case. Views can e.g. have ids while having the same View::Element of its child view. xilem_web has this e.g. for events. Generally this is true for views that take events/messages, without extra widget implementation.

[PoignardAzur]

LGTM. Adding it to the merge queue.