timeline: reimplement snap_to_position a bit more appropriately.

It could yet be made be simpler, but it would require
touching the rest of the timeline editing code.

Fixes https://phabricator.freedesktop.org/T7587

Reviewed-by: Thibault Saunier <thibault.saunier@collabora.com>
Differential Revision: https://phabricator.freedesktop.org/D657

4 files changed, 59 insertions, 81 deletions

diff --git a/ges/ges-timeline.c b/ges/ges-timeline.c
index aa6d9f3674..cd32a37fad 100644
--- a/ges/ges-timeline.c
+++ b/ges/ges-timeline.c
@@ -1271,85 +1271,46 @@ ges_timeline_emit_snappig (GESTimeline * timeline, GESTrackElement * obj1,
   }
 }
 
-static guint64 *
+static GstClockTime *
 ges_timeline_snap_position (GESTimeline * timeline,
-    GESTrackElement * trackelement, guint64 * current, guint64 timecode,
-    gboolean emit)
+    GESTrackElement * trackelement, GstClockTime * current,
+    GstClockTime timecode, gboolean emit)
 {
   GESTimelinePrivate *priv = timeline->priv;
-  GSequenceIter *iter, *prev_iter, *nxt_iter;
-  GESTrackElement *tmp_trackelement;
-  GESContainer *tmp_container, *container;
-
-  GstClockTime *last_snap_ts = priv->movecontext.last_snap_ts;
-  guint64 snap_distance = timeline->priv->snapping_distance;
-  guint64 *prev_tc, *next_tc, *ret = NULL, off = G_MAXUINT64, off1 =
-      G_MAXUINT64;
-
-  /* Avoid useless calculations */
-  if (snap_distance == 0)
-    return NULL;
-
-  /* If we can just resnap as last snap... do it */
-  if (last_snap_ts) {
-    off = timecode > *last_snap_ts ?
-        timecode - *last_snap_ts : *last_snap_ts - timecode;
-    if (off <= snap_distance) {
-      ret = last_snap_ts;
-      goto done;
-    }
-  }
-
-  container = get_toplevel_container (trackelement);
-
-  iter = g_sequence_search (priv->starts_ends, &timecode,
+  GSequenceIter *iter, *end_iter;
+  GESContainer *container = get_toplevel_container (trackelement);
+  GstClockTime *ret = NULL;
+  GstClockTime smallest_offset = G_MAXUINT64;
+  GstClockTime tmp_pos;
+
+  tmp_pos = timecode - priv->snapping_distance;
+  /* Rippling, not snapping with previous elements */
+  if (priv->movecontext.moving_trackelements)
+    tmp_pos = timecode;
+  iter = g_sequence_search (priv->starts_ends, &tmp_pos,
       (GCompareDataFunc) compare_uint64, NULL);
 
-  /* Getting the next/previous  values, and use the closest one if any "respects"
-   * the snap_distance value */
-  nxt_iter = iter;
-  while (!g_sequence_iter_is_end (nxt_iter)) {
-    next_tc = g_sequence_get (iter);
-    tmp_trackelement = g_hash_table_lookup (timeline->priv->by_object, next_tc);
-    tmp_container = get_toplevel_container (tmp_trackelement);
-
-    off = timecode > *next_tc ? timecode - *next_tc : *next_tc - timecode;
-    if (next_tc != current && off <= snap_distance
-        && container != tmp_container) {
-
-      ret = next_tc;
-      break;
-    }
-
-    nxt_iter = g_sequence_iter_next (nxt_iter);
-  }
-
-  if (ret == NULL)
-    off = G_MAXUINT64;
-
-  if (priv->movecontext.moving_trackelements) {
-    GST_INFO_OBJECT (timeline, "Rippling, no way we snap end");
-    goto done;
-  }
+  tmp_pos = timecode + priv->snapping_distance;
+  end_iter = g_sequence_search (priv->starts_ends, &tmp_pos,
+      (GCompareDataFunc) compare_uint64, NULL);
 
-  prev_iter = g_sequence_iter_prev (iter);
-  while (!g_sequence_iter_is_begin (prev_iter)) {
-    prev_tc = g_sequence_get (prev_iter);
-    tmp_trackelement = g_hash_table_lookup (timeline->priv->by_object, prev_tc);
-    tmp_container = get_toplevel_container (tmp_trackelement);
+  for (; iter != end_iter && !g_sequence_iter_is_end (iter);
+      iter = g_sequence_iter_next (iter)) {
+    GstClockTime *iter_tc = g_sequence_get (iter);
+    GESTrackElement *tmp_trackelement =
+        g_hash_table_lookup (priv->by_object, iter_tc);
+    GESContainer *tmp_container = get_toplevel_container (tmp_trackelement);
 
-    off1 = timecode > *prev_tc ? timecode - *prev_tc : *prev_tc - timecode;
-    if (prev_tc != current && off1 < off && off1 <= snap_distance &&
-        container != tmp_container) {
-      ret = prev_tc;
+    if (tmp_container == container)
+      continue;
 
+    if (ABS (timecode - *iter_tc) > smallest_offset)
       break;
-    }
 
-    prev_iter = g_sequence_iter_prev (prev_iter);
+    smallest_offset = ABS (timecode - *iter_tc);
+    ret = iter_tc;
   }
 
-done:
   /* We emit the snapping signal only if we snapped with a different value
    * than the current one */
   if (emit) {
@@ -1413,7 +1374,6 @@ ges_move_context_set_objects (GESTimeline * timeline, GESTrackElement * obj,
   GSequenceIter *iter, *trackelement_iter;
 
   MoveContext *mv_ctx = &timeline->priv->movecontext;
-
   iters = g_hash_table_lookup (timeline->priv->obj_iters, obj);
   trackelement_iter = iters->iter_obj;
   switch (edge) {
diff --git a/tests/check/ges/timelineedition.c b/tests/check/ges/timelineedition.c
index 68d0e21192..c827afca9d 100644
--- a/tests/check/ges/timelineedition.c
+++ b/tests/check/ges/timelineedition.c
@@ -393,9 +393,8 @@ GST_START_TEST (test_snapping)
    * time               26-------- 62 --------122
    */
   ges_timeline_element_set_start (GES_TIMELINE_ELEMENT (clip1), 26);
-  ges_timeline_element_set_duration (GES_TIMELINE_ELEMENT (clip1), 37);
   CHECK_OBJECT_PROPS (trackelement, 25, 0, 37);
-  CHECK_OBJECT_PROPS (trackelement1, 26, 5, 36);
+  CHECK_OBJECT_PROPS (trackelement1, 26, 5, 37);
   CHECK_OBJECT_PROPS (trackelement2, 62, 0, 60);
 
    /**
@@ -409,7 +408,7 @@ GST_START_TEST (test_snapping)
   fail_unless (ges_timeline_element_ripple (GES_TIMELINE_ELEMENT (clip1),
           58) == TRUE);
   CHECK_OBJECT_PROPS (trackelement, 25, 0, 37);
-  CHECK_OBJECT_PROPS (trackelement1, 62, 5, 36);
+  CHECK_OBJECT_PROPS (trackelement1, 62, 5, 37);
   CHECK_OBJECT_PROPS (trackelement2, 98, 0, 60);
 
   /**
@@ -419,7 +418,7 @@ GST_START_TEST (test_snapping)
    */
   ges_timeline_element_set_start (GES_TIMELINE_ELEMENT (clip2), 110);
   CHECK_OBJECT_PROPS (trackelement, 25, 0, 37);
-  CHECK_OBJECT_PROPS (trackelement1, 62, 5, 36);
+  CHECK_OBJECT_PROPS (trackelement1, 62, 5, 37);
   CHECK_OBJECT_PROPS (trackelement2, 110, 0, 60);
 
   /**
@@ -430,7 +429,7 @@ GST_START_TEST (test_snapping)
   fail_unless (ges_container_edit (clip1, NULL, -1, GES_EDIT_MODE_NORMAL,
           GES_EDGE_NONE, 72) == TRUE);
   CHECK_OBJECT_PROPS (trackelement, 25, 0, 37);
-  CHECK_OBJECT_PROPS (trackelement1, 73, 5, 36);
+  CHECK_OBJECT_PROPS (trackelement1, 73, 5, 37);
   CHECK_OBJECT_PROPS (trackelement2, 110, 0, 60);
 
   /**
@@ -441,7 +440,7 @@ GST_START_TEST (test_snapping)
   fail_unless (ges_container_edit (clip1, NULL, -1, GES_EDIT_MODE_NORMAL,
           GES_EDGE_NONE, 58) == TRUE);
   CHECK_OBJECT_PROPS (trackelement, 25, 0, 37);
-  CHECK_OBJECT_PROPS (trackelement1, 62, 5, 36);
+  CHECK_OBJECT_PROPS (trackelement1, 62, 5, 37);
   CHECK_OBJECT_PROPS (trackelement2, 110, 0, 60);
 
 
diff --git a/tests/check/python/test_clip.py b/tests/check/python/test_clip.py
index 6df4f2654f..7f6929a29a 100644
--- a/tests/check/python/test_clip.py
+++ b/tests/check/python/test_clip.py
@@ -25,8 +25,6 @@ gi.require_version("GES", "1.0")
 from gi.repository import Gst  # noqa
 from gi.repository import GES  # noqa
 
-from .common import GESSimpleTimelineTest  # noqa
-
 import unittest  # noqa
 
 
diff --git a/tests/check/python/test_timeline.py b/tests/check/python/test_timeline.py
index 68bc5a3da2..d83cdae90b 100644
--- a/tests/check/python/test_timeline.py
+++ b/tests/check/python/test_timeline.py
@@ -27,7 +27,9 @@ from gi.repository import GES  # noqa
 import unittest  # noqa
 from unittest import mock
 
-from . import common
+from .common import create_main_loop
+from .common import create_project
+from .common import GESSimpleTimelineTest  # noqa
 
 Gst.init(None)
 GES.init()
@@ -36,8 +38,8 @@ GES.init()
 class TestTimeline(unittest.TestCase):
 
     def test_signals_not_emitted_when_loading(self):
-        mainloop = common.create_main_loop()
-        timeline = common.create_project(with_group=True, saved=True)
+        mainloop = create_main_loop()
+        timeline = create_project(with_group=True, saved=True)
 
         # Reload the project, check the group.
         project = GES.Project.new(uri=timeline.get_asset().props.uri)
@@ -52,7 +54,6 @@ class TestTimeline(unittest.TestCase):
         timeline = project.extract()
 
         signals = ["layer-added", "group-added", "track-added"]
-        called = []
         handle = mock.Mock()
         for signal in signals:
             timeline.connect(signal, handle)
@@ -62,7 +63,7 @@ class TestTimeline(unittest.TestCase):
         handle.assert_not_called()
 
 
-class TestEditing(common.GESSimpleTimelineTest):
+class TestEditing(GESSimpleTimelineTest):
 
     def test_transition_disappears_when_moving_to_another_layer(self):
         self.timeline.props.auto_transition = True
@@ -112,3 +113,23 @@ class TestEditing(common.GESSimpleTimelineTest):
         self.assertEquals(len(self.layer.get_clips()), 4)
         clip1.edit([], self.layer.get_priority(), GES.EditMode.EDIT_RIPPLE, GES.Edge.EDGE_NONE, 35 * Gst.SECOND)
         self.assertEquals(len(self.layer.get_clips()), 4)
+
+
+class TestSnapping(GESSimpleTimelineTest):
+
+    def test_snapping(self):
+        self.timeline.props.auto_transition = True
+        self.timeline.set_snapping_distance(1)
+        clip1 = self.add_clip(0, 0, 100)
+
+        # Split clip1.
+        split_position = 50
+        clip2 = clip1.split(split_position)
+        self.assertEquals(len(self.layer.get_clips()), 2)
+        self.assertEqual(clip1.props.duration, split_position)
+        self.assertEqual(clip2.props.start, split_position)
+
+        # Make sure snapping prevents clip2 to be moved to the left.
+        clip2.edit([], self.layer.get_priority(), GES.EditMode.EDIT_NORMAL, GES.Edge.EDGE_NONE,
+                   clip2.props.start - 1)
+        self.assertEqual(clip2.props.start, split_position)