extract reusable functions from plainTextView#27572
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (652 lines, 6 files), I've queued these reviewers:
How this works
|
| * @remarks | ||
| * Seeded from `initialSelection` and adjusted by {@link applyTextOps} as the text changes. It | ||
| * follows the same logical position across edits, but the hook does not observe the user's live | ||
| * caret, so a consumer that needs an accurate caret should track the element's selection itself. |
There was a problem hiding this comment.
I'm not sure I follow this remarks comment. Is this just saying that this should not be used to track cursor position?
There was a problem hiding this comment.
Updated remarks clarifying that if they want the real caret position should get it from the rendered element
| * The consumer supplies the other direction (string → tree) themselves, typically by calling | ||
| * {@link applyTextEdit} from their input's change handler: | ||
| * | ||
| * ```tsx | ||
| * const { text } = useTreeSynchronizedString(tree); | ||
| * return ( | ||
| * <textarea | ||
| * value={text} | ||
| * onChange={(e) => applyTextEdit(tree, e.target.value)} | ||
| * /> | ||
| * ); | ||
| * ``` |
There was a problem hiding this comment.
Nit: I'm wondering if we may want to give a bit more prescriptive guidance here. For simple APIs like textarea, where the events only return the fully updated string, then recommending applyTextEdit probably makes sense. But I think we should probably recommend doing more properly incremental edits when possible.
We can probably get this across by noting that different text APIs offer different formats for change deltas, and so we don't offer a one-size-fits all solution for mapping those edits. But for simple APIs like textarea, we offer this naive implementation that can be used.
There was a problem hiding this comment.
Updated the remarks to note that different text apis report their edits in different formats
| */ | ||
| export function useTreeSynchronizedString( | ||
| tree: TextAsTree.Tree, | ||
| initialSelection?: TextSelection, |
There was a problem hiding this comment.
Probably not for this PR, since it seems like we don't have a use-case for it yet. But I wonder if we should expand this to track 0 or more selections.
| if (context.isBranch()) { | ||
| context.runTransaction(() => syncTextToTree(root, newText), { label }); | ||
| } else { | ||
| syncTextToTree(root, newText); | ||
| } |
There was a problem hiding this comment.
Is this branching needed? context.runTransaction should be callable regardless of whether or not we're on a branch. If we're not, I believe the implementation does what your fallback case here does anyways.
And if this is the case, I'm not sure we need this applyTextEdit helper at all. Users can reasonably grab the node's context and call runTransaction and syncTextToTree on their own.
There was a problem hiding this comment.
removed if statement and updated to use context.runTransaction
Josmithr
left a comment
Josmithr
left a comment
There was a problem hiding this comment.
Left a few more comments. This one is likely the most critical: https://github.com/microsoft/FluidFramework/pull/27572/changes#r3516458250
Bundle size comparisonBase commit: Notable changes
Per-bundle deltas
|
| * ``` | ||
| * @internal | ||
| */ | ||
| export function applyTextEdit( |
There was a problem hiding this comment.
I don't think this helper function is needed. I think we can just let callers combine runTransaction and syncTextToTree as needed.
| } | ||
| }); | ||
|
|
||
| // The hook is a one-way tree → string sync; exercise it directly via renderHook. |
There was a problem hiding this comment.
These tests should live in the test module that corresponds with the source code module. For now, that would be useTreeSynchronizedString.spec.ts. That said, it might make sense to merge that function into plainUtils.ts, in which case these would move to plainUtilts.test.ts
| const clamp = (offset: number): number => | ||
| Math.min(Math.max(offset, 0), reread.length); |
There was a problem hiding this comment.
We've defined clamp in 2 different places in this PR. My guess is that we have a clamp utility already somewhere that we should use. But if not, we should add this to arrayUtils (or someplace) so we can deduplicate.
| // No incremental delta is available for this change, so re-read the whole string. | ||
| // This happens when the character field's marks couldn't be composed into a single | ||
| // delta — e.g. the field was modified across multiple batches within one flush (such | ||
| // as an interleaved schema change) — or when the tree is out of sync with the delta. | ||
| // Any tracked selection is clamped into the new text, since a shrinking edit could | ||
| // otherwise leave it out of bounds. | ||
| const reread = tree.fullString(); | ||
| textRef.current = reread; | ||
| setText(reread); |
There was a problem hiding this comment.
Is this correct for all cases? I can imagine a case where a batched set of changes include deleting all characters, then inserting other characters. In a normal case, this would (presumably) result in the selection being effectively removed. But in this case, it would result in unrelated characters being selected.
I haven't thought through all of the relevant cases here, but I have a suspicion that this isn't the policy we want here.
I would recommend thinking through the cases here and ensuring we have them covered by tests. Since this is a helper utility, rather than built-in functionality, it's probably okay for there to be edge cases we don't handle well. But we should make sure these cases and associated policy are clearly documented, and that the policy we go with is the one that will cause least disruption.
| }); | ||
|
|
||
| // The hook is a one-way tree → string sync; exercise it directly via renderHook. | ||
| describe("useTreeSynchronizedString", () => { |
There was a problem hiding this comment.
I suspect we should probably have many more test cases than this. The code has a number of branches and edge cases we should make sure we are covering. E.g. cases where the selection gets clamped, cases where the selection is reduced to 0 characters, etc.
Description
Extracts reusable functions from plainTextView for shared use in our example app, and boards.