[pitivi] timeline: Added precise clip positioning shortcut.

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

commit 64e8b4ee2bc7f10dba6ef450ff0a3e0d00e833d4
Author: will_swiston <wswiston gmail com>
Date:   Fri Apr 30 09:00:46 2021 -0500

    timeline: Added precise clip positioning shortcut.
    
    Currently, precise positioning of clips is difficult, as it must be done by hand.
    
    With these keyboard shortcuts, the user now has the ability to shift a clip by a single frame.
    
    Fixes #2221

 pitivi/timeline/timeline.py     | 43 +++++++++++++++++++++++
 tests/common.py                 | 28 +++++++++++----
 tests/test_timeline_timeline.py | 75 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+), 6 deletions(-)
---
diff --git a/pitivi/timeline/timeline.py b/pitivi/timeline/timeline.py
index 18f007436..acae28f89 100644
--- a/pitivi/timeline/timeline.py
+++ b/pitivi/timeline/timeline.py
@@ -16,6 +16,7 @@
 # License along with this program; if not, see <http://www.gnu.org/licenses/>.
 import os
 from gettext import gettext as _
+from typing import Optional
 
 from gi.repository import Gdk
 from gi.repository import GES
@@ -1777,6 +1778,20 @@ class TimelineContainer(Gtk.Grid, Zoomable, Loggable):
                                self.seek_backward_clip_action,
                                _("Seek to the first clip edge before the playhead"))
 
+        self.shift_forward_action = Gio.SimpleAction.new("shift-forward", None)
+        self.shift_forward_action.connect("activate", self._shift_forward_cb)
+        group.add_action(self.shift_forward_action)
+        self.app.shortcuts.add("timeline.shift-forward", ["<Primary><Shift>Right"],
+                               self.shift_forward_action,
+                               _("Shift selected clips one frame forward"))
+
+        self.shift_backward_action = Gio.SimpleAction.new("shift-backward", None)
+        self.shift_backward_action.connect("activate", self._shift_backward_cb)
+        group.add_action(self.shift_backward_action)
+        self.app.shortcuts.add("timeline.shift-backward", ["<Primary><Shift>Left"],
+                               self.shift_backward_action,
+                               _("Shift selected clips one frame backward"))
+
         self.add_effect_action = Gio.SimpleAction.new("add-effect", None)
         self.add_effect_action.connect("activate", self.__add_effect_cb)
         group.add_action(self.add_effect_action)
@@ -2189,6 +2204,34 @@ class TimelineContainer(Gtk.Grid, Zoomable, Loggable):
         self._project.pipeline.step_frame(1)
         self.timeline.scroll_to_playhead(align=Gtk.Align.CENTER, when_not_in_view=True)
 
+    def _shift_forward_cb(self, action: Gio.SimpleAction, parameter: Optional[GLib.Variant]) -> None:
+        self._shift_clips(1)
+
+    def _shift_backward_cb(self, action: Gio.SimpleAction, parameter: Optional[GLib.Variant]) -> None:
+        self._shift_clips(-1)
+
+    def _shift_clips(self, delta_frames):
+        """Shifts the selected clips position with the specified number of frames."""
+        if not self.ges_timeline:
+            return
+
+        previous_snapping_distance = self.ges_timeline.get_snapping_distance()
+        self.ges_timeline.set_snapping_distance(0)
+        try:
+            clips = list(self.timeline.selection.selected)
+            clips.sort(key=lambda candidate_clip: candidate_clip.start, reverse=delta_frames > 0)
+            # We must use delta * frame_time because getting negative frame time is not possible.
+            clip_delta = delta_frames * self.ges_timeline.get_frame_time(1)
+            self.app.action_log.begin("Shift clip delta frames")
+
+            for clip in clips:
+                if not clip.set_start(clip.start + clip_delta):
+                    self.app.action_log.rollback()
+                    return
+        finally:
+            self.ges_timeline.set_snapping_distance(previous_snapping_distance)
+        self.app.action_log.commit("Shift clip delta frames")
+
     def do_focus_in_event(self, unused_event):
         self.log("Timeline has grabbed focus")
         self.update_actions()
diff --git a/tests/common.py b/tests/common.py
index 44f461765..cd99ea432 100644
--- a/tests/common.py
+++ b/tests/common.py
@@ -189,9 +189,8 @@ class CheckedOperationDuration:
         signal.alarm(0)
 
 
-def setup_project_with_clips(assets_names: List[str]):
-    """Sets up a Pitivi instance with the specified assets on the timeline."""
-    # Ensure this method is not being used directly as a decorator.
+def setup_project(assets_names: List[str]):
+    """Sets up a Pitivi instance with no assets on the timeline."""
     assert isinstance(assets_names, list)
 
     def decorator(func):
@@ -241,9 +240,6 @@ def setup_project_with_clips(assets_names: List[str]):
                 project.disconnect_by_func(project_loaded_cb)
                 project.disconnect_by_func(progress_cb)
 
-                assets = project.list_assets(GES.UriClip)
-                self.timeline_container.insert_assets(assets, self.timeline.props.duration)
-
                 func(self)
 
             del self.app
@@ -258,6 +254,26 @@ def setup_project_with_clips(assets_names: List[str]):
     return decorator
 
 
+def setup_project_with_clips(assets_names: List[str]):
+    """Sets up a Pitivi instance with the specified assets on the timeline."""
+    # Ensure this method is not being used directly as a decorator.
+    assert isinstance(assets_names, list)
+
+    def decorator(func):
+        nonlocal assets_names
+
+        @setup_project(assets_names)
+        def wrapper(self):
+            assets = self.app.project_manager.current_project.list_assets(GES.UriClip)
+            self.timeline_container.insert_assets(assets, self.timeline.props.duration)
+
+            func(self)
+
+        return wrapper
+
+    return decorator
+
+
 def setup_timeline(func):
     def wrapped(self):
         self.app = create_pitivi()
diff --git a/tests/test_timeline_timeline.py b/tests/test_timeline_timeline.py
index 9c08a90a4..ac9d2c27e 100644
--- a/tests/test_timeline_timeline.py
+++ b/tests/test_timeline_timeline.py
@@ -869,3 +869,78 @@ class TestDragFromOutside(common.TestCase):
         # 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)
+
+
+class TestKeyboardShiftClips(common.TestCase):
+
+    def check_frame_shift_clips(self, *ges_clips):
+        """Checks that clips shifted forwards then backwards work properly."""
+        clip_original_starts = [clip.start for clip in ges_clips]
+        delta = self.timeline.get_frame_time(1)
+
+        event = mock.Mock()
+        event.keyval = Gdk.KEY_Control_L
+        self.timeline_container.do_key_press_event(event)
+        for clip in ges_clips:
+            self.toggle_clip_selection(clip, expect_selected=True)
+        self.timeline_container.do_key_release_event(event)
+
+        self.timeline_container.shift_forward_action.emit("activate", None)
+        self.assertListEqual([clip.start - delta for clip in ges_clips], clip_original_starts)
+
+        self.timeline_container.shift_backward_action.emit("activate", None)
+        self.assertListEqual([clip.start for clip in ges_clips], clip_original_starts)
+
+    @common.setup_project(["tears_of_steel.webm"])
+    def test_clip_shift(self):
+        """Checks that shift methods change position of a single clip by one frame."""
+        ges_clip1 = self.add_clip(self.layer, 5 * Gst.SECOND, duration=2 * Gst.SECOND)
+        self.check_frame_shift_clips(ges_clip1)
+
+    @common.setup_project(["tears_of_steel.webm"])
+    def test_shift_disjoint_clips(self):
+        """Checks that disjoint clips are able to be shifted."""
+        ges_clip1 = self.add_clip(self.layer, 5 * Gst.SECOND, duration=1 * Gst.SECOND)
+        ges_clip2 = self.add_clip(self.layer, 9 * Gst.SECOND, duration=1 * Gst.SECOND)
+        self.check_frame_shift_clips(ges_clip1, ges_clip2)
+
+    @common.setup_project(["tears_of_steel.webm"])
+    def test_shift_adjacent_clips(self):
+        """Checks that adjacent clips are able to be shifted as well."""
+        ges_clip1 = self.add_clip(self.layer, 5 * Gst.SECOND, duration=2 * Gst.SECOND)
+        ges_clip2 = self.add_clip(self.layer, 7 * Gst.SECOND, duration=2 * Gst.SECOND)
+        self.check_frame_shift_clips(ges_clip1, ges_clip2)
+
+    @common.setup_project(["tears_of_steel.webm"])
+    def test_triple_overlap_causes_rollback(self):
+        """Checks that rollback works properly in the event of triple overlap."""
+        ges_clip1 = self.add_clip(self.layer, 5 * Gst.SECOND, duration=2 * Gst.SECOND)
+        ges_clip2 = self.add_clip(self.layer, 10 * Gst.SECOND, duration=2 * Gst.SECOND)
+        self.add_clip(self.layer, start=11 * Gst.SECOND, duration=2 * Gst.SECOND)
+        self.add_clip(self.layer, start=12 * Gst.SECOND, duration=2 * Gst.SECOND)
+        ges_clip3 = self.add_clip(self.layer, 13 * Gst.SECOND, duration=2 * Gst.SECOND)
+        ges_clip4 = self.add_clip(self.layer, 15 * Gst.SECOND, duration=2 * Gst.SECOND)
+
+        event = mock.Mock()
+        event.keyval = Gdk.KEY_Control_L
+        self.timeline_container.do_key_press_event(event)
+        self.toggle_clip_selection(ges_clip1, expect_selected=True)
+        self.toggle_clip_selection(ges_clip2, expect_selected=True)
+        self.toggle_clip_selection(ges_clip3, expect_selected=True)
+        self.toggle_clip_selection(ges_clip4, expect_selected=True)
+
+        self.timeline_container.do_key_release_event(event)
+
+        self.timeline_container.shift_forward_action.emit("activate", None)
+
+        self.assertEqual(5 * Gst.SECOND, ges_clip1.start)
+        self.assertEqual(10 * Gst.SECOND, ges_clip2.start)
+        self.assertEqual(13 * Gst.SECOND, ges_clip3.start)
+        self.assertEqual(15 * Gst.SECOND, ges_clip4.start)
+
+        self.timeline_container.shift_backward_action.emit("activate", None)
+
+        self.assertEqual(5 * Gst.SECOND, ges_clip1.start)
+        self.assertEqual(10 * Gst.SECOND, ges_clip2.start)
+        self.assertEqual(13 * Gst.SECOND, ges_clip3.start)
+        self.assertEqual(15 * Gst.SECOND, ges_clip4.start)