FontEditor: Make undo/redo compatible with multi-glyph selections

Previously the glyph undo stack saved an array of bytes representing
the restore state of an individual glyph when modified. Now the
selection undo stack saves a byte buffer of the entire selection,
letting us restore changes to multiple glyphs at once.

Userland/Applications/FontEditor/CMakeLists.txt

@@ -17,7 +17,7 @@ set(SOURCES
     NewFontDialog.cpp
     NewFontDialogPage1GML.h
     NewFontDialogPage2GML.h
-    UndoGlyph.h
+    UndoSelection.h
 )
 
 serenity_app(FontEditor ICON app-font-editor)

Userland/Applications/FontEditor/FontEditor.cpp

@@ -348,12 +348,15 @@ FontEditorWidget::FontEditorWidget()
     };
 
     m_glyph_editor_widget->on_undo_event = [this] {
-        m_undo_stack->push(make<GlyphUndoCommand>(*m_undo_glyph));
+        m_undo_stack->push(make<SelectionUndoCommand>(*m_undo_selection));
     };
 
     m_glyph_map_widget->on_active_glyph_changed = [this](int glyph) {
-        if (m_undo_glyph)
+        if (m_undo_selection) {
-            m_undo_glyph->set_code_point(glyph);
+            auto selection = m_glyph_map_widget->selection().normalized();
+            m_undo_selection->set_start(selection.start());
+            m_undo_selection->set_size(selection.size());
+        }
         m_glyph_editor_widget->set_glyph(glyph);
         auto glyph_width = m_edited_font->raw_glyph_width(glyph);
         if (m_edited_font->is_fixed_width())
@@ -386,7 +389,7 @@ FontEditorWidget::FontEditorWidget()
     };
 
     m_glyph_editor_width_spinbox->on_change = [this](int value) {
-        m_undo_stack->push(make<GlyphUndoCommand>(*m_undo_glyph));
+        m_undo_stack->push(make<SelectionUndoCommand>(*m_undo_selection));
         m_edited_font->set_glyph_width(m_glyph_map_widget->active_glyph(), value);
         m_glyph_editor_widget->update();
         m_glyph_map_widget->update_glyph(m_glyph_map_widget->active_glyph());
@@ -396,7 +399,7 @@ FontEditorWidget::FontEditorWidget()
     };
 
     m_glyph_editor_present_checkbox->on_checked = [this](bool checked) {
-        m_undo_stack->push(make<GlyphUndoCommand>(*m_undo_glyph));
+        m_undo_stack->push(make<SelectionUndoCommand>(*m_undo_selection));
         m_edited_font->set_glyph_width(m_glyph_map_widget->active_glyph(), checked ? m_edited_font->glyph_fixed_width() : 0);
         m_glyph_editor_widget->update();
         m_glyph_map_widget->update_glyph(m_glyph_map_widget->active_glyph());
@@ -559,7 +562,7 @@ void FontEditorWidget::initialize(String const& path, RefPtr<Gfx::BitmapFont>&&
     });
 
     m_undo_stack = make<GUI::UndoStack>();
-    m_undo_glyph = adopt_ref(*new UndoGlyph(m_glyph_map_widget->active_glyph(), *m_edited_font));
+    m_undo_selection = adopt_ref(*new UndoSelection(m_glyph_map_widget->selection().start(), m_glyph_map_widget->selection().size(), *m_edited_font));
 
     m_undo_stack->on_state_change = [this] {
         m_undo_action->set_enabled(m_undo_stack->can_undo());
@@ -672,8 +675,8 @@ void FontEditorWidget::undo()
     if (!m_undo_stack->can_undo())
         return;
     m_undo_stack->undo();
-    auto glyph = m_undo_glyph->restored_code_point();
+    auto glyph = m_undo_selection->previous_active_glyph();
-    auto glyph_width = m_undo_glyph->restored_width();
+    auto glyph_width = edited_font().raw_glyph_width(glyph);
     m_glyph_map_widget->set_active_glyph(glyph);
     m_glyph_map_widget->scroll_to_glyph(glyph);
     if (m_edited_font->is_fixed_width()) {
@@ -681,9 +684,8 @@ void FontEditorWidget::undo()
     } else {
         m_glyph_editor_width_spinbox->set_value(glyph_width, GUI::AllowCallback::No);
     }
-    m_edited_font->set_glyph_width(m_glyph_map_widget->active_glyph(), glyph_width);
     m_glyph_editor_widget->update();
-    m_glyph_map_widget->update_glyph(glyph);
+    m_glyph_map_widget->update();
     update_preview();
     update_statusbar();
 }
@@ -693,8 +695,8 @@ void FontEditorWidget::redo()
     if (!m_undo_stack->can_redo())
         return;
     m_undo_stack->redo();
-    auto glyph = m_undo_glyph->restored_code_point();
+    auto glyph = m_undo_selection->previous_active_glyph();
-    auto glyph_width = m_undo_glyph->restored_width();
+    auto glyph_width = edited_font().raw_glyph_width(glyph);
     m_glyph_map_widget->set_active_glyph(glyph);
     m_glyph_map_widget->scroll_to_glyph(glyph);
     if (m_edited_font->is_fixed_width()) {
@@ -702,9 +704,8 @@ void FontEditorWidget::redo()
     } else {
         m_glyph_editor_width_spinbox->set_value(glyph_width, GUI::AllowCallback::No);
     }
-    m_edited_font->set_glyph_width(m_glyph_map_widget->active_glyph(), glyph_width);
     m_glyph_editor_widget->update();
-    m_glyph_map_widget->update_glyph(glyph);
+    m_glyph_map_widget->update();
     update_preview();
     update_statusbar();
 }
@@ -857,15 +858,11 @@ void FontEditorWidget::paste_glyphs()
     if (!height)
         return;
 
-    // FIXME: This is a hack to avoid regression and still doesn't support
-    //        multiple glyphs. It should have done proper undo stack integration.
-    if (glyph_count == 1) {
-        if (m_glyph_editor_widget->on_undo_event)
-            m_glyph_editor_widget->on_undo_event();
-    }
-
     auto selection = m_glyph_map_widget->selection().normalized();
     auto range_bound_glyph_count = min(glyph_count, 1 + m_range.last - selection.start());
+    m_undo_selection->set_size(range_bound_glyph_count);
+    if (m_glyph_editor_widget->on_undo_event)
+        m_glyph_editor_widget->on_undo_event();
 
     size_t bytes_per_glyph = Gfx::GlyphBitmap::bytes_per_row() * edited_font().glyph_height();
     size_t bytes_per_copied_glyph = Gfx::GlyphBitmap::bytes_per_row() * height;
@@ -893,15 +890,10 @@ void FontEditorWidget::paste_glyphs()
 
 void FontEditorWidget::delete_selected_glyphs()
 {
+    if (m_glyph_editor_widget->on_undo_event)
+        m_glyph_editor_widget->on_undo_event();
+
     auto selection = m_glyph_map_widget->selection().normalized();
-
-    // FIXME: This is a hack to avoid regression and still doesn't support
-    //        multiple glyphs. It should have done proper undo stack integration.
-    if (selection.size() == 1) {
-        if (m_glyph_editor_widget->on_undo_event)
-            m_glyph_editor_widget->on_undo_event();
-    }
-
     size_t bytes_per_glyph = Gfx::GlyphBitmap::bytes_per_row() * m_edited_font->glyph_height();
     auto* rows = m_edited_font->rows() + selection.start() * bytes_per_glyph;
     auto* widths = m_edited_font->widths() + selection.start();

Userland/Applications/FontEditor/FontEditor.h

@@ -7,7 +7,7 @@
 
 #pragma once
 
-#include "UndoGlyph.h"
+#include "UndoSelection.h"
 #include <LibGUI/ActionGroup.h>
 #include <LibGUI/FilteringProxyModel.h>
 #include <LibGUI/GlyphMapWidget.h>
@@ -77,7 +77,7 @@ private:
 
     RefPtr<GUI::Action> m_undo_action;
     RefPtr<GUI::Action> m_redo_action;
-    RefPtr<UndoGlyph> m_undo_glyph;
+    RefPtr<UndoSelection> m_undo_selection;
     OwnPtr<GUI::UndoStack> m_undo_stack;
 
     RefPtr<GUI::Action> m_go_to_glyph_action;

Userland/Applications/FontEditor/UndoGlyph.h

@@ -1,76 +0,0 @@
-/*
- * Copyright (c) 2021, the SerenityOS developers.
- *
- * SPDX-License-Identifier: BSD-2-Clause
- */
-
-#pragma once
-
-#include <LibGUI/Command.h>
-#include <LibGUI/UndoStack.h>
-#include <LibGfx/BitmapFont.h>
-
-class UndoGlyph : public RefCounted<UndoGlyph> {
-public:
-    explicit UndoGlyph(u32 const code_point, Gfx::BitmapFont const& font)
-        : m_code_point(code_point)
-        , m_font(font)
-    {
-    }
-    NonnullRefPtr<UndoGlyph> save_state() const
-    {
-        auto state = adopt_ref(*new UndoGlyph(m_code_point, *m_font));
-        auto glyph = font().glyph(m_code_point).glyph_bitmap();
-        for (int x = 0; x < font().max_glyph_width(); x++)
-            for (int y = 0; y < font().glyph_height(); y++)
-                state->m_bits[x][y] = glyph.bit_at(x, y);
-        state->m_width = glyph.width();
-        return state;
-    }
-    void restore_state(UndoGlyph const& state) const
-    {
-        auto bitmap = font().glyph(state.m_code_point).glyph_bitmap();
-        for (int x = 0; x < font().max_glyph_width(); x++)
-            for (int y = 0; y < font().glyph_height(); y++)
-                bitmap.set_bit_at(x, y, state.m_bits[x][y]);
-        m_restored_width = state.m_width;
-        m_restored_code_point = state.m_code_point;
-    }
-    void set_code_point(u32 code_point) { m_code_point = code_point; }
-    void set_font(Gfx::BitmapFont& font) { m_font = font; }
-    Gfx::BitmapFont const& font() const { return *m_font; }
-    u8 restored_width() const { return m_restored_width; }
-    u32 restored_code_point() const { return m_restored_code_point; }
-
-private:
-    u32 m_code_point { 0 };
-    RefPtr<Gfx::BitmapFont> m_font;
-    u8 m_bits[Gfx::GlyphBitmap::max_width()][Gfx::GlyphBitmap::max_height()] {};
-    u8 m_width { 0 };
-    mutable u8 m_restored_width { 0 };
-    mutable u32 m_restored_code_point { 0 };
-};
-
-class GlyphUndoCommand : public GUI::Command {
-public:
-    GlyphUndoCommand(UndoGlyph& glyph)
-        : m_undo_state(glyph.save_state())
-        , m_undo_glyph(glyph)
-    {
-    }
-    virtual void undo() override
-    {
-        if (!m_redo_state)
-            m_redo_state = m_undo_state->save_state();
-        m_undo_glyph.restore_state(*m_undo_state);
-    }
-    virtual void redo() override
-    {
-        m_undo_glyph.restore_state(*m_redo_state);
-    }
-
-private:
-    NonnullRefPtr<UndoGlyph> m_undo_state;
-    RefPtr<UndoGlyph> m_redo_state;
-    UndoGlyph& m_undo_glyph;
-};

Userland/Applications/FontEditor/UndoSelection.h

@@ -0,0 +1,75 @@
+/*
+ * Copyright (c) 2021-2022, the SerenityOS developers.
+ *
+ * SPDX-License-Identifier: BSD-2-Clause
+ */
+
+#pragma once
+
+#include <LibGUI/Command.h>
+#include <LibGUI/UndoStack.h>
+#include <LibGfx/BitmapFont.h>
+
+class UndoSelection : public RefCounted<UndoSelection> {
+public:
+    explicit UndoSelection(int const start, int const size, Gfx::BitmapFont const& font)
+        : m_start(start)
+        , m_size(size)
+        , m_font(font)
+    {
+    }
+    NonnullRefPtr<UndoSelection> save_state()
+    {
+        auto state = adopt_ref(*new UndoSelection(m_start, m_size, *m_font));
+        size_t bytes_per_glyph = Gfx::GlyphBitmap::bytes_per_row() * font().glyph_height();
+        auto* rows = font().rows() + m_start * bytes_per_glyph;
+        auto* widths = font().widths() + m_start;
+        state->m_data.append(&rows[0], bytes_per_glyph * m_size);
+        state->m_data.append(&widths[0], m_size);
+        return state;
+    }
+    void restore_state(UndoSelection const& state)
+    {
+        size_t bytes_per_glyph = Gfx::GlyphBitmap::bytes_per_row() * font().glyph_height();
+        auto* rows = font().rows() + state.m_start * bytes_per_glyph;
+        auto* widths = font().widths() + state.m_start;
+        memcpy(rows, &state.m_data[0], bytes_per_glyph * state.m_size);
+        memcpy(widths, &state.m_data[bytes_per_glyph * state.m_size], state.m_size);
+        m_previous_active_glyph = state.m_start;
+    }
+    void set_start(int start) { m_start = start; }
+    void set_size(int size) { m_size = size; }
+    Gfx::BitmapFont& font() { return *m_font; }
+    u32 previous_active_glyph() const { return m_previous_active_glyph; }
+
+private:
+    int m_start { 0 };
+    int m_size { 0 };
+    RefPtr<Gfx::BitmapFont> m_font;
+    ByteBuffer m_data;
+    u32 m_previous_active_glyph { 0 };
+};
+
+class SelectionUndoCommand : public GUI::Command {
+public:
+    SelectionUndoCommand(UndoSelection& selection)
+        : m_undo_state(selection.save_state())
+        , m_undo_selection(selection)
+    {
+    }
+    virtual void undo() override
+    {
+        if (!m_redo_state)
+            m_redo_state = m_undo_state->save_state();
+        m_undo_selection.restore_state(*m_undo_state);
+    }
+    virtual void redo() override
+    {
+        m_undo_selection.restore_state(*m_redo_state);
+    }
+
+private:
+    NonnullRefPtr<UndoSelection> m_undo_state;
+    RefPtr<UndoSelection> m_redo_state;
+    UndoSelection& m_undo_selection;
+};