CUA type-over is a single undo step (Q#U1)
Typing over a selection composed two edits in Lua — delete_region() + insert_char() — so it recorded two undo steps: one undo left the half-replaced text, two restored the original. Undo granularity is per apply_edit in both modes (v0.1 pushes one UndoEntry per edit; CRDT commits per edit via export and groups by commit, with record_checkpoint unused), so the fix is to make type-over one edit. New core EditorCore::insert_char_over_region emits a single EditOp::Replace when a region is active (cursor past the inserted bytes, selection cleared) and delegates to insert_char otherwise. The three type-over commands (buffer.newline / tab / self-insert) call it via a new Lua binding instead of the delete+insert pair. delete_region and insert_char are unchanged for their other callers; region-aware backspace/delete already emit one op. Verified one undo unit in BOTH modes (dual_mode replace_is_a_single_undo_step covers v01 + crdt — a CRDT Replace is delete-then-insert internally but one commit) plus an end-to-end acceptance test through the key-dispatch path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
fde5453721
commit
db073cc668
|
|
@ -101,19 +101,21 @@ cmd { name = "cursor.select-line-start",
|
||||||
cmd { name = "cursor.select-line-end",
|
cmd { name = "cursor.select-line-end",
|
||||||
description = "Extend selection to end of line.",
|
description = "Extend selection to end of line.",
|
||||||
fn = function() ensure_anchor(); ed.move_line_end() end }
|
fn = function() ensure_anchor(); ed.move_line_end() end }
|
||||||
-- CUA type-over: inserting with an active selection replaces it
|
-- CUA type-over: inserting with an active selection replaces it in a
|
||||||
-- (`delete_region` is a no-op without one). The pmacs-gpu frontend
|
-- SINGLE edit (one undo step — `insert_char_over_region` emits one
|
||||||
-- relies on this: its optimistic-insert path detects an own-window
|
-- `Replace`, not a `delete` + `insert` pair). Without a selection it
|
||||||
-- selection and round-trips the key so these commands run.
|
-- is a plain insert. The pmacs-gpu frontend relies on this: its
|
||||||
|
-- optimistic-insert path detects an own-window selection and
|
||||||
|
-- round-trips the key so these commands run daemon-side.
|
||||||
cmd { name = "buffer.newline",
|
cmd { name = "buffer.newline",
|
||||||
description = "Insert a newline at the cursor, replacing the active region.",
|
description = "Insert a newline at the cursor, replacing the active region.",
|
||||||
fn = function() ed.delete_region(); ed.insert_char(10) end }
|
fn = function() ed.insert_char_over_region(10) end }
|
||||||
cmd { name = "buffer.tab",
|
cmd { name = "buffer.tab",
|
||||||
description = "Insert a tab at the cursor, replacing the active region.",
|
description = "Insert a tab at the cursor, replacing the active region.",
|
||||||
fn = function() ed.delete_region(); ed.insert_char(9) end }
|
fn = function() ed.insert_char_over_region(9) end }
|
||||||
cmd { name = "buffer.self-insert",
|
cmd { name = "buffer.self-insert",
|
||||||
description = "Insert the codepoint argument at the cursor, replacing the active region.",
|
description = "Insert the codepoint argument at the cursor, replacing the active region.",
|
||||||
fn = function(codepoint) ed.delete_region(); ed.insert_char(codepoint) end }
|
fn = function(codepoint) ed.insert_char_over_region(codepoint) end }
|
||||||
|
|
||||||
-- History --------------------------------------------------------------------
|
-- History --------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,66 @@
|
||||||
|
# CUA type-over undo grouping — framing pass
|
||||||
|
|
||||||
|
Date: 2026-06-15. The region-aware type-over (PR #60-era) replaces a
|
||||||
|
selection by composing two ops in Lua —
|
||||||
|
`ed.delete_region(); ed.insert_char(cp)` (builtin/commands/default.lua
|
||||||
|
`buffer.{newline,tab,self-insert}`). That's two `apply_edit` calls,
|
||||||
|
hence two undo steps: undo once leaves the half-replaced text, undo
|
||||||
|
twice restores the original. We want one atomic undo step.
|
||||||
|
|
||||||
|
## Survey facts
|
||||||
|
|
||||||
|
- Undo granularity is **per `apply_edit`** in both modes. v0.1 pushes
|
||||||
|
one `UndoEntry` per edit (buffer.rs:1162). CRDT mode bypasses that
|
||||||
|
stack and lets loro's `UndoManager` group ops; each `apply_edit`
|
||||||
|
commits via `export_updates_since` (buffer.rs:1061), and nothing
|
||||||
|
calls `record_checkpoint` (it's dead code) — so the undo unit is
|
||||||
|
effectively the commit, i.e. one per `apply_edit`.
|
||||||
|
- `EditOp::Replace { range, bytes }` already exists end to end:
|
||||||
|
`rope.replace` in v0.1 (buffer.rs:1111), delete-then-insert on the
|
||||||
|
loro text in CRDT (buffer.rs:1028) — but **one** `apply_edit`, so
|
||||||
|
one undo unit either way.
|
||||||
|
- Type-over always runs daemon-side: with a selection, pmacs-gpu
|
||||||
|
round-trips the key rather than optimistically applying (the comment
|
||||||
|
at default.lua:104), so the daemon's command does the edit and
|
||||||
|
broadcasts the result. No frontend-undo coordination needed.
|
||||||
|
|
||||||
|
## Q#U1 — mechanism
|
||||||
|
|
||||||
|
**Stance: model type-over as a single `EditOp::Replace`, not a
|
||||||
|
delete+insert pair.** This is the *correct* representation (a
|
||||||
|
type-over is one replace, not two edits) and gives one undo step in
|
||||||
|
both modes by construction — no undo-group / transaction machinery,
|
||||||
|
no reliance on loro's time-based merge interval (which is unset). A
|
||||||
|
general `begin/end_undo_group` boundary was considered and rejected
|
||||||
|
as over-built for the one concrete case; it can be inducted later if a
|
||||||
|
third multi-edit command needs it.
|
||||||
|
|
||||||
|
## Q#U2 — surface
|
||||||
|
|
||||||
|
**Stance: a core `insert_char_over_region(ch)` + one Lua binding.**
|
||||||
|
Region active → one `Replace(region, ch_bytes)`, cursor → region
|
||||||
|
start + len, selection cleared. No region → delegate to the existing
|
||||||
|
`insert_char` (unchanged Insert path). The three type-over commands
|
||||||
|
call the single binding with their codepoint (10 / 9 / arg).
|
||||||
|
`delete_region` and `insert_char` stay (other callers); only the
|
||||||
|
type-over composition moves into the core. Region-aware
|
||||||
|
backspace/delete already emit one `delete_region` op, so they're
|
||||||
|
already one undo step — untouched.
|
||||||
|
|
||||||
|
## Predicted findings (categorical bets)
|
||||||
|
|
||||||
|
1. **CRDT undo-unit assumption**: the per-commit grouping reasoning is
|
||||||
|
inferred from the code, not loro docs — the dual-mode test is the
|
||||||
|
proof; if CRDT still shows two steps, loro is checkpointing between
|
||||||
|
the internal delete and insert and an explicit boundary is needed.
|
||||||
|
2. **Cursor/selection after replace**: an off-by-the-inserted-length
|
||||||
|
cursor or a lingering selection in some multi-byte / empty-insert
|
||||||
|
edge — pinned by an editor_core test.
|
||||||
|
|
||||||
|
## Session plan
|
||||||
|
|
||||||
|
One commit: core `insert_char_over_region`, Lua binding, rewire the
|
||||||
|
three commands, a dual-mode buffer test (Replace = one undo step) and
|
||||||
|
an editor_core / acceptance test (type-over over a selection = one
|
||||||
|
undo step, cursor + selection correct). Manual validation deferred to
|
||||||
|
the user as usual.
|
||||||
|
|
@ -2071,6 +2071,27 @@ mod tests {
|
||||||
assert!(matches!(b.undo(), Err(BufferError::NothingToUndo)));
|
assert!(matches!(b.undo(), Err(BufferError::NothingToUndo)));
|
||||||
});
|
});
|
||||||
|
|
||||||
|
dual_mode_test!(replace_is_a_single_undo_step, |_make, make_bytes| {
|
||||||
|
// CUA type-over models "replace the selection with the typed
|
||||||
|
// char" as one `EditOp::Replace`, so it must undo in ONE step
|
||||||
|
// in both modes — including CRDT, where a Replace is a
|
||||||
|
// delete-then-insert on the loro text but still a single
|
||||||
|
// `apply_edit` (one commit, one undo unit). If it were two
|
||||||
|
// units, the first undo would leave "hel" and the second
|
||||||
|
// would be needed to reach "hello".
|
||||||
|
let mut b = make_bytes("test", b"hello");
|
||||||
|
b.apply_edit(EditOp::Replace {
|
||||||
|
range: Range::new(2, 5),
|
||||||
|
bytes: b"X",
|
||||||
|
})
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(collect(&b), b"heX");
|
||||||
|
|
||||||
|
b.undo().unwrap();
|
||||||
|
assert_eq!(collect(&b), b"hello", "type-over undoes in one step");
|
||||||
|
assert!(matches!(b.undo(), Err(BufferError::NothingToUndo)));
|
||||||
|
});
|
||||||
|
|
||||||
dual_mode_test!(redo_replays_undone_edit, |_make, make_bytes| {
|
dual_mode_test!(redo_replays_undone_edit, |_make, make_bytes| {
|
||||||
let mut b = make_bytes("test", b"abc");
|
let mut b = make_bytes("test", b"abc");
|
||||||
b.apply_edit(EditOp::Insert {
|
b.apply_edit(EditOp::Insert {
|
||||||
|
|
|
||||||
|
|
@ -954,6 +954,32 @@ impl EditorCore {
|
||||||
self.active_window_mut().cursor += bytes.len() as u64;
|
self.active_window_mut().cursor += bytes.len() as u64;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// CUA type-over: insert `ch`, replacing the active region if one
|
||||||
|
/// exists. With a region this is a *single* `EditOp::Replace` — one
|
||||||
|
/// undo step — rather than the former `delete_region()` +
|
||||||
|
/// `insert_char()` pair, which recorded two. With no region it
|
||||||
|
/// delegates to [`Self::insert_char`] (a plain insert). The cursor
|
||||||
|
/// lands just past the inserted bytes and any selection is cleared.
|
||||||
|
pub fn insert_char_over_region(&mut self, ch: char) {
|
||||||
|
let Some((lo, hi)) = self.active_region() else {
|
||||||
|
self.insert_char(ch);
|
||||||
|
return;
|
||||||
|
};
|
||||||
|
self.active_window_mut().goal_col = None;
|
||||||
|
let mut buf = [0u8; 4];
|
||||||
|
let bytes = ch.encode_utf8(&mut buf).as_bytes();
|
||||||
|
if let Err(e) = self.apply_active_edit(EditOp::Replace {
|
||||||
|
range: Range { start: lo, end: hi },
|
||||||
|
bytes,
|
||||||
|
}) {
|
||||||
|
self.status = format!("replace failed: {e}");
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
let aw = self.active_window_mut();
|
||||||
|
aw.cursor = lo + bytes.len() as u64;
|
||||||
|
aw.selection = None;
|
||||||
|
}
|
||||||
|
|
||||||
/// Delete the codepoint immediately before the cursor.
|
/// Delete the codepoint immediately before the cursor.
|
||||||
pub fn backspace(&mut self) {
|
pub fn backspace(&mut self) {
|
||||||
self.active_window_mut().goal_col = None;
|
self.active_window_mut().goal_col = None;
|
||||||
|
|
|
||||||
|
|
@ -11411,6 +11411,17 @@ fn install_editing(editor: &Table, lua: &Lua, core: &SharedCore) -> mlua::Result
|
||||||
})?,
|
})?,
|
||||||
)?;
|
)?;
|
||||||
}
|
}
|
||||||
|
{
|
||||||
|
let cc = core.clone();
|
||||||
|
editor.set(
|
||||||
|
"insert_char_over_region",
|
||||||
|
lua.create_function(move |_, codepoint: i64| {
|
||||||
|
let ch = char_from_lua_codepoint(codepoint)?;
|
||||||
|
cc.borrow_mut().insert_char_over_region(ch);
|
||||||
|
Ok(())
|
||||||
|
})?,
|
||||||
|
)?;
|
||||||
|
}
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -147,6 +147,34 @@ fn typing_replaces_the_shift_selected_region() {
|
||||||
assert_eq!(cursor, 4);
|
assert_eq!(cursor, 4);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn type_over_is_a_single_undo_step() {
|
||||||
|
let mut s = EditorState::new();
|
||||||
|
type_str(&mut s, "hello");
|
||||||
|
|
||||||
|
// Select "llo" (region [2, 5)) and type 'X' → "heX".
|
||||||
|
for _ in 0..3 {
|
||||||
|
s.dispatch_key(FrontendId::LOCAL, key(KeyCode::Left, KeyModifiers::SHIFT));
|
||||||
|
}
|
||||||
|
s.dispatch_key(
|
||||||
|
FrontendId::LOCAL,
|
||||||
|
key(KeyCode::Char('X'), KeyModifiers::SHIFT),
|
||||||
|
);
|
||||||
|
let (text, _, _) = probe(&s);
|
||||||
|
assert_eq!(text, "heX");
|
||||||
|
|
||||||
|
// A single undo restores the pre-type-over text — not the
|
||||||
|
// half-replaced "hel" the old delete-then-insert pair left after
|
||||||
|
// one undo.
|
||||||
|
s.lua_host
|
||||||
|
.lua()
|
||||||
|
.load("pmacs.editor.undo()")
|
||||||
|
.exec()
|
||||||
|
.expect("undo");
|
||||||
|
let (text, _, _) = probe(&s);
|
||||||
|
assert_eq!(text, "hello", "type-over undoes in one step");
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn delete_forward_deletes_the_shift_selected_region() {
|
fn delete_forward_deletes_the_shift_selected_region() {
|
||||||
let mut s = EditorState::new();
|
let mut s = EditorState::new();
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue