Add opt-in deferred layout rebuilds to PlainEditor

[AdrianEddy]

Every mutating PlainEditor operation (insert, delete, IME compose/commit) rebuilds the full layout before returning. Hosts that apply several operations in one frame — replaying a batch of input events, multi-step IME commits, programmatic multi-part edits — pay one full relayout per operation, even though only the final layout is ever read or drawn.

This PR adds an opt-in mode that defers the rebuild to the next read:

editor.set_defer_layout(true);
// ... apply a batch of edits: one rebuild happens at the next
// refresh_layout()/layout() or any driver method that reads geometry.

• Default behavior is unchanged. With deferral off (the default), every call site behaves exactly as before.
• While the layout is dirty, selections are stored as provisional byte-offset cursors; the rebuild's existing selection refresh snaps them to cluster boundaries, so the end state is identical to eager mode (covered by an eager-vs-deferred equivalence test).
• Operations that must read the layout before mutating (delete, delete_word, backdelete, backdelete_word — cluster and word boundaries) now call refresh_layout() first. Besides making them correct mid-batch, this closes a latent stale-read: today these methods read the old layout if called right after set_text.
• try_layout() returning None while dirty, &self geometry readers (cursor_geometry, selection_geometry, ime_cursor_area) returning last-built geometry mid-batch, and the Generation nudge moving to rebuild time are the observable differences — all documented on set_defer_layout — which is why the mode is opt-in rather than the new default.

Tests cover: a batch performing a single rebuild with correct text and selection, an emoji-cluster backspace mid-batch (fresh-layout boundary read), provisional-cursor clamping for mid-cluster IME offsets, byte-wise deletes matching eager mode, and a mixed op sequence (moves, word deletes, IME compose/commit) asserting eager and deferred editors end in identical states.

^{This PR was generated by Claude}

[DJMcNab]

For triage, when you say:

Hosts that apply several operations in one frame

Do you have an actual use case in an application you're developing where this is becoming a bottleneck?

This change is somewhat complicated to reason about, although my gut reaction is that the core conceit is correct (that the editing operations which mutate the text based on externally provided indices don't need a fully-updated text). It seems like it would be easy to get this wrong, and having two code paths here is concerning to me.

It isn't clear to me what advantage you think there is in having this mode be optional...

As we start to be more careful about Grapheme clusters (see e.g. #715), this who editing code is likely to change somewhat. As such, I'd be inclined to defer this until more stuff in that direction lands.

[AdrianEddy]

Yes, I have a real app and need for this shape - not necessarily on its own, but it becomes essential when paired with #730 to make text and styles atomic.

The reason for having it optional is just to not change any existing code/behavior while adding a way to achieve atomic text+styles updates. However I have now reshaped the PR to use the deferred mode as the only code path and made eager mode a simple wrapper for deferred+flush

[DJMcNab]

It is helpful context that you believe the styling change is incoherent without this (personally, I can't really see why that would be the case, but that's by-the-by.

However I have now reshaped the PR to use the deferred mode as the only code path and made eager mode a simple wrapper for deferred+flush

Changing the formulation to a different once with not at all clear semantic differences (is there any semantic difference? If not, why do it?) without any explanation of what that helps with does not make for a good review experience. I'm quite quickly reaching the end of my patience to review this change.

My overall feedback (echoing that which I know you've received in Vello...) is that:

1. You need to get better at explaining the motivation behind PRs - that is, you need to write PR descriptions which focus on the actually useful information. For example, my question about this having observable differences is already answered in the PR description, but I didn't get that far in reading it because it has too much useless information otherwise.
2. You need to put more human thought into making sure that your changes lead to/towards a coherent end-state, rather than asking reviewers to do that work. As an example, instead of asking 'why does selection_geometry return stale information?' (and can we solve that, or make it return None in this case), you've instead gone for the path of making the entire thing optional, which doesn't really serve anyone's needs well.

I think that we're likely to change our LLM policy to disallow LLM-generated PR descriptions.

[AdrianEddy]

I'm sorry my PRs cause too much review burden, this is not my intention.

I thought it's clear that my latest change addresses your two code paths concern. My previous comment (contrary to the PR description) was written by me and I kept it short to avoid lengthy LLM description of that change, but apparently that also backfired since now it was too short. I also now see that I misunderstood what you were referring to. I'm sorry for that.

My PRs are driven by real world usage and I'm just trying to add something useful to the libraries I use. I'm always trying to keep the changes minimal to avoid large refactors or behavior changes, which sometimes is good, and sometimes not desirable. However, I can't know which approach is best for every change because I don't have the full mental model of the entire codebase and the goals and roadmaps of the maintainers. Even when we remove the LLM from the picture, adding more human thought is not always possible without knowing the entire project end-to-end and its history and roadmap and refactor plans.

I'm always reviewing the PRs myself before opening and I do understand these changes, and they are also always validated in my app before opening.

I'm happy to change any of this according to your feedback and your vision, but I can't possibly know everything beforehand. The reason for making it optional was to avoid a refactor of public API, but I see now that was not a good reason.

Anyway, concrete answers for your questions:

I can't really see why that would be the case

Example: text is foo bar, highlighter set red on 4..7 (bar). Typing X at the start rebuilds right away with the old overlay, so red lands on ba. That rebuild bumps the generation, so an app that redraws on generation change paints that frame. I can't set the new overlay before the edit - it's computed from text that doesn't exist yet. With this PR, I can edit without rebuild, then set the overlay and then rebuild.

instead of asking 'why does selection_geometry return stale information?' (and can we solve that, or make it return None in this case), you've instead gone for the path of making the entire thing optional, which doesn't really serve anyone's needs well.

I'm not opening a new PR to not create more burden, but I believe changing it like that would be the proper way to handle this. It changes the public API, but looks like this is what you prefer. Let me know if you want to see this as a separate PR, and base 729 on top of it.

diff --git a/parley/src/editing/editor.rs b/parley/src/editing/editor.rs
--- a/parley/src/editing/editor.rs
+++ b/parley/src/editing/editor.rs
@@ -797,6 +797,41 @@   (PlainEditorDriver, after `layout()`)
+    /// Get rectangles, and their corresponding line indices, representing the selected portions of
+    /// text, refreshing the layout as needed.
+    pub fn selection_geometry(&mut self) -> Vec<(BoundingBox, usize)> {
+        self.refresh_layout();
+        // We do not check `show_cursor` here, as the IME handling code collapses the
+        // selection to a caret in that case.
+        self.editor.selection.geometry(&self.editor.layout)
+    }
+
+    /// Invoke a callback with each rectangle representing the selected portions of text, and the
+    /// indices of the lines to which they belong, refreshing the layout as needed.
+    pub fn selection_geometry_with(&mut self, f: impl FnMut(BoundingBox, usize)) {
+        self.refresh_layout();
+        // We do not check `show_cursor` here, as the IME handling code collapses the
+        // selection to a caret in that case.
+        self.editor.selection.geometry_with(&self.editor.layout, f);
+    }
+
+    /// Get a rectangle representing the current caret cursor position, refreshing the layout as
+    /// needed.
+    ///
+    /// There is not always a caret. For example, the IME may have indicated the caret should be
+    /// hidden.
+    pub fn cursor_geometry(&mut self, size: f32) -> Option<BoundingBox> {
+        self.refresh_layout();
+        self.editor.cursor_geometry(size)
+    }
+
+    /// Get a rectangle bounding the text the user is currently editing, refreshing the layout as
+    /// needed (see [`PlainEditor::ime_cursor_area`]).
+    pub fn ime_cursor_area(&mut self) -> BoundingBox {
+        self.refresh_layout();
+        self.editor.ime_cursor_area_unchecked()
+    }
+
     // --- MARK: Internal helpers---
@@ -858,26 +893,43 @@   (PlainEditor readers)
     /// Get rectangles, and their corresponding line indices, representing the selected portions of
     /// text.
-    pub fn selection_geometry(&self) -> Vec<(BoundingBox, usize)> {
+    ///
+    /// Returns `None` while the layout is dirty, as the geometry would be computed against a
+    /// stale layout; refresh the layout first, or use
+    /// [the driver's equivalent](PlainEditorDriver::selection_geometry).
+    pub fn selection_geometry(&self) -> Option<Vec<(BoundingBox, usize)>> {
+        if self.layout_dirty {
+            return None;
+        }
         // We do not check `self.show_cursor` here, as the IME handling code collapses the
         // selection to a caret in that case.
-        self.selection.geometry(&self.layout)
+        Some(self.selection.geometry(&self.layout))
     }
 
     /// Invoke a callback with each rectangle representing the selected portions of text, and the
     /// indices of the lines to which they belong.
-    pub fn selection_geometry_with(&self, f: impl FnMut(BoundingBox, usize)) {
+    ///
+    /// Returns `false` without invoking the callback while the layout is dirty, as the geometry
+    /// would be computed against a stale layout; refresh the layout first, or use
+    /// [the driver's equivalent](PlainEditorDriver::selection_geometry_with).
+    pub fn selection_geometry_with(&self, f: impl FnMut(BoundingBox, usize)) -> bool {
+        if self.layout_dirty {
+            return false;
+        }
         // We do not check `self.show_cursor` here, as the IME handling code collapses the
         // selection to a caret in that case.
         self.selection.geometry_with(&self.layout, f);
+        true
     }
 
     /// Get a rectangle representing the current caret cursor position.
     ///
     /// There is not always a caret. For example, the IME may have indicated the caret should be
-    /// hidden.
+    /// hidden. `None` is also returned while the layout is dirty, as the geometry would be
+    /// computed against a stale layout; refresh the layout first, or use
+    /// [the driver's equivalent](PlainEditorDriver::cursor_geometry).
     pub fn cursor_geometry(&self, size: f32) -> Option<BoundingBox> {
-        self.show_cursor
+        (!self.layout_dirty && self.show_cursor)
             .then(|| self.selection.focus().geometry(&self.layout, size))
     }
 
@@ -886,7 +938,17 @@
     /// This is useful for suggesting an exclusion area to the platform for, e.g., IME candidate
     /// box placement. This bounds the area of the preedit text if present, otherwise it bounds the
     /// selection on the focused line.
-    pub fn ime_cursor_area(&self) -> BoundingBox {
+    ///
+    /// Returns `None` while the layout is dirty, as the geometry would be computed against a
+    /// stale layout; refresh the layout first, or use
+    /// [the driver's equivalent](PlainEditorDriver::ime_cursor_area).
+    pub fn ime_cursor_area(&self) -> Option<BoundingBox> {
+        (!self.layout_dirty).then(|| self.ime_cursor_area_unchecked())
+    }
+
+    /// Get a rectangle bounding the text the user is currently editing, assuming that the layout
+    /// is valid (see [`ime_cursor_area`](Self::ime_cursor_area)).
+    fn ime_cursor_area_unchecked(&self) -> BoundingBox {
         let (area, focus) = if let Some(preedit_range) = &self.compose {

If there's anything else I can do to make your job easier, please let me know. I'm also happy to talk directly on Zulip if needed.

[DJMcNab]

an app that redraws on generation change paints that frame

I don't have an understanding of an app setup where this could happen. At the point where you do the edit, you have an exclusive reference to the editor, so it isn't clear to me why you can't just do the edit, then apply the recomputed styles immediately. Like, I do agree that having an extra rebuild in that case is not ideal, but you seem to be treating this PR as if it's semantically a blocking concern, which it isn't clear to me that this is.

Ultimately, what's happened here is that:

1. You have identified a real opportunity, that text editing operations which change grapheme clusters for cursor position can effectively be ignored (your proposed new behaviour matches Firefox, whereas the current behaviour probably matches Chrome; both are reasonable).
2. This has raised a pre-existing bug with how selection geometry works, in that it assumes that the geometry isn't dirty, but that already can be an incorrect assumption.
3. The fix you made to that issue being expanded by this issue was to make the model change you want optional, thus adding complexity but avoiding exposing more users to this bug. (Fwiw, I find it hard to imagine people using the cursor geometry without also accessing the Layout, so I don't think this bug is any more blocking for this PR than it already was). However, this added complexity is not well motivated, given that all it is doing is.

Additionally, all of this context is hidden within a lot of "filler" content from the LLM (I don't care to know that "Default behavior is unchanged." as the top level thing, I need to know why you thought that behaviour would be changed, and why you evaluated that not changing that behaviour was more important than the added complexity).

Ultimately, I do agree that we need to make changes here, but unfortunately I don't think it's a suitable use of my time for me to be the one digging into that. But I hope you recognise that there are ways that you could have packaged this PR/bug you have identified which would have taken much less time to interact with. The actual important part of this which needs reckoning with is "how do we handle selection_geometry et al. requiring an up-to-date layout"; if you'd made that the focus of the PR description, and hadn't made the decision to mask the issue by making this optional, it's likely that I would have had time to give the quick answer (we need to change the API, probably to just match .layout()/try_layout) instead of using all of my time budget on trying to understand the process which led to the odd decisions in this PR.

[AdrianEddy]

I don't care to know that "Default behavior is unchanged." as the top level thing

LLMs aside, this is surprising to me, as it is an important thing for me to know, and I would write this myself in 100% human written PR as well. From my personal experience of writing software for the past 20 years, any behavior or API changes are at very top of the things I care about, because once enough people use certain API or rely on behavior - any change to it requires careful consideration. In my view "Default behavior is unchanged" is something that makes the review easier, not harder. But of course, personal opinions vary and that's fine.

In any case, I agree that this PR was badly executed and I will make sure to create better ones in the future. I just feel like the communication here could be better, instead of arguing over LLM.
I'm sorry to have wasted your time. I will come back with better PR.

Would it at least make you feel better about your time to know that I've been supporting linebender by sponsoring Raph $200/mo since 2022? I know this doesn't entitle me to any kind of support, but at least it shows I care about the work you're doing.

[DJMcNab]

Thank you for your support to Raph (I also recognise a lot of other important faces in your GitHub sponsors - thank you for those as well).
I'm sorry that this PR has been a bad experience, and I hope to work with you productively in the future.

My choice of focusing on 'default behaviour is unchanged' was an attempt to highlight that the actual important part is 'what behaviour changes if the option is enabled'. For a refactor PR, something like 'behaviour is unchanged' definitely is helpful.
Ultimately, the main message is that a PR authoring body is a good place to highlight places where decisions were made, and why you made those decisions (as opposed to a recap of what the PR actually does). But yeah, re-litigating this forever isn't helpful.