From ac0eadc34537ea23cec67ed8abbb314da795e366 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Tue, 10 Feb 2026 22:39:55 -0800 Subject: [PATCH] Add undo system with coalescing logic and comprehensive tests. - Implemented robust undo system supporting coalescing of text operations (insert, backspace, delete). - Added `UndoSystem` integration into the editor/commands pipeline. - Wrote extensive unit tests for various undo/redo scenarios, including multiline operations, cursor preservation, and history management. - Refactored to ensure consistent cursor behavior during undo/redo actions. - Updated CMake to include new tests. --- CMakeLists.txt | 1 + Command.cc | 27 ++- UndoSystem.cc | 186 ++++++++++++++++-- UndoSystem.h | 7 + tests/test_undo.cc | 343 +++++++++++++++++++++++++++++++++ tests/test_visual_line_mode.cc | 33 ++++ 6 files changed, 571 insertions(+), 26 deletions(-) create mode 100644 tests/test_undo.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 9b87122..1a46453 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -302,6 +302,7 @@ if (BUILD_TESTS) tests/test_piece_table.cc tests/test_search.cc tests/test_reflow_paragraph.cc + tests/test_undo.cc tests/test_visual_line_mode.cc # minimal engine sources required by Buffer diff --git a/Command.cc b/Command.cc index 65e3b6f..5be1a9b 100644 --- a/Command.cc +++ b/Command.cc @@ -2009,27 +2009,22 @@ cmd_insert_text(CommandContext &ctx) return true; } - std::size_t ins_y = y; - std::size_t ins_x = x; // remember insertion start for undo positioning - + UndoSystem *u = buf->Undo(); + if (u) { + // Start/extend a typed-run batch. Do NOT commit here; commit happens on boundaries + // (cursor movement, prompts, undo/redo, etc.) so consecutive InsertText commands coalesce. + buf->SetCursor(x, y); + u->Begin(UndoType::Insert); + } // Apply edits to the underlying PieceTable through Buffer::insert_text, // not directly to the legacy rows_ cache. This ensures Save() persists text. for (int i = 0; i < repeat; ++i) { buf->insert_text(static_cast(y), static_cast(x), std::string_view(ctx.arg)); + if (u) + u->Append(std::string_view(ctx.arg)); x += ctx.arg.size(); } buf->SetDirty(true); - // Record undo for this contiguous insert at the original insertion point - if (auto *u = buf->Undo()) { - // Position cursor at insertion start for the undo record - buf->SetCursor(ins_x, ins_y); - u->Begin(UndoType::Insert); - for (int i = 0; i < repeat; ++i) { - u->Append(std::string_view(ctx.arg)); - } - // Finalize this contiguous insert as a single undoable action - u->commit(); - } buf->SetCursor(x, y); ensure_cursor_visible(ctx.editor, *buf); return true; @@ -3217,6 +3212,10 @@ cmd_yank(CommandContext &ctx) for (int i = 0; i < repeat; ++i) { insert_text_at_cursor(*buf, text); } + // Yank is a paste operation; it should clear the mark/region and any selection highlighting. + buf->ClearMark(); + if (buf->VisualLineActive()) + buf->VisualLineClear(); ensure_cursor_visible(ctx.editor, *buf); // Start a new kill chain only from kill commands; yanking should break it ctx.editor.SetKillChain(false); diff --git a/UndoSystem.cc b/UndoSystem.cc index 8335951..9ebf8b3 100644 --- a/UndoSystem.cc +++ b/UndoSystem.cc @@ -11,66 +11,216 @@ UndoSystem::UndoSystem(Buffer &owner, UndoTree &tree) void UndoSystem::Begin(UndoType type) { - // STUB: Undo system incomplete - disabled until it can be properly implemented - (void) type; + if (!buf_) + return; + const int row = static_cast(buf_->Cury()); + const int col = static_cast(buf_->Curx()); + + // Some operations should always be standalone undo steps. + const bool always_standalone = (type == UndoType::Newline || type == UndoType::DeleteRow); + if (always_standalone) { + commit(); + } + + if (tree_.pending) { + if (tree_.pending->type == type) { + // Typed-run coalescing rules. + switch (type) { + case UndoType::Insert: + case UndoType::Paste: { + // Cursor must be at the end of the pending insert. + if (tree_.pending->row == row + && col == tree_.pending->col + static_cast(tree_.pending->text.size())) { + pending_mode_ = PendingAppendMode::Append; + return; + } + break; + } + case UndoType::Delete: { + if (tree_.pending->row == row) { + // Two common delete shapes: + // 1) backspace-run: cursor moves left each time (so new col is pending.col - 1) + // 2) delete-run: cursor stays, always deleting at the same col + if (col == tree_.pending->col) { + pending_mode_ = PendingAppendMode::Append; + return; + } + if (col + 1 == tree_.pending->col) { + // Extend a backspace run to the left; update the start column now. + tree_.pending->col = col; + pending_mode_ = PendingAppendMode::Prepend; + return; + } + } + break; + } + case UndoType::Newline: + case UndoType::DeleteRow: + break; + } + } + // Can't coalesce: seal the previous pending step. + commit(); + } + + // Start a new pending node. + tree_.pending = new UndoNode{}; + tree_.pending->type = type; + tree_.pending->row = row; + tree_.pending->col = col; + tree_.pending->text.clear(); + tree_.pending->child = nullptr; + tree_.pending->next = nullptr; + pending_mode_ = PendingAppendMode::Append; } void UndoSystem::Append(char ch) { - // STUB: Undo system incomplete - disabled until it can be properly implemented - (void) ch; + if (!tree_.pending) + return; + if (pending_mode_ == PendingAppendMode::Prepend) { + tree_.pending->text.insert(tree_.pending->text.begin(), ch); + } else { + tree_.pending->text.push_back(ch); + } } void UndoSystem::Append(std::string_view text) { - // STUB: Undo system incomplete - disabled until it can be properly implemented - (void) text; + if (!tree_.pending) + return; + if (text.empty()) + return; + if (pending_mode_ == PendingAppendMode::Prepend) { + tree_.pending->text.insert(0, text.data(), text.size()); + } else { + tree_.pending->text.append(text.data(), text.size()); + } } void UndoSystem::commit() { - // STUB: Undo system incomplete - disabled until it can be properly implemented + if (!tree_.pending) + return; + + // Drop empty text batches for text-based operations. + if ((tree_.pending->type == UndoType::Insert || tree_.pending->type == UndoType::Delete + || tree_.pending->type == UndoType::Paste) + && tree_.pending->text.empty()) { + delete tree_.pending; + tree_.pending = nullptr; + pending_mode_ = PendingAppendMode::Append; + return; + } + + // Linear semantics: if we are not at the tip, discard redo. + if (tree_.current && tree_.current->child) { + // Prevent dangling `saved` pointer if it sits in the discarded redo chain. + if (tree_.saved && is_descendant(tree_.current->child, tree_.saved)) { + tree_.saved = nullptr; + } + free_branch(tree_.current->child); + tree_.current->child = nullptr; + } + + if (!tree_.root) { + tree_.root = tree_.pending; + tree_.current = tree_.pending; + } else if (!tree_.current) { + // We are at the "pre-first-edit" state. Attach as the new root child. + // For v1 linear history, this means starting the chain anew. + // The existing root represents edits from the past; attach the new node as the new root. + // (This situation happens after undoing past the first node.) + if (tree_.saved && is_descendant(tree_.root, tree_.saved)) { + // ok + } + // Discard the old root chain because it is redo from the pre-edit state. + if (tree_.saved && is_descendant(tree_.root, tree_.saved)) { + tree_.saved = nullptr; + } + free_node(tree_.root); + tree_.root = tree_.pending; + tree_.current = tree_.pending; + } else { + tree_.current->child = tree_.pending; + tree_.current = tree_.pending; + } + + tree_.pending = nullptr; + pending_mode_ = PendingAppendMode::Append; + update_dirty_flag(); } void UndoSystem::undo() { - // STUB: Undo system incomplete - disabled until it can be properly implemented + // Seal any in-progress typed run before undo. + commit(); + if (!tree_.current) + return; + debug_log("undo"); + apply(tree_.current, -1); + UndoNode *parent = find_parent(tree_.root, tree_.current); + tree_.current = parent; + update_dirty_flag(); } void UndoSystem::redo() { - // STUB: Undo system incomplete - disabled until it can be properly implemented + commit(); + UndoNode *next = nullptr; + if (!tree_.current) { + next = tree_.root; + } else { + next = tree_.current->child; + } + if (!next) + return; + debug_log("redo"); + apply(next, +1); + tree_.current = next; + update_dirty_flag(); } void UndoSystem::mark_saved() { - // STUB: Undo system incomplete - disabled until it can be properly implemented + commit(); + tree_.saved = tree_.current; + update_dirty_flag(); } void UndoSystem::discard_pending() { - // STUB: Undo system incomplete - disabled until it can be properly implemented + if (tree_.pending) { + delete tree_.pending; + tree_.pending = nullptr; + } + pending_mode_ = PendingAppendMode::Append; } void UndoSystem::clear() { - // STUB: Undo system incomplete - disabled until it can be properly implemented + discard_pending(); + free_node(tree_.root); + tree_.root = nullptr; + tree_.current = nullptr; + tree_.saved = nullptr; + update_dirty_flag(); } @@ -79,34 +229,46 @@ UndoSystem::apply(const UndoNode *node, int direction) { if (!node) return; + // Cursor positioning: keep the point at a sensible location after undo/redo. + // Low-level Buffer edit primitives do not move the cursor. switch (node->type) { case UndoType::Insert: 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)); } else { buf_->delete_text(node->row, node->col, node->text.size()); + buf_->SetCursor(static_cast(node->col), static_cast(node->row)); } break; case UndoType::Delete: if (direction > 0) { buf_->delete_text(node->row, node->col, node->text.size()); + 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)); } break; case UndoType::Newline: if (direction > 0) { buf_->split_line(node->row, node->col); + buf_->SetCursor(0, static_cast(node->row + 1)); } else { buf_->join_lines(node->row); + buf_->SetCursor(static_cast(node->col), static_cast(node->row)); } break; case UndoType::DeleteRow: if (direction > 0) { buf_->delete_row(node->row); + buf_->SetCursor(0, static_cast(node->row)); } else { buf_->insert_row(node->row, node->text); + buf_->SetCursor(0, static_cast(node->row)); } break; } diff --git a/UndoSystem.h b/UndoSystem.h index 506cabf..65193ba 100644 --- a/UndoSystem.h +++ b/UndoSystem.h @@ -33,6 +33,11 @@ public: void UpdateBufferReference(Buffer &new_buf); private: + enum class PendingAppendMode : std::uint8_t { + Append, + Prepend, + }; + void apply(const UndoNode *node, int direction); // +1 redo, -1 undo void free_node(UndoNode *node); @@ -48,6 +53,8 @@ private: void update_dirty_flag(); + PendingAppendMode pending_mode_ = PendingAppendMode::Append; + Buffer *buf_; UndoTree &tree_; }; \ No newline at end of file diff --git a/tests/test_undo.cc b/tests/test_undo.cc new file mode 100644 index 0000000..fd04d01 --- /dev/null +++ b/tests/test_undo.cc @@ -0,0 +1,343 @@ +#include "Test.h" +#include "Buffer.h" + + +TEST (Undo_InsertRun_Coalesces) +{ + Buffer b; + UndoSystem *u = b.Undo(); + ASSERT_TRUE(u != nullptr); + + // Simulate two separate "typed" insert commands without committing in between. + b.SetCursor(0, 0); + u->Begin(UndoType::Insert); + b.insert_text(0, 0, std::string_view("h")); + u->Append('h'); + b.SetCursor(1, 0); + + u->Begin(UndoType::Insert); + b.insert_text(0, 1, std::string_view("i")); + u->Append('i'); + b.SetCursor(2, 0); + + u->commit(); + ASSERT_EQ(b.Rows().size(), (std::size_t) 1); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("hi")); + + u->undo(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("")); +} + + +TEST (Undo_BackspaceRun_Coalesces) +{ + Buffer b; + UndoSystem *u = b.Undo(); + ASSERT_TRUE(u != nullptr); + + // Seed content. + b.insert_text(0, 0, std::string_view("abc")); + b.SetCursor(3, 0); + u->mark_saved(); + + // Simulate two backspaces: delete 'c' then 'b'. + { + const auto &rows = b.Rows(); + char deleted = rows[0][2]; + b.delete_text(0, 2, 1); + b.SetCursor(2, 0); + u->Begin(UndoType::Delete); + u->Append(deleted); + } + { + const auto &rows = b.Rows(); + char deleted = rows[0][1]; + b.delete_text(0, 1, 1); + b.SetCursor(1, 0); + u->Begin(UndoType::Delete); + u->Append(deleted); + } + + u->commit(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("a")); + + // One undo should restore both characters. + u->undo(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("abc")); +} + + +TEST (Undo_Linear_RedoDiscardedAfterNewEdit) +{ + Buffer b; + UndoSystem *u = b.Undo(); + ASSERT_TRUE(u != nullptr); + + b.SetCursor(0, 0); + u->Begin(UndoType::Insert); + b.insert_text(0, 0, std::string_view("a")); + u->Append('a'); + b.SetCursor(1, 0); + u->commit(); + + u->Begin(UndoType::Insert); + b.insert_text(0, 1, std::string_view("b")); + u->Append('b'); + b.SetCursor(2, 0); + u->commit(); + + ASSERT_EQ(std::string(b.Rows()[0]), std::string("ab")); + + u->undo(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("a")); + + // New edit after undo should discard redo. + u->Begin(UndoType::Insert); + b.insert_text(0, 1, std::string_view("c")); + u->Append('c'); + b.SetCursor(2, 0); + u->commit(); + + ASSERT_EQ(std::string(b.Rows()[0]), std::string("ac")); + u->redo(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("ac")); +} + + +TEST (Undo_DirtyFlag_MarkSavedAndUndoRedo) +{ + Buffer b; + UndoSystem *u = b.Undo(); + ASSERT_TRUE(u != nullptr); + + u->mark_saved(); + ASSERT_TRUE(!b.Dirty()); + + b.SetCursor(0, 0); + u->Begin(UndoType::Insert); + b.insert_text(0, 0, std::string_view("x")); + u->Append('x'); + b.SetCursor(1, 0); + u->commit(); + + ASSERT_TRUE(b.Dirty()); + u->undo(); + ASSERT_TRUE(!b.Dirty()); + u->redo(); + ASSERT_TRUE(b.Dirty()); +} + + +TEST (Undo_Newline_UndoRedo_SplitJoin) +{ + Buffer b; + UndoSystem *u = b.Undo(); + ASSERT_TRUE(u != nullptr); + + // Seed a single line and split it. + b.insert_text(0, 0, std::string_view("hello")); + b.SetCursor(2, 0); // split after "he" + u->Begin(UndoType::Newline); + b.split_line(0, 2); + u->commit(); + + ASSERT_EQ(b.Rows().size(), (std::size_t) 2); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("he")); + ASSERT_EQ(std::string(b.Rows()[1]), std::string("llo")); + + // Undo should join the lines back. + u->undo(); + ASSERT_EQ(b.Rows().size(), (std::size_t) 1); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("hello")); + + // Redo should split again at the same point. + u->redo(); + ASSERT_EQ(b.Rows().size(), (std::size_t) 2); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("he")); + ASSERT_EQ(std::string(b.Rows()[1]), std::string("llo")); +} + + +TEST (Undo_DeleteKeyRun_Coalesces) +{ + Buffer b; + UndoSystem *u = b.Undo(); + ASSERT_TRUE(u != nullptr); + + // Seed content: delete-key semantics keep cursor at the same column. + b.insert_text(0, 0, std::string_view("abcd")); + b.SetCursor(1, 0); // on 'b' + + // Delete 'b' + { + const auto &rows = b.Rows(); + char deleted = rows[0][1]; + u->Begin(UndoType::Delete); + b.delete_text(0, 1, 1); + u->Append(deleted); + b.SetCursor(1, 0); + } + // Delete next char (was 'c', now at same col=1) + { + const auto &rows = b.Rows(); + char deleted = rows[0][1]; + u->Begin(UndoType::Delete); + b.delete_text(0, 1, 1); + u->Append(deleted); + b.SetCursor(1, 0); + } + + u->commit(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("ad")); + + // One undo should restore both deleted characters. + u->undo(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("abcd")); +} + + +TEST (Undo_UndoPastFirstEdit_RedoFromPreFirstEdit) +{ + Buffer b; + UndoSystem *u = b.Undo(); + ASSERT_TRUE(u != nullptr); + + // Commit two separate insert edits. + b.SetCursor(0, 0); + u->Begin(UndoType::Insert); + b.insert_text(0, 0, std::string_view("a")); + u->Append('a'); + b.SetCursor(1, 0); + u->commit(); + + b.SetCursor(1, 0); + u->Begin(UndoType::Insert); + b.insert_text(0, 1, std::string_view("b")); + u->Append('b'); + b.SetCursor(2, 0); + u->commit(); + + ASSERT_EQ(std::string(b.Rows()[0]), std::string("ab")); + + // Undo twice: we should reach the pre-first-edit state. + u->undo(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("a")); + u->undo(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("")); + + // Redo twice should restore both edits. + u->redo(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("a")); + u->redo(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("ab")); +} + + +TEST (Undo_NewEditFromPreFirstEdit_DiscardsOldHistory) +{ + Buffer b; + UndoSystem *u = b.Undo(); + ASSERT_TRUE(u != nullptr); + + // Build up two edits. + b.SetCursor(0, 0); + u->Begin(UndoType::Insert); + b.insert_text(0, 0, std::string_view("a")); + u->Append('a'); + b.SetCursor(1, 0); + u->commit(); + + b.SetCursor(1, 0); + u->Begin(UndoType::Insert); + b.insert_text(0, 1, std::string_view("b")); + u->Append('b'); + b.SetCursor(2, 0); + u->commit(); + + ASSERT_EQ(std::string(b.Rows()[0]), std::string("ab")); + + // Undo past first edit so current becomes null. + u->undo(); + u->undo(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("")); + + // Commit a new edit from the pre-first-edit state. + b.SetCursor(0, 0); + u->Begin(UndoType::Insert); + b.insert_text(0, 0, std::string_view("x")); + u->Append('x'); + b.SetCursor(1, 0); + u->commit(); + + ASSERT_EQ(std::string(b.Rows()[0]), std::string("x")); + + // Old history should be gone: redo should not resurrect "ab". + u->redo(); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("x")); +} + + +TEST (Undo_MultiLineDelete_ConsumesNewline_UndoRestores) +{ + Buffer b; + UndoSystem *u = b.Undo(); + ASSERT_TRUE(u != nullptr); + + // Create two lines. PieceTable treats '\n' between logical lines. + b.insert_text(0, 0, std::string_view("ab\ncd")); + ASSERT_EQ(b.Rows().size(), (std::size_t) 2); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("ab")); + ASSERT_EQ(std::string(b.Rows()[1]), std::string("cd")); + + // Delete spanning the newline: delete "b\n" starting at (0,1). + b.SetCursor(1, 0); + u->Begin(UndoType::Delete); + b.delete_text(0, 1, 2); + u->Append(std::string_view("b\n")); + u->commit(); + + ASSERT_EQ(b.Rows().size(), (std::size_t) 1); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("acd")); + + // Undo should restore exact original text/line structure. + u->undo(); + ASSERT_EQ(b.Rows().size(), (std::size_t) 2); + ASSERT_EQ(std::string(b.Rows()[0]), std::string("ab")); + ASSERT_EQ(std::string(b.Rows()[1]), std::string("cd")); +} + + +TEST (Undo_DeleteIndent_UndoRestoresCursorAtText) +{ + Buffer b; + UndoSystem *u = b.Undo(); + ASSERT_TRUE(u != nullptr); + + // Seed 3-line content with indentation on the middle line. + b.insert_text(0, 0, + std::string_view("I did a thing\n and then I edited a thing\nbut there were gaps")); + ASSERT_EQ(b.Rows().size(), (std::size_t) 3); + + // Cursor at start of the line (before spaces), then C-d C-d deletes two spaces. + b.SetCursor(0, 1); + for (int i = 0; i < 2; ++i) { + const auto &rows = b.Rows(); + char deleted = rows[1][0]; + ASSERT_EQ(deleted, ' '); + u->Begin(UndoType::Delete); + b.delete_text(1, 0, 1); + u->Append(deleted); + b.SetCursor(0, 1); // delete-key keeps col the same + } + u->commit(); + + ASSERT_EQ(std::string(b.Rows()[1]), std::string("and then I edited a thing")); + ASSERT_EQ(b.Cury(), (std::size_t) 1); + ASSERT_EQ(b.Curx(), (std::size_t) 0); + + // Undo should restore indentation, and keep cursor on the text (at 'a'), not at EOL. + u->undo(); + ASSERT_EQ(std::string(b.Rows()[1]), std::string(" and then I edited a thing")); + ASSERT_EQ(b.Cury(), (std::size_t) 1); + ASSERT_EQ(b.Curx(), (std::size_t) 2); +} \ No newline at end of file diff --git a/tests/test_visual_line_mode.cc b/tests/test_visual_line_mode.cc index 5dafaab..f41856f 100644 --- a/tests/test_visual_line_mode.cc +++ b/tests/test_visual_line_mode.cc @@ -122,4 +122,37 @@ TEST (VisualLineMode_CancelWithCtrlG) std::cerr << "Got (len=" << got.size() << ") bytes: " << dump_bytes(got) << "\n"; } ASSERT_TRUE(got == exp); +} + + +TEST (Yank_ClearsMarkAndVisualLine) +{ + InstallDefaultCommands(); + + Editor ed; + ed.SetDimensions(24, 80); + + Buffer b; + b.insert_text(0, 0, "foo\nbar\n"); + b.SetCursor(1, 0); + ed.AddBuffer(std::move(b)); + + ASSERT_TRUE(ed.CurrentBuffer() != nullptr); + Buffer *buf = ed.CurrentBuffer(); + + // Seed mark + visual-line highlighting. + buf->SetMark(buf->Curx(), buf->Cury()); + ASSERT_TRUE(buf->MarkSet()); + + ASSERT_TRUE(Execute(ed, std::string("visual-line-toggle"))); + ASSERT_TRUE(Execute(ed, std::string("down"), std::string(), 1)); + ASSERT_TRUE(buf->VisualLineActive()); + + // Yank should clear mark and any highlighting. + ed.KillRingClear(); + ed.KillRingPush("X"); + ASSERT_TRUE(Execute(ed, std::string("yank"))); + + ASSERT_TRUE(!buf->MarkSet()); + ASSERT_TRUE(!buf->VisualLineActive()); } \ No newline at end of file