[pitivi/wip-titles: 4/5] undo: Join successive similar operations on title clips

[Alexandru Băluț <alexbalut src gnome org>]

commit eec855a1bba51dbb974debac7b895410df919b17
Author: Alexandru Băluț <alexandru balut gmail com>
Date:   Sun Jan 24 20:44:25 2021 +0100

    undo: Join successive similar operations on title clips
    
    Fixes #2022

 pitivi/clip_properties/title.py |  9 ++---
 pitivi/undo/timeline.py         |  9 +++++
 pitivi/undo/undo.py             | 73 +++++++++++++++++++++++++++++++++--------
 tests/test_undo.py              | 37 +++++++++++++++++++--
 4 files changed, 108 insertions(+), 20 deletions(-)
---
diff --git a/pitivi/clip_properties/title.py b/pitivi/clip_properties/title.py
index 7a612b7a0..a1a61b62b 100644
--- a/pitivi/clip_properties/title.py
+++ b/pitivi/clip_properties/title.py
@@ -114,8 +114,9 @@ class TitleProperties(Gtk.Expander, Loggable):
 
         self.show_all()
 
-    def _set_child_property(self, name, value):
-        with self.app.action_log.started("Title change property",
+    def _set_child_property(self, name, value, mergeable=False):
+        with self.app.action_log.started("Title change property %s" % name,
+                                         mergeable=mergeable,
                                          toplevel=True):
             self._setting_props = True
             try:
@@ -173,7 +174,7 @@ class TitleProperties(Gtk.Expander, Loggable):
 
         escaped_text = html.escape(self.textbuffer.props.text)
         self.log("Source text updated to %s", escaped_text)
-        self._set_child_property("text", escaped_text)
+        self._set_child_property("text", escaped_text, mergeable=True)
 
     def _alignment_changed_cb(self, combo):
         """Handles changes in the h/v alignment widgets."""
@@ -201,7 +202,7 @@ class TitleProperties(Gtk.Expander, Loggable):
         else:
             prop_name = "y-absolute"
         value = spin.get_value()
-        self._set_child_property(prop_name, value)
+        self._set_child_property(prop_name, value, mergeable=True)
 
     def _update_absolute_alignment_widgets_visibility(self):
         visible = self.valignment_combo.get_active_id() == "absolute"
diff --git a/pitivi/undo/timeline.py b/pitivi/undo/timeline.py
index 1b478754d..7d03e1670 100644
--- a/pitivi/undo/timeline.py
+++ b/pitivi/undo/timeline.py
@@ -77,6 +77,15 @@ class TrackElementPropertyChanged(UndoableAction):
         st['value'] = GObject.Value(pspec.value_type, value)
         return st
 
+    def expand(self, action):
+        if not isinstance(action, TrackElementPropertyChanged) or \
+                not action.track_element == self.track_element or \
+                not action.property_name == self.property_name:
+            return False
+
+        self.new_value = action.new_value
+        return True
+
 
 class TimelineElementObserver(Loggable):
     """Monitors the props of an element and all its children.
diff --git a/pitivi/undo/undo.py b/pitivi/undo/undo.py
index 39de7b247..8e7ec0f19 100644
--- a/pitivi/undo/undo.py
+++ b/pitivi/undo/undo.py
@@ -121,26 +121,57 @@ class UndoableActionStack(UndoableAction, Loggable):
         action_group_name (str): The name of the operation.
         done_actions (List[UndoableAction]): The UndoableActions pushed in
             the stack.
+        mergeable (bool): Whether this stack accepts merges with other
+            compatible stacks.
         finalizing_action (FinalizingAction): The action to be performed
             at the end of undoing or redoing the stacked actions.
     """
 
-    def __init__(self, action_group_name, finalizing_action=None):
+    def __init__(self, action_group_name, mergeable, finalizing_action=None):
         UndoableAction.__init__(self)
         Loggable.__init__(self)
         self.action_group_name = action_group_name
         self.done_actions = []
+        self.mergeable = mergeable
         self.finalizing_action = finalizing_action
 
+    def __len__(self):
+        return len(self.done_actions)
+
     def __repr__(self):
         return "%s: %s" % (self.action_group_name, self.done_actions)
 
+    def attempt_merge(self, stack, action):
+        """Merges the action into the previous one if possible.
+
+        Returns:
+            bool: Whether the merge has been done.
+        """
+        if not self.mergeable:
+            return False
+
+        if not self.done_actions:
+            return False
+
+        if not self.action_group_name == stack.action_group_name:
+            return False
+
+        return self.attempt_expand_action(action)
+
+    def attempt_expand_action(self, action):
+        """Expands the last action with the specified action if possible."""
+        if not self.done_actions:
+            return False
+
+        last_action = self.done_actions[-1]
+        return last_action.expand(action)
+
     def push(self, action):
-        if self.done_actions:
-            last_action = self.done_actions[-1]
-            if last_action.expand(action):
-                # The action has been included in the previous one.
-                return
+        """Adds an action unless it's possible to expand the previous."""
+        if self.attempt_expand_action(action):
+            # The action has been merged into the last one.
+            return
+
         self.done_actions.append(action)
 
     def _run_action(self, actions, method_name):
@@ -202,7 +233,7 @@ class UndoableActionLog(GObject.Object, Loggable):
         else:
             self.commit(action_group_name)
 
-    def begin(self, action_group_name, finalizing_action=None, toplevel=False):
+    def begin(self, action_group_name, finalizing_action=None, mergeable=False, toplevel=False):
         """Starts recording a high-level operation which later can be undone.
 
         The recording can be stopped by calling the `commit` method or
@@ -218,14 +249,14 @@ class UndoableActionLog(GObject.Object, Loggable):
         if toplevel and self.is_in_transaction():
             raise UndoWrongStateError("Toplevel operation started as suboperation", self.stacks)
 
-        stack = UndoableActionStack(action_group_name, finalizing_action)
+        stack = UndoableActionStack(action_group_name, mergeable, finalizing_action)
         self.stacks.append(stack)
         self.debug("begin action group %s, nested %s",
                    stack.action_group_name, len(self.stacks))
         self.emit("begin", stack)
 
     def push(self, action):
-        """Reports a change.
+        """Records a change noticed by the monitoring system.
 
         Args:
             action (Action): The action representing the change.
@@ -249,11 +280,24 @@ class UndoableActionLog(GObject.Object, Loggable):
         try:
             stack = self._get_last_stack()
         except UndoWrongStateError as e:
-            self.warning("Failed pushing '%s' because: %s", action, e)
+            self.warning("Failed pushing '%s' because no transaction started: %s", action, e)
             return
-        stack.push(action)
-        self.debug("push action %s in action group %s",
-                   action, stack.action_group_name)
+
+        merged = False
+        if stack.mergeable and len(self.stacks[0]) == 0 and self.undo_stacks:
+            # The current undoable operation is empty, this is the first action.
+            # Check if it can be merged with the previous operation.
+            previous_operation = self.undo_stacks[-1]
+            if previous_operation.attempt_merge(stack, action):
+                self.debug("Merging undoable operations")
+                self.stacks = [self.undo_stacks.pop()]
+                merged = True
+
+        if not merged:
+            stack.push(action)
+            self.debug("push action %s in action group %s",
+                       action, stack.action_group_name)
+
         self.emit("push", stack, action)
 
     def rollback(self, undo=True):
@@ -299,9 +343,11 @@ class UndoableActionLog(GObject.Object, Loggable):
         stack = self._get_last_stack(pop=True)
         if action_group_name != stack.action_group_name:
             raise UndoWrongStateError("Unexpected commit", action_group_name, stack, self.stacks)
+
         if not stack.done_actions:
             self.debug("Ignore empty stack %s", stack.action_group_name)
             return
+
         if not self.stacks:
             self.undo_stacks.append(stack)
             stack.finish_operation()
@@ -461,6 +507,7 @@ class PropertyChangedAction(UndoableAutomaticObjectAction):
                 self.auto_object != action.auto_object or \
                 self.field_name != action.field_name:
             return False
+
         self.new_value = action.new_value
         return True
 
diff --git a/tests/test_undo.py b/tests/test_undo.py
index 7c87a684e..be81dac3f 100644
--- a/tests/test_undo.py
+++ b/tests/test_undo.py
@@ -46,7 +46,7 @@ class TestUndoableActionStack(common.TestCase):
             def undo(self):
                 state["actions"] -= 1
 
-        stack = UndoableActionStack("meh")
+        stack = UndoableActionStack("meh", mergeable=False)
         action1 = Action()
         action2 = Action()
         stack.push(action1)
@@ -60,7 +60,7 @@ class TestUndoableActionStack(common.TestCase):
 
     def test_undo_error(self):
         """Checks undo a stack containing a failing action."""
-        stack = UndoableActionStack("meh")
+        stack = UndoableActionStack("meh", mergeable=False)
         action1 = mock.Mock(spec=UndoableAction)
         action1.expand.return_value = False
         action2 = mock.Mock(spec=UndoableAction)
@@ -414,6 +414,37 @@ class TestUndoableActionLog(common.TestCase):
         self.assertEqual(len(self.log.undo_stacks), 0)
         self.assertEqual(len(self.log.redo_stacks), 0)
 
+    def test_merging(self):
+        with self.log.started("one", mergeable=False):
+            action = mock.Mock(spec=UndoableAction)
+            action.expand.side_effect = Exception("should not have been called")
+            self.log.push(action)
+        self.assertEqual(len(self.log.undo_stacks), 1)
+
+        with self.log.started("one", mergeable=True):
+            action = mock.Mock(spec=UndoableAction)
+            action.expand.return_value = False
+            self.log.push(action)
+        self.assertEqual(len(self.log.undo_stacks), 2)
+
+        with self.log.started("one", mergeable=True):
+            action = mock.Mock(spec=UndoableAction)
+            action.expand.return_value = True
+            self.log.push(action)
+        self.assertEqual(len(self.log.undo_stacks), 3)
+
+        with self.log.started("one", mergeable=True):
+            action = mock.Mock(spec=UndoableAction)
+            action.expand.return_value = True
+            self.log.push(action)
+        self.assertEqual(len(self.log.undo_stacks), 3)
+
+        with self.log.started("one", mergeable=False):
+            action = mock.Mock(spec=UndoableAction)
+            action.expand.side_effect = Exception("should not have been called")
+            self.log.push(action)
+        self.assertEqual(len(self.log.undo_stacks), 4)
+
 
 class TestRollback(common.TestCase):
 
@@ -473,7 +504,7 @@ class TestGObjectObserver(common.TestCase):
 class TestPropertyChangedAction(common.TestCase):
 
     def test_expand(self):
-        stack = UndoableActionStack("good one!")
+        stack = UndoableActionStack("good one!", mergeable=False)
         gobject = mock.Mock()
         stack.push(PropertyChangedAction(gobject, "field", 5, 7))
         stack.push(PropertyChangedAction(gobject, "field", 11, 13))