[pitivi] undo: Reuse the effect added/removed logic

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

commit bac5cd9ebf26dcce4374186dff6f864d24ac7a1f
Author: Alexandru Băluț <alexandru balut gmail com>
Date:   Mon May 30 09:14:54 2016 +0200

    undo: Reuse the effect added/removed logic
    
    Differential Revision: https://phabricator.freedesktop.org/D1031

 pitivi/undo/timeline.py     |   77 ++++++++++++++++++-------------------------
 tests/test_undo_timeline.py |    8 ++--
 2 files changed, 36 insertions(+), 49 deletions(-)
---
diff --git a/pitivi/undo/timeline.py b/pitivi/undo/timeline.py
index 3b55540..2677ca0 100644
--- a/pitivi/undo/timeline.py
+++ b/pitivi/undo/timeline.py
@@ -114,14 +114,7 @@ class TimelineElementObserver(Loggable):
         self.action_log.push(action)
 
 
-class TrackElementAdded(UndoableAction):
-    # Note: We have a bug if we just remove the Effect from the timeline
-    # and keep it saved here and then readd it to corresponding timeline (it
-    # freezes everything). So what we are doing is  to free the Effect,
-    # keep its settings here when undoing, and instanciate a new one when
-    # doing again. We have to keep all EffectPropertyChanged object that refers
-    # to the Effect when undoing so we reset theirs track_element when
-    # doing it again. The way of doing it is the same with EffectRemoved
+class TrackElementAction(UndoableAction):
 
     def __init__(self, clip, track_element):
         UndoableAction.__init__(self)
@@ -129,60 +122,54 @@ class TrackElementAdded(UndoableAction):
         self.track_element = track_element
         self.asset = track_element.get_asset()
         self.track_element_props = []
+        for prop in self.track_element.list_children_properties():
+            if not prop.flags & GObject.PARAM_WRITABLE or \
+                    prop.name in PROPS_TO_IGNORE:
+                continue
+            prop_name = child_property_name(prop)
+            res, value = self.track_element.get_child_property(prop_name)
+            assert res
+            self.track_element_props.append((prop_name, value))
 
-    def do(self):
+    def add(self):
         self.track_element = self.clip.add_asset(self.asset)
         for prop_name, prop_value in self.track_element_props:
             self.track_element.set_child_property(prop_name, prop_value)
 
-    def undo(self):
-        props = self.track_element.list_children_properties()
-        self.track_element_props = [(child_property_name(prop), 
self.track_element.get_child_property(child_property_name(prop))[1])
-                                    for prop in props
-                                    if prop.flags & GObject.PARAM_WRITABLE and prop.name not in 
PROPS_TO_IGNORE]
+    def remove(self):
         self.clip.remove(self.track_element)
-        del self.track_element
         self.track_element = None
 
+
+class EffectAddedAction(TrackElementAction):
+
+    def do(self):
+        self.add()
+
+    def undo(self):
+        self.remove()
+
     def asScenarioAction(self):
         st = Gst.Structure.new_empty("container-add-child")
         st["container-name"] = self.clip.get_name()
         st["asset-id"] = self.track_element.get_id()
         st["child-type"] = GObject.type_name(
             self.track_element.get_asset().get_extractable_type())
-
         return st
 
 
-class TrackElementRemoved(UndoableAction):
-
-    def __init__(self, clip, track_element):
-        UndoableAction.__init__(self)
-        self.track_element = track_element
-        self.clip = clip
-        self.asset = track_element.get_asset()
-        self.track_element_props = []
+class EffectRemovedAction(TrackElementAction):
 
     def do(self):
-        props = self.track_element.list_children_properties()
-        self.track_element_props = [(child_property_name(prop), 
self.track_element.get_child_property(child_property_name(prop))[1])
-                                    for prop in props
-                                    if prop.flags & GObject.PARAM_WRITABLE and prop.name not in 
PROPS_TO_IGNORE]
-
-        self.clip.remove(self.track_element)
-        del self.track_element
-        self.track_element = None
+        self.remove()
 
     def undo(self):
-        self.track_element = self.clip.add_asset(self.asset)
-        for prop_name, prop_value in self.track_element_props:
-            self.track_element.set_child_property(prop_name, prop_value)
+        self.add()
 
     def asScenarioAction(self):
         st = Gst.Structure.new_empty("container-remove-child")
         st["container-name"] = self.clip.get_name()
         st["child-name"] = self.track_element.get_name()
-
         return st
 
 
@@ -569,17 +556,17 @@ class TimelineObserver(Loggable):
         action = ClipRemoved(layer, clip)
         self.action_log.push(action)
 
-    def _clipTrackElementAddedCb(self, clip, track_element):
-        self._connectToTrackElement(track_element)
-        if isinstance(track_element, GES.BaseEffect):
-            action = TrackElementAdded(clip, track_element)
+    def _clipTrackElementAddedCb(self, clip, ges_track_element):
+        self._connectToTrackElement(ges_track_element)
+        if isinstance(ges_track_element, GES.BaseEffect):
+            action = EffectAddedAction(clip, ges_track_element)
             self.action_log.push(action)
 
-    def _clipTrackElementRemovedCb(self, clip, track_element):
-        self.debug("%s REMOVED from (%s)" % (track_element, clip))
-        self._disconnectFromTrackElement(track_element)
-        if isinstance(track_element, GES.BaseEffect):
-            action = TrackElementRemoved(clip, track_element)
+    def _clipTrackElementRemovedCb(self, clip, ges_track_element):
+        self.debug("%s REMOVED from %s", ges_track_element, clip)
+        self._disconnectFromTrackElement(ges_track_element)
+        if isinstance(ges_track_element, GES.BaseEffect):
+            action = EffectRemovedAction(clip, ges_track_element)
             self.action_log.push(action)
 
     def _trackElementActiveChangedCb(self, track_element, active, add_effect_action):
diff --git a/tests/test_undo_timeline.py b/tests/test_undo_timeline.py
index 5b269e2..62377b3 100644
--- a/tests/test_undo_timeline.py
+++ b/tests/test_undo_timeline.py
@@ -29,8 +29,8 @@ from pitivi.timeline.timeline import Timeline
 from pitivi.undo.project import AssetAddedAction
 from pitivi.undo.timeline import ClipAdded
 from pitivi.undo.timeline import ClipRemoved
+from pitivi.undo.timeline import EffectAddedAction
 from pitivi.undo.timeline import TimelineObserver
-from pitivi.undo.timeline import TrackElementAdded
 from pitivi.undo.undo import PropertyChangedAction
 from pitivi.undo.undo import UndoableActionLog
 from tests import common
@@ -321,7 +321,7 @@ class TestTimelineUndo(TestCase):
         stack = stacks[0]
         self.assertEqual(1, len(stack.done_actions), stack.done_actions)
         action = stack.done_actions[0]
-        self.assertTrue(isinstance(action, TrackElementAdded))
+        self.assertTrue(isinstance(action, EffectAddedAction))
 
         self.assertTrue(effect1 in clip1.get_children(True))
         self.assertEqual(1, len([effect for effect in
@@ -351,7 +351,7 @@ class TestTimelineUndo(TestCase):
         stack = stacks[0]
         self.assertEqual(1, len(stack.done_actions), stack.done_actions)
         action = stack.done_actions[0]
-        self.assertTrue(isinstance(action, TrackElementAdded))
+        self.assertTrue(isinstance(action, EffectAddedAction))
 
         self.assertTrue(effect1 in clip1.get_children(True))
         self.assertEqual(1, len([effect for effect in
@@ -390,7 +390,7 @@ class TestTimelineUndo(TestCase):
         stack = stacks[0]
         self.assertEqual(1, len(stack.done_actions), stack.done_actions)
         action = stack.done_actions[0]
-        self.assertTrue(isinstance(action, TrackElementAdded))
+        self.assertTrue(isinstance(action, EffectAddedAction))
 
         self.assertTrue(effect1 in clip1.get_children(True))
         self.assertEqual(1, len([effect for effect in