[pitivi] timeline: Fix drag&drop of multiple assets

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

commit 3c5c84f5e248c51982f66d0056efd22c56ed29dd
Author: AsociTon <asociton outlook com>
Date:   Sun Apr 5 19:25:51 2020 +0530

    timeline: Fix drag&drop of multiple assets
    
    When a clip cannot be placed on the timeline, we want to rollback the
    previously added clips, if any. This is difficult though because GES
    emits signals as if the clip which cannot be placed has been placed AND
    removed. These invalid operations cannot be rolled back.
    
    To be able to rollback, we record each clip add operation in its own
    transaction. Then if it fails, we first rollback that transaction
    without doing anything, then we rollback the rest so the previously
    added clips are removed.
    
    Fixes #2414

 pitivi/timeline/timeline.py     | 24 +++++++++++++++------
 pitivi/undo/undo.py             | 14 ++++++++----
 tests/test_timeline_timeline.py | 48 ++++++++++++++++++++++++++++++++---------
 tests/test_undo.py              | 26 +++++++++++++++++++---
 4 files changed, 88 insertions(+), 24 deletions(-)
---
diff --git a/pitivi/timeline/timeline.py b/pitivi/timeline/timeline.py
index 44e7b72c..0cf2f7c7 100644
--- a/pitivi/timeline/timeline.py
+++ b/pitivi/timeline/timeline.py
@@ -949,21 +949,31 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
         assets = self._project.assets_for_uris(self.drop_data)
         if not assets:
             self._project.add_uris(self.drop_data)
-            return False
+            return
 
         ges_clips = []
+        self.app.action_log.begin("Add clips")
         for asset in assets:
-
             ges_layer, unused_on_sep = self.get_layer_at(y)
             if not placement:
                 placement = self.pixel_to_ns(x)
             placement = max(0, placement)
 
-            self.debug("Creating %s at %s", asset.props.id, Gst.TIME_ARGS(placement))
-
+            self.debug("Adding %s at %s on layer %s", asset.props.id, Gst.TIME_ARGS(placement), ges_layer)
+            self.app.action_log.begin("Add one clip")
             ges_clip = self.add_clip_to_layer(ges_layer, asset, placement)
             if not ges_clip:
-                return False
+                # The clip cannot be placed.
+
+                # Rollback the current "Add one asset" transaction without
+                # doing anything, since nothing really changed but GES still
+                # emitted signals as if it added AND removed the clip.
+                self.app.action_log.rollback(undo=False)
+                # Rollback the rest of the "Add assets" transaction.
+                self.app.action_log.rollback()
+                return
+
+            self.app.action_log.commit("Add one clip")
 
             placement += ges_clip.props.duration
             ges_clip.first_placement = True
@@ -971,6 +981,8 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
 
             ges_clips.append(ges_clip)
 
+        self.app.action_log.commit("Add clips")
+
         if ges_clips:
             ges_clip = ges_clips[0]
             self.dragging_element = ges_clip.ui
@@ -981,8 +993,6 @@ class Timeline(Gtk.EventBox, Zoomable, Loggable):
 
             self.dragging_group = self.selection.group()
 
-        return True
-
     def _drag_motion_cb(self, widget, context, x, y, timestamp):
         target = self.drag_dest_find_target(context, None)
         if not target:
diff --git a/pitivi/undo/undo.py b/pitivi/undo/undo.py
index 13e7cde5..3803fcbb 100644
--- a/pitivi/undo/undo.py
+++ b/pitivi/undo/undo.py
@@ -254,20 +254,26 @@ class UndoableActionLog(GObject.Object, Loggable):
                    action, stack.action_group_name)
         self.emit("push", stack, action)
 
-    def rollback(self):
-        """Forgets about the last started operation."""
+    def rollback(self, undo=True):
+        """Forgets about the last started operation.
+
+        Args:
+            undo (bool): Whether to undo the last started operation.
+                If False, it's disregarded without any action.
+        """
         if self.running:
             self.debug("Ignore rollback because running")
             return
 
+        self.debug("Rolling back, undo=%s", undo)
         self.rolling_back = True
         try:
-            self.debug("Rolling back")
             stack = self._get_last_stack(pop=True)
             self.debug("rollback action group %s, nested %s",
                        stack.action_group_name, len(self.stacks))
             self.emit("rollback", stack)
-            stack.undo()
+            if undo:
+                stack.undo()
         finally:
             self.rolling_back = False
 
diff --git a/tests/test_timeline_timeline.py b/tests/test_timeline_timeline.py
index e094302c..d77a4fa6 100644
--- a/tests/test_timeline_timeline.py
+++ b/tests/test_timeline_timeline.py
@@ -23,6 +23,8 @@ from gi.repository import GES
 from gi.repository import Gst
 from gi.repository import Gtk
 
+from pitivi.undo.timeline import TimelineObserver
+from pitivi.undo.undo import UndoableActionLog
 from pitivi.utils.timeline import UNSELECT
 from pitivi.utils.ui import LAYER_HEIGHT
 from pitivi.utils.ui import SEPARATOR_HEIGHT
@@ -826,17 +828,17 @@ class TestClipsEdges(BaseTestTimeline):
 
 class TestDragFromOutside(BaseTestTimeline):
 
-    def test_adding_overlap_clip(self):
-        """Checks asset drag&drop on top of an existing clip."""
+    def setUp(self):
+        super().setUp()
+
         timeline_container = common.create_timeline_container()
-        timeline_ui = timeline_container.timeline
+        self.ges_timeline = timeline_container.timeline.ges_timeline
 
-        asset = GES.UriClipAsset.request_sync(
-            common.get_sample_uri("tears_of_steel.webm"))
-        layer, = timeline_ui.ges_timeline.get_layers()
-        layer.add_asset(asset, 0, 0, 10, GES.TrackType.UNKNOWN)
+        timeline_container.app.action_log = UndoableActionLog()
+        self.timeline_observer = TimelineObserver(timeline_container.ges_timeline, 
timeline_container.app.action_log)
 
-        # Events emitted while dragging an asset over a clip in the timeline:
+    def check_drag_assets_to_timeline(self, timeline_ui, assets):
+        # Events emitted while dragging assets over a clip in the timeline:
         # motion, receive, motion.
         with mock.patch.object(Gdk, "drag_status") as _drag_status_mock:
             with mock.patch.object(Gtk, "drag_finish") as _drag_finish_mock:
@@ -850,11 +852,11 @@ class TestDragFromOutside(BaseTestTimeline):
                 self.assertFalse(timeline_ui.drop_data_ready)
                 selection_data = mock.Mock()
                 selection_data.get_data_type = mock.Mock(return_value=target)
-                selection_data.get_uris.return_value = [asset.props.id]
+                selection_data.get_uris.return_value = [asset.props.id for asset in assets]
                 self.assertIsNone(timeline_ui.drop_data)
                 self.assertFalse(timeline_ui.drop_data_ready)
                 timeline_ui._drag_data_received_cb(None, None, 0, 0, selection_data, None, 0)
-                self.assertEqual(timeline_ui.drop_data, [asset.props.id])
+                self.assertEqual(timeline_ui.drop_data, [asset.props.id for asset in assets])
                 self.assertTrue(timeline_ui.drop_data_ready)
 
                 timeline_ui.drag_get_data.reset_mock()
@@ -869,3 +871,29 @@ class TestDragFromOutside(BaseTestTimeline):
                 self.assertFalse(timeline_ui.drag_get_data.called)
                 self.assertIsNone(timeline_ui.dragging_element)
                 self.assertFalse(timeline_ui.dropping_clips)
+
+    def test_adding_overlap_clip(self):
+        """Checks asset drag&drop on top of an existing clip."""
+        asset = GES.UriClipAsset.request_sync(
+            common.get_sample_uri("tears_of_steel.webm"))
+
+        layer, = self.ges_timeline.get_layers()
+        layer.add_asset(asset, 0, 0, 10, GES.TrackType.UNKNOWN)
+        clips = layer.get_clips()
+
+        self.check_drag_assets_to_timeline(self.ges_timeline.ui, [asset])
+        self.assertEqual(layer.get_clips(), clips)
+
+    def test_dragging_multiple_clips_over_timeline(self):
+        """Checks drag&drop two assets when only the first one can be placed."""
+        asset = GES.UriClipAsset.request_sync(
+            common.get_sample_uri("tears_of_steel.webm"))
+
+        layer, = self.ges_timeline.get_layers()
+        start = asset.get_duration()
+        layer.add_asset(asset, start, 0, 10, GES.TrackType.UNKNOWN)
+        clips = layer.get_clips()
+
+        # Use same asset to mimic dragging multiple assets
+        self.check_drag_assets_to_timeline(self.ges_timeline.ui, [asset, asset])
+        self.assertEqual(layer.get_clips(), clips)
diff --git a/tests/test_undo.py b/tests/test_undo.py
index 95fcde20..29a7976d 100644
--- a/tests/test_undo.py
+++ b/tests/test_undo.py
@@ -89,6 +89,10 @@ class TestUndoableActionLog(common.TestCase):
     def tearDown(self):
         self._disconnect_from_undoable_action_log()
 
+    def check_signals(self, *expected_signals):
+        signals = [item[0] for item in self.signals]
+        self.assertListEqual(signals, list(expected_signals))
+
     def _undo_action_log_signal_cb(self, log, *args):
         args = list(args)
         signal_name = args.pop(-1)
@@ -219,19 +223,35 @@ class TestUndoableActionLog(common.TestCase):
         self.assertEqual(len(self.log.undo_stacks), 0)
         self.assertEqual(len(self.log.redo_stacks), 0)
         self.log.begin("meh")
-        self.assertEqual(len(self.signals), 1)
+        self.check_signals("begin")
         name, (_stack,) = self.signals[0]
         self.assertEqual(name, "begin")
         self.assertTrue(self.log.is_in_transaction())
 
+        action = mock.Mock(spec=UndoableAction)
+        self.log.push(action)
+
         self.log.rollback()
-        self.assertEqual(len(self.signals), 2)
-        name, (_stack,) = self.signals[1]
+
+        action.undo.assert_called_once_with()
+
+        self.check_signals("begin", "push", "rollback")
+        name, (_stack,) = self.signals[2]
         self.assertEqual(name, "rollback")
         self.assertFalse(self.log.is_in_transaction())
         self.assertEqual(len(self.log.undo_stacks), 0)
         self.assertEqual(len(self.log.redo_stacks), 0)
 
+    def test_rollback_noop(self):
+        """Checks a rollback which does not act."""
+        self.log.begin("meh")
+
+        action = mock.Mock(spec=UndoableAction)
+        self.log.push(action)
+
+        self.log.rollback(undo=False)
+        action.undo.assert_not_called()
+
     def test_nested_rollback(self):
         """Checks two nested rollbacks."""
         self.assertEqual(len(self.log.undo_stacks), 0)