[pitivi] undo: Fix video transition identification

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

commit 228a24e63028d13ad50465a324dc830ff8f298cd
Author: Alexandru Băluț <alexandru balut gmail com>
Date:   Thu Jun 23 21:23:55 2022 +0200

    undo: Fix video transition identification

 pitivi/timeline/timeline.py |  6 ++---
 pitivi/undo/base.py         |  4 ++++
 pitivi/undo/timeline.py     | 16 +++++++++----
 pitivi/undo/undo.py         | 38 ++++++++++++++++++++----------
 tests/common.py             |  2 +-
 tests/test_undo_timeline.py | 56 +++++++++++++++++++++++++++++++++++++++++++--
 6 files changed, 100 insertions(+), 22 deletions(-)
---
diff --git a/pitivi/timeline/timeline.py b/pitivi/timeline/timeline.py
index 9cb506566..95786378c 100644
--- a/pitivi/timeline/timeline.py
+++ b/pitivi/timeline/timeline.py
@@ -406,7 +406,7 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
         # Which handle of the dragging_element has been clicked, if any.
         # If set, it means we are in a trim operation.
         self.__clicked_handle = None
-        # The GES object for controlling the operation.
+        # The object keeping track of the current operation being performed.
         self.editing_context = None
         # Whether dragging_element really got dragged.
         self.__got_dragged = False
@@ -1281,7 +1281,7 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
 
         self.hadj.set_value(0)
 
-    def __get_editing_mode(self):
+    def __get_editing_mode(self) -> GES.EditMode:
         if not self.editing_context:
             is_handle = False
         else:
@@ -1384,7 +1384,7 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
                                                   self.app,
                                                   not self.dropping_clips)
 
-        mode = self.__get_editing_mode()
+        mode: GES.EditMode = self.__get_editing_mode()
         self.editing_context.set_mode(mode)
 
         if self.editing_context.edge is GES.Edge.EDGE_END:
diff --git a/pitivi/undo/base.py b/pitivi/undo/base.py
index fd8a7ac98..7023cbb86 100644
--- a/pitivi/undo/base.py
+++ b/pitivi/undo/base.py
@@ -28,6 +28,10 @@ class UndoWrongStateError(UndoError):
     """Exception related to the current state of the undo/redo stack."""
 
 
+class ConditionsNotReadyYetError(UndoError):
+    """The operation cannot be performed at the moment, maybe later."""
+
+
 class Action(GObject.Object, Loggable):
     """Something which might worth logging in a scenario."""
 
diff --git a/pitivi/undo/timeline.py b/pitivi/undo/timeline.py
index 4d7a3040f..f12f419b1 100644
--- a/pitivi/undo/timeline.py
+++ b/pitivi/undo/timeline.py
@@ -21,6 +21,7 @@ from gi.repository import GObject
 from gi.repository import Gst
 
 from pitivi.effects import PROPS_TO_IGNORE
+from pitivi.undo.base import ConditionsNotReadyYetError
 from pitivi.undo.base import FinalizingAction
 from pitivi.undo.base import GObjectObserver
 from pitivi.undo.base import UndoableAction
@@ -383,15 +384,20 @@ class ClipRemoved(ClipAction):
 
 
 class TransitionClipAction(UndoableAction):
-    # pylint: disable=abstract-method
 
-    def __init__(self, ges_layer, ges_clip, track_element):
+    def __init__(self, ges_layer: GES.Layer, ges_clip: GES.TransitionClip, track_element: GES.Transition):
         UndoableAction.__init__(self)
         self.ges_layer = ges_layer
         self.start = ges_clip.props.start
         self.duration = ges_clip.props.duration
         self.track_element = track_element
 
+    def do(self):
+        raise NotImplementedError()
+
+    def undo(self):
+        raise NotImplementedError()
+
     @staticmethod
     def get_video_element(ges_clip: GES.TransitionClip) -> Optional[GES.VideoTransition]:
         for track_element in ges_clip.get_children(recursive=True):
@@ -426,7 +432,9 @@ class TransitionClipAddedAction(TransitionClipAction):
     def do(self):
         """Searches the transition clip created automatically to update it."""
         track_element = self.find_video_transition()
-        assert track_element
+        if not track_element:
+            raise ConditionsNotReadyYetError("transition missing when re-doing ADD")
+
         UndoableAutomaticObjectAction.update_object(self.track_element, track_element)
 
     def undo(self):
@@ -460,7 +468,7 @@ class TransitionClipRemovedAction(TransitionClipAction):
         # Search the transition clip created automatically to update it.
         track_element = self.find_video_transition()
         if not track_element:
-            return
+            raise ConditionsNotReadyYetError("transition missing when un-doing REMOVE")
 
         UndoableAutomaticObjectAction.update_object(self.track_element, track_element)
         for prop_name, value in self.properties:
diff --git a/pitivi/undo/undo.py b/pitivi/undo/undo.py
index e1683dcf2..c2497456d 100644
--- a/pitivi/undo/undo.py
+++ b/pitivi/undo/undo.py
@@ -19,18 +19,13 @@ import contextlib
 
 from gi.repository import GObject
 
+from pitivi.undo.base import ConditionsNotReadyYetError
 from pitivi.undo.base import UndoableAction
+from pitivi.undo.base import UndoError
+from pitivi.undo.base import UndoWrongStateError
 from pitivi.utils.loggable import Loggable
 
 
-class UndoError(Exception):
-    """Base class for undo/redo exceptions."""
-
-
-class UndoWrongStateError(UndoError):
-    """Exception related to the current state of the undo/redo stack."""
-
-
 class UndoableActionStack(UndoableAction, Loggable):
     """A stack of UndoableAction objects.
 
@@ -91,18 +86,37 @@ class UndoableActionStack(UndoableAction, Loggable):
 
         self.done_actions.append(action)
 
-    def _run_action(self, actions, method_name):
+    def _perform_actions(self, actions, method_name):
+        delayed_actions = []
+        delayed_reasons = {}
         for action in actions:
             self.log("Performing %s.%s()", action, method_name)
             method = getattr(action, method_name)
-            method()
+            try:
+                method()
+                for delayed_action in list(delayed_actions):
+                    self.log("Performing delayed %s.%s()", action, method_name)
+                    delayed_method = getattr(delayed_action, method_name)
+                    try:
+                        delayed_method()
+                        self.log("Succeeded performing action")
+                        delayed_actions.remove(delayed_action)
+                        delayed_reasons.pop(delayed_action)
+                    except ConditionsNotReadyYetError:
+                        self.log("Further delaying action")
+            except ConditionsNotReadyYetError as e:
+                self.log("Delaying action")
+                delayed_actions.append(action)
+                delayed_reasons[action] = e
+        if delayed_actions:
+            raise UndoError("Delayable actions failed to apply: {} {}".format(delayed_actions, 
delayed_reasons))
         self.finish_operation()
 
     def do(self):
-        self._run_action(self.done_actions, "do")
+        self._perform_actions(self.done_actions, "do")
 
     def undo(self):
-        self._run_action(self.done_actions[::-1], "undo")
+        self._perform_actions(self.done_actions[::-1], "undo")
 
     def finish_operation(self):
         if not self.finalizing_action:
diff --git a/tests/common.py b/tests/common.py
index 3f895999a..3a7dbe297 100644
--- a/tests/common.py
+++ b/tests/common.py
@@ -652,7 +652,7 @@ def cloned_sample(*samples):
             module["get_sample_uri"] = original_get_sample_uri
 
 
-def get_clip_children(ges_clip, *track_types, recursive=False):
+def get_clip_children(ges_clip: GES.Clip, *track_types: List[GES.TrackType], recursive: bool = False):
     for ges_timeline_element in ges_clip.get_children(recursive):
         if not track_types or ges_timeline_element.get_track_type() in track_types:
             yield ges_timeline_element
diff --git a/tests/test_undo_timeline.py b/tests/test_undo_timeline.py
index 3f20003cb..aaa390255 100644
--- a/tests/test_undo_timeline.py
+++ b/tests/test_undo_timeline.py
@@ -30,7 +30,9 @@ from pitivi.undo.base import PropertyChangedAction
 from pitivi.undo.project import AssetAddedAction
 from pitivi.undo.timeline import ClipAdded
 from pitivi.undo.timeline import ClipRemoved
+from pitivi.undo.timeline import CommitTimelineFinalizingAction
 from pitivi.undo.timeline import TrackElementAdded
+from pitivi.utils.timeline import EditingContext
 from pitivi.utils.ui import LAYER_HEIGHT
 from pitivi.utils.ui import URI_TARGET_ENTRY
 from tests import common
@@ -550,10 +552,12 @@ class TestLayerObserver(common.TestCase):
         self.layer.add_clip(clip2)
         self.assertEqual(len(self.layer.get_clips()), 3)
 
-        with self.action_log.started("move clip"):
+        with self.action_log.started("move clip",
+                                     
finalizing_action=CommitTimelineFinalizingAction(self.project.pipeline)):
             clip2.set_start(20 * Gst.SECOND)
         self.assertEqual(clip2.get_start(), 20 * Gst.SECOND)
-        self.assertEqual(len(self.layer.get_clips()), 2)
+        self.assertEqual(len(self.layer.get_clips()), 2,
+                         "The two title clips don't overlap so there should be no transition clip")
 
         self.action_log.undo()
         self.assertEqual(clip2.get_start(), 5 * Gst.SECOND)
@@ -563,6 +567,54 @@ class TestLayerObserver(common.TestCase):
         self.assertEqual(clip2.get_start(), 20 * Gst.SECOND)
         self.assertEqual(len(self.layer.get_clips()), 2)
 
+    @common.setup_project(assets_names=["mp3_sample.mp3"])
+    def test_move_transition_to_different_layer_audio(self):
+        uri = common.get_sample_uri("mp3_sample.mp3")
+        asset = GES.UriClipAsset.request_sync(uri)
+        self.__check_move_transition_to_different_layer(asset)
+
+    @common.setup_project(assets_names=["30fps_numeroted_frames_red.mkv"])
+    def test_move_transition_to_different_layer_video(self):
+        uri = common.get_sample_uri("30fps_numeroted_frames_red.mkv")
+        asset = GES.UriClipAsset.request_sync(uri)
+        self.__check_move_transition_to_different_layer(asset)
+
+    def __check_move_transition_to_different_layer(self, asset):
+        clip1 = asset.extract()
+        clip1.set_start(0 * Gst.SECOND)
+        self.layer.add_clip(clip1)
+        self.assertEqual(len(self.layer.get_clips()), 1)
+
+        clip2 = asset.extract()
+        clip2.set_start(clip1.props.duration * 9 // 10)
+        self.layer.add_clip(clip2)
+        clips = self.layer.get_clips()
+        self.assertEqual(len(clips), 3)
+
+        # Click all three clips including the transition clip to make sure
+        # it's not included in the group.
+        for clip in clips:
+            self.click_clip(clip, expect_selected=True, ctrl_key=True)
+        self.timeline_container.group_action.activate()
+
+        layer2 = self.timeline.append_layer()
+        with self.action_log.started("move clips to different layer"):
+            editing_context = EditingContext(clip1, self.timeline, GES.EditMode.EDIT_NORMAL, 
GES.Edge.EDGE_NONE, self.app)
+            editing_context.edit_to(0, layer2)
+            editing_context.finish()
+        self.assertEqual(len(self.layer.get_clips()), 0)
+        self.assertEqual(len(layer2.get_clips()), 3)
+
+        with self.project.pipeline.commit_timeline_after():
+            self.action_log.undo()
+        self.assertEqual(len(self.layer.get_clips()), 3)
+        self.assertEqual(len(layer2.get_clips()), 0)
+
+        with self.project.pipeline.commit_timeline_after():
+            self.action_log.redo()
+        self.assertEqual(len(self.layer.get_clips()), 0)
+        self.assertEqual(len(layer2.get_clips()), 3)
+
     @common.setup_timeline
     def test_transition_type(self):
         """Checks the transitions keep their type."""