From 4dbefcc41d303a60fe825d90e6066875b7f4c456 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 06:32:31 +0000 Subject: [PATCH] Undo/dirty correctness from the stress-testing review - Redoing an insert and undoing a delete put the cursor at col + size on the start row even when the text spans lines, i.e. past the end of the line. Later edits and their undo records then went to the wrong place (a join recorded at column 2 of 'a' re-split the text wrongly) and delete-word-prev read past the line (heap-buffer-overflow under ASan). The cursor now goes after the text's last newline, and undo and redo clamp it to the buffer. - A recovered buffer stays modified in every undo state (mark_base_unsaved): one edit and its undo made it look clean, and closing it then discarded the recovered text and its journal. - Save-and-quit records the saved undo state. - Visual-line selections are clamped to the buffer when read; the visual newline counts the cursor's own line only if it was selected. A stale selection put the cursor rows past the end, where typed text could not be undone. - The yank size cap accounts for visual-line yanks (once per line). - After an exception escapes a command, open undo groups are closed so later edits do not join them. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB --- Buffer.h | 20 ++++++- Command.cc | 29 +++++++++-- Editor.cc | 4 ++ UndoSystem.cc | 53 ++++++++++++++++--- UndoSystem.h | 9 ++++ tests/test_audit_regressions.cc | 84 ++++++++++++++++++++++++++++++ tests/test_swap_recovery_prompt.cc | 26 +++++++++ 7 files changed, 213 insertions(+), 12 deletions(-) diff --git a/Buffer.h b/Buffer.h index 50394a9..17e8b46 100644 --- a/Buffer.h +++ b/Buffer.h @@ -482,15 +482,22 @@ public: } + // The selected rows, clamped to the buffer: they are stored as row numbers + // and go stale when lines are removed, and commands must never act on rows + // past the end (edits there were misplaced and could not be undone). [[nodiscard]] std::size_t VisualLineStartY() const { - return visual_line_anchor_y_ < visual_line_active_y_ ? visual_line_anchor_y_ : visual_line_active_y_; + const std::size_t y = visual_line_anchor_y_ < visual_line_active_y_ ? visual_line_anchor_y_ + : visual_line_active_y_; + return clamp_row_(y); } [[nodiscard]] std::size_t VisualLineEndY() const { - return visual_line_anchor_y_ < visual_line_active_y_ ? visual_line_active_y_ : visual_line_anchor_y_; + const std::size_t y = visual_line_anchor_y_ < visual_line_active_y_ ? visual_line_active_y_ + : visual_line_anchor_y_; + return clamp_row_(y); } @@ -513,6 +520,15 @@ public: [[nodiscard]] std::string AsString() const; +private: + [[nodiscard]] std::size_t clamp_row_(std::size_t y) const + { + const std::size_t n = Nrows(); + return (n == 0) ? 0 : (y < n ? y : n - 1); + } + +public: + // Syntax highlighting integration (per-buffer) [[nodiscard]] std::uint64_t Version() const { diff --git a/Command.cc b/Command.cc index db76b18..c45e0ca 100644 --- a/Command.cc +++ b/Command.cc @@ -1120,6 +1120,8 @@ cmd_save_and_quit(CommandContext &ctx) buf->SetDirty(false); if (auto *sm = ctx.editor.Swap()) sm->ResetJournal(*buf); + if (auto *u = buf->Undo()) + u->mark_saved(); } else { ctx.editor.SetStatus(err); return false; @@ -1129,6 +1131,8 @@ cmd_save_and_quit(CommandContext &ctx) buf->SetDirty(false); if (auto *sm = ctx.editor.Swap()) sm->ResetJournal(*buf); + if (auto *u = buf->Undo()) + u->mark_saved(); } else { ctx.editor.SetStatus(err); return false; @@ -3420,10 +3424,13 @@ cmd_newline(CommandContext &ctx) buf->SetDirty(true); // Cursor: end up on the final inserted line for the original cursor line. - // Each selected line above the cursor gained `repeat` lines. - std::size_t new_y = y + static_cast(repeat); - new_y += splits_above * static_cast(repeat); + // Each selected line above the cursor gained `repeat` lines, and the + // cursor's own line only if it was selected. + const bool own_split = (y >= sy && y <= ey); + std::size_t new_y = y + (own_split ? static_cast(repeat) : 0); + new_y += splits_above * static_cast(repeat); buf->SetCursor(0, new_y); + clamp_cursor_to_buffer(*buf); ensure_cursor_visible(ctx.editor, *buf); return true; } @@ -3770,6 +3777,7 @@ cmd_undo(CommandContext &ctx) for (int i = 0; i < repeat; ++i) u->undo(); // Keep cursor within buffer bounds + clamp_cursor_to_buffer(*buf); // never leave the cursor past a line or the buffer ensure_cursor_visible(ctx.editor, *buf); ctx.editor.SetStatus("Undone"); return true; @@ -3795,6 +3803,7 @@ cmd_redo(CommandContext &ctx) } else { u->redo(); } + clamp_cursor_to_buffer(*buf); // never leave the cursor past a line or the buffer ensure_cursor_visible(ctx.editor, *buf); ctx.editor.SetStatus("Redone"); return true; @@ -3971,7 +3980,10 @@ cmd_yank(CommandContext &ctx) // Bound the total: C-u 1000000 C-y of a large kill would allocate // gigabytes (and bad_alloc or the OOM killer end the session). constexpr std::size_t kMaxYankBytes = std::size_t{256} << 20; - if (text.size() * static_cast(repeat) > kMaxYankBytes) { + // Visual-line yank inserts the text once per selected line. + const std::size_t yank_lines = + buf->VisualLineActive() ? (buf->VisualLineEndY() - buf->VisualLineStartY() + 1) : 1; + if (text.size() * static_cast(repeat) * yank_lines > kMaxYankBytes) { ctx.editor.SetStatus("Yank too large (" + std::to_string(repeat) + " x " + std::to_string(text.size()) + " bytes)"); return false; @@ -5673,12 +5685,21 @@ run_handler(Editor &ed, const Command &cmd, CommandContext &ctx) { if (!cmd.handler) return false; + // An exception can leave undo groups opened by hand (BeginGroup without + // its EndGroup); close them, or every later edit would join that group. + auto close_undo_groups = [&ed] { + if (Buffer *b = ed.CurrentBuffer()) + if (UndoSystem *u = b->Undo()) + u->AbortGroups(); + }; try { return cmd.handler(ctx); } catch (const std::exception &e) { + close_undo_groups(); kte::ErrorHandler::Instance().Error("Command", std::string(cmd.name) + ": " + e.what(), ""); ed.SetStatus(std::string("Error in ") + cmd.name + ": " + e.what()); } catch (...) { + close_undo_groups(); kte::ErrorHandler::Instance().Error("Command", std::string(cmd.name) + ": unknown exception", ""); ed.SetStatus(std::string("Error in ") + cmd.name); } diff --git a/Editor.cc b/Editor.cc index 5dffa53..82e64c4 100644 --- a/Editor.cc +++ b/Editor.cc @@ -435,6 +435,10 @@ Editor::ResolveRecoveryPrompt(const bool yes) } catch (...) { // Best effort; the replay itself succeeded. } + // Nothing in the undo history matches the file on disk now: keep + // the buffer dirty even after undoing back to the loaded state. + if (auto *u = b->Undo()) + u->mark_base_unsaved(); b->SetDirty(true); apply_pending_line(*this, req.line1); SetStatus("Recovered " + req.path); diff --git a/UndoSystem.cc b/UndoSystem.cc index ba1cc95..389cbd0 100644 --- a/UndoSystem.cc +++ b/UndoSystem.cc @@ -247,7 +247,25 @@ void UndoSystem::mark_saved() { commit(); - tree_.saved = tree_.current; + tree_.saved = tree_.current; + base_unsaved_ = false; + update_dirty_flag(); +} + + +void +UndoSystem::AbortGroups() +{ + commit(); + group_depth_ = 0; + active_group_id_ = 0; +} + + +void +UndoSystem::mark_base_unsaved() +{ + base_unsaved_ = true; update_dirty_flag(); } @@ -272,6 +290,7 @@ UndoSystem::clear() tree_.root = nullptr; tree_.current = nullptr; tree_.saved = nullptr; + base_unsaved_ = false; active_group_id_ = 0; group_depth_ = 0; next_group_id_ = 1; @@ -279,9 +298,31 @@ UndoSystem::clear() } +// Position just after `node->text` inserted at (node->col, node->row). The text +// may span lines: the column is then the length after its last newline, not +// col + size (which left the cursor past the end of the line, so later edits +// and their undo records went to the wrong place). +static void +end_of_text(const UndoNode *node, std::size_t &x, std::size_t &y) +{ + y = static_cast(node->row); + x = static_cast(node->col); + const auto nl = node->text.rfind('\n'); + if (nl == std::string::npos) { + x += node->text.size(); + return; + } + for (char c: node->text) + if (c == '\n') + ++y; + x = node->text.size() - nl - 1; +} + + void UndoSystem::apply(const UndoNode *node, int direction) { + std::size_t ex = 0, ey = 0; if (!node) return; // Cursor positioning: keep the point at a sensible location after undo/redo. @@ -291,8 +332,8 @@ UndoSystem::apply(const UndoNode *node, int direction) case UndoType::Paste: if (direction > 0) { buf_->insert_text(node->row, node->col, node->text); - buf_->SetCursor(static_cast(node->col + node->text.size()), - static_cast(node->row)); + end_of_text(node, ex, ey); + buf_->SetCursor(ex, ey); } else { buf_->delete_text(node->row, node->col, node->text.size()); buf_->SetCursor(static_cast(node->col), static_cast(node->row)); @@ -304,8 +345,8 @@ UndoSystem::apply(const UndoNode *node, int direction) buf_->SetCursor(static_cast(node->col), static_cast(node->row)); } else { buf_->insert_text(node->row, node->col, node->text); - buf_->SetCursor(static_cast(node->col + node->text.size()), - static_cast(node->row)); + end_of_text(node, ex, ey); + buf_->SetCursor(ex, ey); } break; case UndoType::Newline: @@ -409,7 +450,7 @@ void UndoSystem::update_dirty_flag() { // dirty if current != saved - bool dirty = (tree_.current != tree_.saved); + bool dirty = base_unsaved_ || (tree_.current != tree_.saved); buf_->SetDirty(dirty); } diff --git a/UndoSystem.h b/UndoSystem.h index 4102d69..b4d827d 100644 --- a/UndoSystem.h +++ b/UndoSystem.h @@ -76,6 +76,14 @@ public: void mark_saved(); + // No undo state matches the file on disk any more (e.g. after crash + // recovery replayed edits onto it): the buffer stays dirty in every state + // until the next mark_saved() or clear(). + void mark_base_unsaved(); + + // Close any open group (after an interrupted command) and seal pending. + void AbortGroups(); + void discard_pending(); void clear(); @@ -115,6 +123,7 @@ private: std::uint64_t active_group_id_ = 0; int group_depth_ = 0; // nesting depth of BeginGroup/EndGroup + bool base_unsaved_ = false; // see mark_base_unsaved() std::uint64_t next_group_id_ = 1; Buffer *buf_; diff --git a/tests/test_audit_regressions.cc b/tests/test_audit_regressions.cc index 50cb211..52feaff 100644 --- a/tests/test_audit_regressions.cc +++ b/tests/test_audit_regressions.cc @@ -1026,3 +1026,87 @@ TEST(Audit_Yank_HugeRepeatRefused) ASSERT_EQ(h.Buf().Nrows(), (std::size_t) 1); ASSERT_EQ(h.Line(0), std::string()); } + + +// Undoing a multi-line kill puts the cursor after the restored text (it was +// placed at col + size on the start row, past the end of the line, so later +// edits and their undo went to the wrong place). +TEST(Audit_Undo_MultiLineRestore_CursorAfterText) +{ + TestHarness h; + Buffer &b = h.Buf(); + b.insert_text(0, 0, "a\nb\n"); + b.SetCursor(0, 0); + ASSERT_TRUE(h.Exec(CommandId::ToggleMark)); + ASSERT_TRUE(h.Exec(CommandId::MoveDown)); + ASSERT_TRUE(h.Exec(CommandId::KillRegion)); + ASSERT_EQ(h.Text(), std::string("b\n")); + ASSERT_TRUE(h.Undo()); + ASSERT_EQ(h.Text(), std::string("a\nb\n")); + ASSERT_EQ(b.Cury(), (std::size_t) 1); + ASSERT_EQ(b.Curx(), (std::size_t) 0); + ASSERT_TRUE(h.Exec(CommandId::KillLine)); + ASSERT_TRUE(h.Undo()); + ASSERT_EQ(h.Text(), std::string("a\nb\n")); +} + + +// Same shape that overflowed the heap in delete-word-prev: after the undo +// the cursor is within its line. +TEST(Audit_Undo_ThenDeleteWordPrev_NoOverflow) +{ + TestHarness h; + Buffer &b = h.Buf(); + b.insert_text(0, 0, std::string(24, 'a') + "\n" + std::string(30, 'b')); + b.SetCursor(0, 0); + ASSERT_TRUE(h.Exec(CommandId::ToggleMark)); + ASSERT_TRUE(h.Exec(CommandId::MoveDown)); + ASSERT_TRUE(h.Exec(CommandId::MoveEnd)); + ASSERT_TRUE(h.Exec(CommandId::KillRegion)); + ASSERT_TRUE(h.Undo()); + ASSERT_TRUE(b.Curx() <= h.Line(b.Cury()).size()); + ASSERT_TRUE(h.Exec(CommandId::DeleteWordPrev)); +} + + +// Save-and-quit records the saved state, so undo afterwards marks the buffer +// dirty again (it differs from the file). +TEST(Audit_SaveAndQuit_MarksSaved) +{ + TempDir d("saq_marksaved"); + std::ofstream(d.path / "a.txt") << "aaa\n"; + TestHarness h; + Editor &ed = h.EditorRef(); + h.Buf().insert_text(0, 0, "dirty other"); + h.Buf().SetDirty(true); + std::string err; + ASSERT_TRUE(ed.OpenFile((d.path / "a.txt").string(), err)); + ASSERT_TRUE(h.Exec(CommandId::InsertText, "X")); + ASSERT_TRUE(h.Exec(CommandId::SaveAndQuit)); + ASSERT_TRUE(!ed.QuitRequested()); // other buffer still dirty + ASSERT_EQ(slurp(d.path / "a.txt"), std::string("Xaaa\n")); + ASSERT_TRUE(!ed.CurrentBuffer()->Dirty()); + ASSERT_TRUE(h.Undo()); + ASSERT_TRUE(ed.CurrentBuffer()->Dirty()); +} + + +// Visual-line newline with a stale selection never puts the cursor past the +// buffer, so later typing is undoable. +TEST(Audit_VisualLine_StaysInsideBuffer) +{ + TestHarness h; + Buffer &b = h.Buf(); + b.insert_text(0, 0, "one\ntwo\nthree\n"); + b.SetCursor(0, 0); + const std::string orig = h.Text(); + ASSERT_TRUE(h.Exec(CommandId::VisualLineModeToggle)); + ASSERT_TRUE(h.Exec(CommandId::MarkAllAndJumpEnd)); + ASSERT_TRUE(h.Exec(CommandId::Newline)); + ASSERT_TRUE(h.Exec(CommandId::VisualLineModeToggle)); + ASSERT_TRUE(b.Cury() < b.Nrows()); + ASSERT_TRUE(h.Exec(CommandId::InsertText, "Z")); + for (int i = 0; i < 10; ++i) + (void) h.Undo(); + ASSERT_EQ(h.Text(), orig); +} diff --git a/tests/test_swap_recovery_prompt.cc b/tests/test_swap_recovery_prompt.cc index 67208aa..42e4a40 100644 --- a/tests/test_swap_recovery_prompt.cc +++ b/tests/test_swap_recovery_prompt.cc @@ -679,3 +679,29 @@ TEST(SwapRecoveryPrompt_TouchedFile_StillRecoverable) answer(ed, "y"); ASSERT_EQ(buffer_bytes_via_views(*ed.CurrentBuffer()), expected); } + + +// A recovered buffer stays modified even after an edit and its undo: its +// content differs from the file on disk in every undo state. +TEST(SwapRecoveryPrompt_Recovered_StaysDirtyAfterUndo) +{ + ktet::InstallDefaultCommandsOnce(); + XdgSandbox sb("recovered_dirty"); + const std::string file = (sb.root / "work" / "rd.txt").string(); + write_file_bytes(file, "base\n"); + std::string expected; + (void) make_journal(file, [](Buffer &b) { + b.insert_text(0, 0, std::string("IMPORTANT ")); + }, expected); + + Editor ed; + ed.SetDimensions(24, 80); + ed.AddBuffer(Buffer()); + ed.RequestOpenFile(file); + (void) ed.ProcessPendingOpens(); + answer(ed, "y"); + ASSERT_TRUE(ed.CurrentBuffer()->Dirty()); + ASSERT_TRUE(Execute(ed, CommandId::InsertText, "a")); + ASSERT_TRUE(Execute(ed, CommandId::Undo)); + ASSERT_TRUE(ed.CurrentBuffer()->Dirty()); +}