clip: Emit additional signals after child-removed

When removing an effect from a clip, first the notify::priority signals
were being emitted for the remaining effects which changed priority, and only
at the end the child-removed signal. Now the child-removed signal is emitted
first.

ges/ges-clip.c

@@ -369,18 +369,6 @@ static gboolean
 _remove_child (GESContainer * container, GESTimelineElement * element)
 {
   if (GES_IS_BASE_EFFECT (element)) {
-    GList *tmp;
-    GESChildrenControlMode mode = container->children_control_mode;
-
-    GST_DEBUG_OBJECT (container, "Resyncing effects priority.");
-
-    container->children_control_mode = GES_CHILDREN_UPDATE_OFFSETS;
-    tmp = g_list_find (GES_CONTAINER_CHILDREN (container), element);
-    for (tmp = tmp->next; tmp; tmp = tmp->next) {
-      ges_timeline_element_set_priority (GES_TIMELINE_ELEMENT (tmp->data),
-          GES_TIMELINE_ELEMENT_PRIORITY (tmp->data) - 1);
-    }
-    container->children_control_mode = mode;
     GES_CLIP (container)->priv->nb_effects--;
   }
 
@@ -405,6 +393,28 @@ _child_removed (GESContainer * container, GESTimelineElement * element)
   g_signal_handlers_disconnect_by_func (element, _child_priority_changed_cb,
       container);
 
+  if (GES_IS_BASE_EFFECT (element)) {
+    GList *tmp;
+    guint32 priority;
+    GESChildrenControlMode mode = container->children_control_mode;
+
+    GST_DEBUG_OBJECT (container, "Resyncing effects priority.");
+
+    container->children_control_mode = GES_CHILDREN_UPDATE_OFFSETS;
+    tmp = GES_CONTAINER_CHILDREN (container);
+    priority =
+        ges_timeline_element_get_priority (GES_TIMELINE_ELEMENT (element));
+    for (; tmp; tmp = tmp->next) {
+      if (ges_timeline_element_get_priority (GES_TIMELINE_ELEMENT (tmp->data)) >
+          priority) {
+        ges_timeline_element_set_priority (GES_TIMELINE_ELEMENT (tmp->data),
+            GES_TIMELINE_ELEMENT_PRIORITY (tmp->data) - 1);
+      }
+    }
+
+    container->children_control_mode = mode;
+  }
+
   _compute_height (container);
 }
 

ges/ges-container.c

@@ -465,7 +465,7 @@ ges_container_class_init (GESContainerClass * klass)
    */
   ges_container_signals[CHILD_REMOVED_SIGNAL] =
       g_signal_new ("child-removed", G_TYPE_FROM_CLASS (klass),
-      G_SIGNAL_RUN_FIRST, G_STRUCT_OFFSET (GESContainerClass, child_removed),
+      G_SIGNAL_RUN_LAST, G_STRUCT_OFFSET (GESContainerClass, child_removed),
       NULL, NULL, g_cclosure_marshal_generic, G_TYPE_NONE, 1,
       GES_TYPE_TIMELINE_ELEMENT);
 

tests/check/python/common.py

@@ -38,15 +38,17 @@ def create_main_loop():
     mainloop = GLib.MainLoop()
     timed_out = False
 
-    def quit_cb(unused):
+    def timeout_cb(unused):
         nonlocal timed_out
         timed_out = True
         mainloop.quit()
 
-    def run(timeout_seconds=5):
+    def run(timeout_seconds=5, until_empty=False):
         source = GLib.timeout_source_new_seconds(timeout_seconds)
-        source.set_callback(quit_cb)
+        source.set_callback(timeout_cb)
         source.attach()
+        if until_empty:
+            GLib.idle_add(mainloop.quit)
         GLib.MainLoop.run(mainloop)
         source.destroy()
         if timed_out:
@@ -81,6 +83,7 @@ def create_project(with_group=False, saved=False):
 
 
 class GESTest(unittest.TestCase):
+
     def _log(self, func, format, *args):
         string = format
         if args:
@@ -105,8 +108,27 @@ class GESTest(unittest.TestCase):
     def error(self, format, *args):
         self._log(Gst.error, format, *args)
 
+    def check_clip_values(self, clip, start, in_point, duration):
+        for elem in [clip] + clip.get_children(False):
+            self.check_element_values(elem, start, in_point, duration)
+
+    def check_element_values(self, element, start, in_point, duration):
+        self.assertEqual(element.props.start, start, element)
+        self.assertEqual(element.props.in_point, in_point, element)
+        self.assertEqual(element.props.duration, duration, element)
+
+    def assert_effects(self, clip, *effects):
+        # Make sure there are no other effects.
+        self.assertEqual(set(clip.get_top_effects()), set(effects))
+
+        # Make sure their order is correct.
+        indexes = [clip.get_top_effect_index(effect)
+                   for effect in effects]
+        self.assertEqual(indexes, list(range(len(effects))))
+
 
 class GESSimpleTimelineTest(GESTest):
+
     def __init__(self, *args):
         self.track_types = [GES.TrackType.AUDIO, GES.TrackType.VIDEO]
         super(GESSimpleTimelineTest, self).__init__(*args)
@@ -117,9 +139,9 @@ class GESSimpleTimelineTest(GESTest):
             self.assertIn(
                 track_type, [GES.TrackType.AUDIO, GES.TrackType.VIDEO])
             if track_type == GES.TrackType.AUDIO:
-                self.timeline.add_track(GES.AudioTrack.new())
+                self.assertTrue(self.timeline.add_track(GES.AudioTrack.new()))
             else:
-                self.timeline.add_track(GES.VideoTrack.new())
+                self.assertTrue(self.timeline.add_track(GES.VideoTrack.new()))
 
         self.assertEqual(len(self.timeline.get_tracks()),
                          len(self.track_types))
@@ -130,15 +152,7 @@ class GESSimpleTimelineTest(GESTest):
         clip.props.start = start
         clip.props.in_point = in_point
         clip.props.duration = duration
-        self.layer.add_clip(clip)
+        self.assertTrue(self.layer.add_clip(clip))
 
         return clip
 
-    def check_clip_values(self, clip, start, in_point, duration):
-        for elem in [clip] + clip.get_children(False):
-            self.check_element_values(elem, start, in_point, duration)
-
-    def check_element_values(self, element, start, in_point, duration):
-        self.assertEqual(element.props.start, start, element)
-        self.assertEqual(element.props.in_point, in_point, element)
-        self.assertEqual(element.props.duration, duration, element)

tests/check/python/test_clip.py

@@ -30,6 +30,8 @@ Gst.init(None)  # noqa
 from gi.repository import GES  # noqa
 GES.init()
 
+from . import common  # noqa
+
 import unittest  # noqa
 
 
@@ -137,7 +139,7 @@ class TestTitleClip(unittest.TestCase):
                             children2[1].props.priority)
 
 
-class TestTrackElements(unittest.TestCase):
+class TestTrackElements(common.GESTest):
 
     def test_add_to_layer_with_effect_remove_add(self):
         timeline = GES.Timeline.new_audio_video()
@@ -167,31 +169,58 @@ class TestTrackElements(unittest.TestCase):
         self.assertFalse(audio_source is None)
         self.assertEqual(audio_source.get_child_property("volume")[1], 0.0)
 
-    def check_effects(self, clip, expected_effects, expected_indexes):
-        effects = clip.get_top_effects()
-        self.assertEqual(effects, expected_effects)
-        self.assertEqual([clip.get_top_effect_index(effect) for effect in effects], expected_indexes)
-
     def test_effects_priority(self):
         timeline = GES.Timeline.new_audio_video()
-        self.assertEqual(len(timeline.get_tracks()), 2)
         layer = timeline.append_layer()
 
-        test_clip = GES.TestClip()
+        test_clip = GES.TestClip.new()
-        self.assertEqual(test_clip.get_children(True), [])
+        layer.add_clip(test_clip)
-        self.assertTrue(layer.add_clip(test_clip))
+        self.assert_effects(test_clip)
 
         effect1 = GES.Effect.new("agingtv")
         test_clip.add(effect1)
-        self.check_effects(test_clip, [effect1], [0])
+        self.assert_effects(test_clip, effect1)
 
         test_clip.set_top_effect_index(effect1, 1)
-        self.check_effects(test_clip, [effect1], [0])
+        self.assert_effects(test_clip, effect1)
         test_clip.set_top_effect_index(effect1, 10)
-        self.check_effects(test_clip, [effect1], [0])
+        self.assert_effects(test_clip, effect1)
 
         effect2 = GES.Effect.new("dicetv")
         test_clip.add(effect2)
+        self.assert_effects(test_clip, effect1, effect2)
 
         test_clip.remove(effect1)
-        self.check_effects(test_clip, [effect2], [0])
+        self.assert_effects(test_clip, effect2)
+
+    def test_signal_order_when_removing_effect(self):
+        timeline = GES.Timeline.new_audio_video()
+        layer = timeline.append_layer()
+
+        test_clip = GES.TestClip.new()
+        layer.add_clip(test_clip)
+        self.assert_effects(test_clip)
+
+        effect1 = GES.Effect.new("agingtv")
+        test_clip.add(effect1)
+        effect2 = GES.Effect.new("dicetv")
+        test_clip.add(effect2)
+        self.assert_effects(test_clip, effect1, effect2)
+
+        mainloop = common.create_main_loop()
+
+        signals = []
+
+        def handler_cb(*args):
+            signals.append(args[-1])
+
+        test_clip.connect("child-removed", handler_cb, "child-removed")
+        effect2.connect("notify::priority", handler_cb, "notify::priority")
+        test_clip.remove(effect1)
+        test_clip.disconnect_by_func(handler_cb)
+        effect2.disconnect_by_func(handler_cb)
+        self.assert_effects(test_clip, effect2)
+
+        mainloop.run(until_empty=True)
+
+        self.assertEqual(signals, ["child-removed", "notify::priority"])

tests/check/python/test_group.py

@@ -37,6 +37,7 @@ GES.init()
 
 
 class TestGroup(common.GESSimpleTimelineTest):
+
     def testCopyGroup(self):
         clip1 = GES.TestClip.new()
         clip1.props.duration = 10