[pitivi] undo: Fix clip ungroup redo adding extra track element

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

commit 11edd0917baafeb85cfd44d08092bbf4cd52c43a
Author: Alexandru Băluț <alexandru balut gmail com>
Date:   Wed Jan 4 06:37:25 2017 +0100

    undo: Fix clip ungroup redo adding extra track element
    
    When a clip is ungrouped, an empty clip is added (by GES) then a video
    track element is added to it. When redoing these steps in Pitivi, GES
    adds track elements itself because the clip has none, so we end up with
    too many.
    
    Added a new ClipAction undoable class to reuse the add-clip-as-is logic
    between ClipAdded and ClipRemoved.
    
    Reviewed-by: Thibault Saunier <tsaunier gnome org>
    Differential Revision: https://phabricator.freedesktop.org/D1583

 pitivi/undo/timeline.py     |   56 +++++++++++++++++++++++++++++-------------
 pitivi/undo/undo.py         |   35 +++++++++++---------------
 tests/test_undo.py          |    6 ++++
 tests/test_undo_timeline.py |   22 +++++++++++++++++
 4 files changed, 81 insertions(+), 38 deletions(-)
---
diff --git a/pitivi/undo/timeline.py b/pitivi/undo/timeline.py
index ee1aa47..52d1742 100644
--- a/pitivi/undo/timeline.py
+++ b/pitivi/undo/timeline.py
@@ -22,7 +22,6 @@ from gi.repository import Gst
 
 from pitivi.effects import PROPS_TO_IGNORE
 from pitivi.undo.undo import Action
-from pitivi.undo.undo import ExpandableUndoableAction
 from pitivi.undo.undo import FinalizingAction
 from pitivi.undo.undo import GObjectObserver
 from pitivi.undo.undo import MetaContainerObserver
@@ -267,25 +266,42 @@ class ControlSourceObserver(GObject.Object):
         self.action_log.push(action)
 
 
-class ClipAdded(UndoableAction):
+class ClipAction(UndoableAction):
 
     def __init__(self, layer, clip):
         UndoableAction.__init__(self)
         self.layer = layer
         self.clip = clip
 
-    def __repr__(self):
-        return "<ClipAdded %s>" % self.clip
-
-    def do(self):
+    def add(self):
         self.clip.set_name(None)
+        children = self.clip.get_children(False)
         self.layer.add_clip(self.clip)
+        # GES adds children if the clip had none. Make sure they are removed.
+        for child in self.clip.get_children(False):
+            if child not in children:
+                self.clip.remove(child)
         self.layer.get_timeline().get_asset().pipeline.commit_timeline()
 
-    def undo(self):
+    def _child_added_cb(self, clip, track_element):
+        clip.remove(track_element)
+
+    def remove(self):
         self.layer.remove_clip(self.clip)
         self.layer.get_timeline().get_asset().pipeline.commit_timeline()
 
+
+class ClipAdded(ClipAction):
+
+    def __repr__(self):
+        return "<ClipAdded %s>" % self.clip
+
+    def do(self):
+        self.add()
+
+    def undo(self):
+        self.remove()
+
     def asScenarioAction(self):
         timeline = self.layer.get_timeline()
         if hasattr(self.layer, "splitting_object") and \
@@ -305,12 +321,10 @@ class ClipAdded(UndoableAction):
         return st
 
 
-class ClipRemoved(ExpandableUndoableAction):
+class ClipRemoved(ClipAction):
 
     def __init__(self, layer, clip):
-        ExpandableUndoableAction.__init__(self)
-        self.layer = layer
-        self.clip = clip
+        ClipAction.__init__(self, layer, clip)
         self.transition_removed_actions = []
 
     def __repr__(self):
@@ -323,13 +337,10 @@ class ClipRemoved(ExpandableUndoableAction):
         return True
 
     def do(self):
-        self.layer.remove_clip(self.clip)
-        self.layer.get_timeline().get_asset().pipeline.commit_timeline()
+        self.remove()
 
     def undo(self):
-        self.clip.set_name(None)
-        self.layer.add_clip(self.clip)
-        self.layer.get_timeline().get_asset().pipeline.commit_timeline()
+        self.add()
         # Update the automatically created transitions.
         for action in self.transition_removed_actions:
             action.undo()
@@ -727,6 +738,9 @@ class TimelineElementAddedToGroup(UndoableAction):
         self.ges_group = ges_group
         self.ges_timeline_element = ges_timeline_element
 
+    def __repr__(self):
+        return "<TimelineElementAddedToGroup %s, %s>" % (self.ges_group, self.ges_timeline_element)
+
     def do(self):
         self.ges_group.add(self.ges_timeline_element)
 
@@ -741,6 +755,9 @@ class TimelineElementRemovedFromGroup(UndoableAction):
         self.ges_group = ges_group
         self.ges_timeline_element = ges_timeline_element
 
+    def __repr__(self):
+        return "<TimelineElementRemovedFromGroup %s, %s>" % (self.ges_group, self.ges_timeline_element)
+
     def do(self):
         self.ges_group.remove(self.ges_timeline_element)
 
@@ -819,7 +836,7 @@ class TimelineObserver(Loggable):
 
     def _connect_to_group(self, ges_group):
         if not ges_group.props.serialize:
-            return
+            return False
 
         # A group is added when it gets its first element, thus
         # when undoing/redoing a group can be added multiple times.
@@ -828,9 +845,12 @@ class TimelineObserver(Loggable):
         if ges_group not in self.group_observers:
             group_observer = GroupObserver(ges_group, self.action_log)
             self.group_observers[ges_group] = group_observer
+        return True
 
     def __group_added_cb(self, unused_ges_timeline, ges_group):
-        self._connect_to_group(ges_group)
+        if not self._connect_to_group(ges_group):
+            return
+
         # This should be a single clip.
         for ges_clip in ges_group.get_children(recursive=False):
             action = TimelineElementAddedToGroup(ges_group, ges_clip)
diff --git a/pitivi/undo/undo.py b/pitivi/undo/undo.py
index eaaa692..845cbbd 100644
--- a/pitivi/undo/undo.py
+++ b/pitivi/undo/undo.py
@@ -58,6 +58,18 @@ class UndoableAction(Action):
     def undo(self):
         raise NotImplementedError()
 
+    def expand(self, action):
+        """Allows the action to expand by including the specified action.
+
+        Args:
+            action (UndoableAction): The action to include.
+
+        Returns:
+            bool: Whether the action has been included, in which case
+                it should not be used for anything else.
+        """
+        return False
+
 
 class UndoableAutomaticObjectAction(UndoableAction):
     """An action on an automatically created object.
@@ -94,22 +106,6 @@ class UndoableAutomaticObjectAction(UndoableAction):
             cls.__updates[other] = new_auto_object
 
 
-class ExpandableUndoableAction(UndoableAction):
-    """An action which can include immediately following actions."""
-
-    def expand(self, action):
-        """Expands including the specified action.
-
-        Args:
-            action (UndoableAction): The action to include.
-
-        Returns:
-            bool: Whether the action has been included, in which case
-                it should not be used for anything else.
-        """
-        raise NotImplementedError()
-
-
 class FinalizingAction:
     """Base class for actions applied when an undo or redo is performed."""
 
@@ -140,10 +136,9 @@ class UndoableActionStack(UndoableAction):
     def push(self, action):
         if self.done_actions:
             last_action = self.done_actions[-1]
-            if isinstance(last_action, ExpandableUndoableAction):
-                if last_action.expand(action):
-                    # The action has been included in the previous one.
-                    return
+            if last_action.expand(action):
+                # The action has been included in the previous one.
+                return
         self.done_actions.append(action)
 
     def _run_action(self, actions, method_name):
diff --git a/tests/test_undo.py b/tests/test_undo.py
index c2d82d9..03b9b29 100644
--- a/tests/test_undo.py
+++ b/tests/test_undo.py
@@ -63,7 +63,9 @@ class TestUndoableActionStack(TestCase):
         """
         stack = UndoableActionStack("meh")
         action1 = mock.Mock(spec=UndoableAction)
+        action1.expand.return_value = False
         action2 = mock.Mock(spec=UndoableAction)
+        action2.expand.return_value = False
         action2.undo.side_effect = UndoError("meh")
         action3 = mock.Mock(spec=UndoableAction)
         stack.push(action1)
@@ -284,6 +286,7 @@ class TestUndoableActionLog(TestCase):
 
         # push two actions
         action1 = mock.Mock(spec=UndoableAction)
+        action1.expand.return_value = False
         self.log.push(action1)
         self.assertEqual(len(self.signals), 2)
         name, (stack, signalAction) = self.signals[1]
@@ -342,8 +345,11 @@ class TestUndoableActionLog(TestCase):
         """
         order = mock.Mock()
         order.action1 = mock.Mock(spec=UndoableAction)
+        order.action1.expand.return_value = False
         order.action2 = mock.Mock(spec=UndoableAction)
+        order.action2.expand.return_value = False
         order.action3 = mock.Mock(spec=UndoableAction)
+        order.action3.expand.return_value = False
 
         with self.log.started("meh"):
             self.log.push(order.action1)
diff --git a/tests/test_undo_timeline.py b/tests/test_undo_timeline.py
index 96a7c96..2a4d833 100644
--- a/tests/test_undo_timeline.py
+++ b/tests/test_undo_timeline.py
@@ -187,7 +187,23 @@ class TestTimelineObserver(BaseTestUndoTimeline):
         self.assertEqual(len(clips[0].get_children(False)), 1)
         self.assertEqual(len(clips[1].get_children(False)), 1)
 
+        timeline.selection.select(clips)
+        timeline.resetSelectionGroup()
+        for clip in clips:
+            timeline.current_group.add(clip)
+        self.timeline_container.group_action.activate(None)
+        clips = list(self.getTimelineClips())
+        self.assertEqual(len(clips), 1, clips)
+        self.assertEqual(len(clips[0].get_children(False)), 2)
+
         for i in range(2):
+            # Undo grouping.
+            self.action_log.undo()
+            clips = list(self.getTimelineClips())
+            self.assertEqual(len(clips), 2, clips)
+            self.assertEqual(len(clips[0].get_children(False)), 1)
+            self.assertEqual(len(clips[1].get_children(False)), 1)
+
             # Undo ungrouping.
             self.action_log.undo()
             clips = list(self.getTimelineClips())
@@ -201,6 +217,12 @@ class TestTimelineObserver(BaseTestUndoTimeline):
             self.assertEqual(len(clips[0].get_children(False)), 1)
             self.assertEqual(len(clips[1].get_children(False)), 1)
 
+            # Redo grouping.
+            self.action_log.redo()
+            clips = list(self.getTimelineClips())
+            self.assertEqual(len(clips), 1, clips)
+            self.assertEqual(len(clips[0].get_children(False)), 2)
+
 
 class TestLayerObserver(BaseTestUndoTimeline):