From 161568103e67830f60a686751b2e260dffc90420 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sat, 8 May 2021 21:08:41 +0200 Subject: [PATCH] LibGUI: Remove UndoStack's automatic command combo'ing UndoStack will now merge adjacent commands *if they want to be merged* instead of bundling everything you push onto it until you tell it to "finalize the combo." This uses less memory and gives applications full control over how their undo stacks end up. :^) --- .../Applications/FontEditor/FontEditor.cpp | 5 +- .../Applications/PixelPaint/ImageEditor.cpp | 1 - Userland/Libraries/LibGUI/TextDocument.cpp | 26 ++++--- Userland/Libraries/LibGUI/UndoStack.cpp | 70 +++++-------------- Userland/Libraries/LibGUI/UndoStack.h | 12 +--- 5 files changed, 37 insertions(+), 77 deletions(-) diff --git a/Userland/Applications/FontEditor/FontEditor.cpp b/Userland/Applications/FontEditor/FontEditor.cpp index 338f08796b..af2ccfdd47 100644 --- a/Userland/Applications/FontEditor/FontEditor.cpp +++ b/Userland/Applications/FontEditor/FontEditor.cpp @@ -340,10 +340,9 @@ FontEditorWidget::FontEditorWidget(const String& path, RefPtr&& did_modify_font(); }; - m_glyph_editor_widget->on_undo_event = [this](bool finalize) { + m_glyph_editor_widget->on_undo_event = [this](bool) { + // FIXME: UndoStack no longer has finalization concept, so this needs some fixing. m_undo_stack->push(make(*m_undo_glyph)); - if (finalize) - m_undo_stack->finalize_current_combo(); did_change_undo_stack(); }; diff --git a/Userland/Applications/PixelPaint/ImageEditor.cpp b/Userland/Applications/PixelPaint/ImageEditor.cpp index e904f4c7d1..07408ea3e6 100644 --- a/Userland/Applications/PixelPaint/ImageEditor.cpp +++ b/Userland/Applications/PixelPaint/ImageEditor.cpp @@ -48,7 +48,6 @@ void ImageEditor::did_complete_action() { if (!m_image) return; - m_undo_stack->finalize_current_combo(); m_undo_stack->push(make(*m_image)); } diff --git a/Userland/Libraries/LibGUI/TextDocument.cpp b/Userland/Libraries/LibGUI/TextDocument.cpp index bca690e3df..6687574513 100644 --- a/Userland/Libraries/LibGUI/TextDocument.cpp +++ b/Userland/Libraries/LibGUI/TextDocument.cpp @@ -726,6 +726,8 @@ bool InsertTextCommand::merge_with(GUI::Command const& other) auto& typed_other = static_cast(other); if (m_range.end() != typed_other.m_range.start()) return false; + if (m_range.start().line() != m_range.end().line()) + return false; StringBuilder builder(m_text.length() + typed_other.m_text.length()); builder.append(m_text); builder.append(typed_other.m_text); @@ -807,16 +809,18 @@ bool RemoveTextCommand::merge_with(GUI::Command const& other) if (!is(other)) return false; auto& typed_other = static_cast(other); - if (m_range.start() == typed_other.m_range.end()) { - // Merge backspaces - StringBuilder builder(m_text.length() + typed_other.m_text.length()); - builder.append(typed_other.m_text); - builder.append(m_text); - m_text = builder.to_string(); - m_range.set_start(typed_other.m_range.start()); - return true; - } - // FIXME: Merge forward-deletes + if (m_range.start() != typed_other.m_range.end()) + return false; + if (m_range.start().line() != m_range.end().line()) + return false; + // Merge backspaces + StringBuilder builder(m_text.length() + typed_other.m_text.length()); + builder.append(typed_other.m_text); + builder.append(m_text); + m_text = builder.to_string(); + m_range.set_start(typed_other.m_range.start()); + return true; + return false; } @@ -834,7 +838,7 @@ void RemoveTextCommand::undo() void TextDocument::update_undo() { - m_undo_stack.finalize_current_combo(); + // FIXME: Maybe seal the last command somehow? } TextPosition TextDocument::insert_at(const TextPosition& position, const StringView& text, const Client* client) diff --git a/Userland/Libraries/LibGUI/UndoStack.cpp b/Userland/Libraries/LibGUI/UndoStack.cpp index 2df8460070..759b97b5f4 100644 --- a/Userland/Libraries/LibGUI/UndoStack.cpp +++ b/Userland/Libraries/LibGUI/UndoStack.cpp @@ -19,14 +19,14 @@ UndoStack::~UndoStack() bool UndoStack::can_undo() const { - return m_stack_index > 0 || (m_stack.size() == 1 && m_stack[0].commands.size() > 0); + return m_stack_index > 0; } bool UndoStack::can_redo() const { if (m_stack.is_empty()) return false; - return m_stack_index != m_stack.size() - 1; + return m_stack_index != m_stack.size(); } void UndoStack::undo() @@ -34,10 +34,8 @@ void UndoStack::undo() if (!can_undo()) return; - finalize_current_combo(); - auto& combo = m_stack[--m_stack_index]; - for (auto i = static_cast(combo.commands.size()) - 1; i >= 0; i--) - combo.commands[i].undo(); + auto& command = m_stack[--m_stack_index]; + command.undo(); if (on_state_change) on_state_change(); @@ -48,69 +46,39 @@ void UndoStack::redo() if (!can_redo()) return; - auto& commands = m_stack[m_stack_index++].commands; - for (auto& command : commands) - command.redo(); + auto& command = m_stack[m_stack_index++]; + command.redo(); if (on_state_change) on_state_change(); } -void UndoStack::pop() +void UndoStack::push(NonnullOwnPtr command) { - VERIFY(!m_stack.is_empty()); - m_stack.take_last(); + // If the stack cursor is behind the top of the stack, nuke everything from here to the top. + while (m_stack.size() != m_stack_index) + m_stack.take_last(); + if (m_clean_index.has_value() && m_clean_index.value() > m_stack.size()) m_clean_index = {}; -} -void UndoStack::push(NonnullOwnPtr&& command) -{ - if (m_stack.is_empty()) - finalize_current_combo(); - - // If the stack cursor is behind the top of the stack, nuke everything from here to the top. - if (m_stack_index != m_stack.size() - 1) { - while (m_stack.size() != m_stack_index) { - pop(); - } - finalize_current_combo(); - } - - if (!m_stack.last().commands.is_empty()) { - bool merged = m_stack.last().commands.last().merge_with(*command); - if (merged) + if (!m_stack.is_empty()) { + if (m_stack.last().merge_with(*command)) return; } - m_stack.last().commands.append(move(command)); + m_stack.append(move(command)); + m_stack_index = m_stack.size(); if (on_state_change) on_state_change(); } -void UndoStack::finalize_current_combo() -{ - if (m_stack.is_empty()) { - m_stack.append(make()); - return; - } - - if (!m_stack.last().commands.is_empty()) { - m_stack.append(make()); - m_stack_index = m_stack.size() - 1; - - if (on_state_change) - on_state_change(); - } -} - void UndoStack::set_current_unmodified() { - finalize_current_combo(); - if (m_clean_index.has_value() && m_clean_index.value() == m_stack_index) return; + m_clean_index = m_stack_index; if (on_state_change) @@ -119,14 +87,12 @@ void UndoStack::set_current_unmodified() bool UndoStack::is_current_modified() const { + if (m_stack.is_empty()) + return false; if (!m_clean_index.has_value()) return true; if (m_stack_index != m_clean_index.value()) return true; - if (m_stack.is_empty()) - return false; - if (m_stack_index == m_stack.size() - 1 && !m_stack[m_stack_index].commands.is_empty()) - return true; return false; } diff --git a/Userland/Libraries/LibGUI/UndoStack.h b/Userland/Libraries/LibGUI/UndoStack.h index 3f720ebd01..2957ca0b97 100644 --- a/Userland/Libraries/LibGUI/UndoStack.h +++ b/Userland/Libraries/LibGUI/UndoStack.h @@ -17,7 +17,7 @@ public: UndoStack(); ~UndoStack(); - void push(NonnullOwnPtr&&); + void push(NonnullOwnPtr); bool can_undo() const; bool can_redo() const; @@ -25,8 +25,6 @@ public: void undo(); void redo(); - void finalize_current_combo(); - void set_current_unmodified(); bool is_current_modified() const; @@ -35,13 +33,7 @@ public: Function on_state_change; private: - struct Combo { - NonnullOwnPtrVector commands; - }; - - void pop(); - - NonnullOwnPtrVector m_stack; + NonnullOwnPtrVector m_stack; size_t m_stack_index { 0 }; Optional m_clean_index; };