Open matthunz opened 1 month ago
@matthunz This mostly all looks pretty good, but:
NodeHandle
?You might also want to consider making the type:
struct NodeHandle<'a> {
doc: &'a Document,
node: &'a Node,
}
Which would allow the type to implement more things. I think we should also look at adding a
struct NodeHandleMut<'a> {
doc: &mut 'a Document,
node_id: NodeId,
}
which would allow a lot of the methods from Document
to moved to NodeHandleMut
.
Thanks for the review!
- Can we split it into separate PRs: in particular, the documentation and zoom changes seem standalone and uncontroversial, so would be good to get those merged!
Definitely! I think I got sucked into a bit of a rabbit hole here... I'll try to factor out the less controversial stuff
- Can we call the type
NodeHandle
?
Ooo yeah I actually like that a lot better
You might also want to consider making the type:
struct NodeHandle<'a> { doc: &'a Document, node: &'a Node, }
That sounds interesting too, it does sound like we'd have more flexibility to add things. I also really like the mutable handle for the same reasons.
Just to clarify on that todo task though unfortunately it looks like stylo
only excepts pointer-sized element types (specifically usize-sized, which sounds like a provenance issue so maybe we could submit a PR there 🤔). I was thinking we could do impl TElement for &'a NodeHandle<'a>
but that's tricky with Traverser
... I think it's possible with stylo as-is but I'm a little stuck so far
The methods for
Node
currently seem unsafe, as aNode
can outlive its tree field of*mut Slab<Node>
. It is also possible to obtain mutable references to the same node.Replaces the current unsafe method of storing raw pointers to the tree of each node:
With a container handle struct safely immutably borrowing from both:
Node
fields and methods