arrayNode anchor tracking using deltas#27697
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (262 lines, 10 files), I've queued these reviewers:
How this works
|
| * After {@link ArrayPlaceAnchor.dispose} has been called the anchor stops updating and this returns the last | ||
| * tracked value, which may no longer be within the array's bounds if the array has since shrunk. Do not use the | ||
| * index of a disposed anchor as an insertion index without first re-validating it against the current length. |
There was a problem hiding this comment.
Is there any reason to support users calling index after the anchor has been disposed? We could just throw an exception in this case, or leverage our breakable infrastructure to accomplish this.
There was a problem hiding this comment.
Updated with usage error when anchor is disposed
There was a problem hiding this comment.
Did you take a look at our breakable infra? I think this is the sort of use case it was built for.
Josmithr
left a comment
Josmithr
left a comment
There was a problem hiding this comment.
I left a few comments. Overall direction looks good!
Question: can we update our selection-tracking utilities in the react package to leverage this? Seems like we should be able to remove some code over there.
github-actions
Bot
added
area: framework
|
I made some unpushed changes that updated our selection tracking utilities, and it cleans up nicely :) Did you want me to include those changes in this PR, or in a follow up? |
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Either way is fine by me. Whatever is easiest. |
| --- | ||
| Array insertion anchors now track their index from change deltas | ||
|
|
||
| The `@alpha` `ArrayPlaceAnchor` returned by `createArrayInsertionAnchor` now maintains its `index` incrementally from the array node's change delta instead of re-deriving it from the child that happened to sit at the anchor point when it was created. Inserts and removes before the anchor shift it, while edits after it leave it in place. |
There was a problem hiding this comment.
Nit: let's link to the API docs where appropriate here, since these APIs already exist.
| * Stop tracking this anchor and release any resources it holds. | ||
| * @remarks | ||
| * Call this when the anchor is no longer needed (for example when a tracked cursor position is discarded). | ||
| * Reading {@link ArrayPlaceAnchor.index} after disposal throws. Calling `dispose` more than once has no effect. |
There was a problem hiding this comment.
To make this a bit more future-proof, it might be better to say something like "Interacting with an anchor (including reading its properties) after it has been disposed is invalid and will throw."
| node: TreeArrayNode, | ||
| currentIndex: number, | ||
| ): ArrayPlaceAnchor { | ||
| // Validate the caller-provided index rather than silently correcting it: an out-of-range or non-integer |
There was a problem hiding this comment.
Nit: I don't think we need to note the alternative option of silently correcting. This is another example of (subtly) referencing previous implementation details in a way that is more likely to be confusing than helpful, in my opinion.
Bundle size comparisonBase commit: Notable changes
Per-bundle deltas
|
| * carry both `detach` and `attach`, which is handled as a removal followed by an insertion. The per-mark behavior is | ||
| * commented inline below. | ||
| * | ||
| * This mirrors the cursor-tracking accounting used for text selections. |
There was a problem hiding this comment.
And presumably the latter will go away (and just leverage this code) in a future PR? If so, might be best to just remove this line (or be sure to remember to remove it in the follow-up PR).
| }); | ||
|
|
||
| // This case sticks to the end of the array, which is not ideal, and will need to be fixed with a more sophisticated anchor implementation. | ||
| // With delta-based tracking the anchor stays in the gap the removed item occupied rather than |
There was a problem hiding this comment.
This comment appears to be referencing the old (unwanted) behavior. Let's just remove it.
| // It's good to test that this still gives a valid index and does not crash, but ideally this would anchor to the range between items rather than jumping to the end. | ||
| assert.equal(anchor.index, 2); | ||
| // The item originally at the anchor point is gone; the anchor rests between the surviving | ||
| // neighbors (now [1, 3]) instead of jumping to the end. |
There was a problem hiding this comment.
Let's remove the "instead of jumping to the end" - this is a reference to previous behavior, which is more likely to be confusing than helpful.
| anchor.dispose(); | ||
| }); | ||
|
|
||
| it("collapses to the start when a removed range straddles the anchor", () => { |
There was a problem hiding this comment.
Nit: it isn't immediately clear to me what "straddles the anchor" means. Might be worth updating the wording here to be a bit more explicit.
Description
Adds anchor tracking to arrayNodes using its deltas. This can be used in our collab text apis utilities to leverage this