ges: Don't remove track elements from clips when removing from layer

And reuse the same previously created element when adding the clip
back to a layer, avoiding losing all setting done on clip children
in that situation

This is a behaviour change but previous behaviour was actually totally
unexpected and people working around that weird behaviour will moste
probably not care about that change

Differential Revision: https://phabricator.freedesktop.org/D1094

6 files changed, 67 insertions, 12 deletions

diff --git a/ges/ges-clip.c b/ges/ges-clip.c
index a7bcc5d2..74438488 100644
--- a/ges/ges-clip.c
+++ b/ges/ges-clip.c
@@ -869,7 +869,7 @@ ges_clip_create_track_element (GESClip * clip, GESTrackType type)
 GList *
 ges_clip_create_track_elements (GESClip * clip, GESTrackType type)
 {
-  GList *result, *tmp;
+  GList *result = NULL, *tmp, *children;
   GESClipClass *klass;
   guint max_prio, min_prio;
 
@@ -884,7 +884,23 @@ ges_clip_create_track_elements (GESClip * clip, GESTrackType type)
 
   GST_DEBUG_OBJECT (clip, "Creating TrackElements for type: %s",
       ges_track_type_name (type));
-  result = klass->create_track_elements (clip, type);
+  children = ges_container_get_children (GES_CONTAINER (clip), TRUE);
+  for (tmp = children; tmp; tmp = tmp->next) {
+    GESTrackElement *child = GES_TRACK_ELEMENT (tmp->data);
+
+    if (!GES_IS_BASE_EFFECT (child) && !ges_track_element_get_track (child) &&
+        ges_track_element_get_track_type (child) & type) {
+
+      GST_DEBUG_OBJECT (clip, "Removing for reusage: %" GST_PTR_FORMAT, child);
+      result = g_list_prepend (result, g_object_ref (child));
+      ges_container_remove (GES_CONTAINER (clip), tmp->data);
+    }
+  }
+  g_list_free_full (children, gst_object_unref);
+
+  if (!result) {
+    result = klass->create_track_elements (clip, type);
+  }
 
   _get_priority_range (GES_CONTAINER (clip), &min_prio, &max_prio);
   for (tmp = result; tmp; tmp = tmp->next) {
diff --git a/ges/ges-container.c b/ges/ges-container.c
index 7c62305e..f52aea00 100644
--- a/ges/ges-container.c
+++ b/ges/ges-container.c
@@ -355,9 +355,15 @@ _paste (GESTimelineElement * element, GESTimelineElement * ref,
 static void
 _dispose (GObject * object)
 {
+  GList *tmp;
   GESContainer *self = GES_CONTAINER (object);
+  GList *children = ges_container_get_children (self, FALSE);
+
+  for (tmp = children; tmp; tmp = tmp->next)
+    ges_container_remove (self, tmp->data);
 
-  g_hash_table_unref (self->priv->mappings);
+  g_list_free_full (children, gst_object_unref);
+  self->children = NULL;
 
   G_OBJECT_CLASS (ges_container_parent_class)->dispose (object);
 }
diff --git a/ges/ges-timeline.c b/ges/ges-timeline.c
index e8d06552..681d0a9b 100644
--- a/ges/ges-timeline.c
+++ b/ges/ges-timeline.c
@@ -2511,6 +2511,9 @@ layer_object_removed_cb (GESLayer * layer, GESClip * clip,
     GESTrackElement *track_element = (GESTrackElement *) tmp->data;
     GESTrack *track = ges_track_element_get_track (track_element);
 
+    if (!track)
+      continue;
+
     GST_DEBUG_OBJECT (timeline, "Trying to remove TrackElement %p",
         track_element);
 
@@ -2521,8 +2524,7 @@ layer_object_removed_cb (GESLayer * layer, GESClip * clip,
                 (GCompareFunc) custom_find_track) || track == NULL)) {
       GST_DEBUG ("Belongs to one of the tracks we control");
 
-      ges_container_remove (GES_CONTAINER (clip),
-          GES_TIMELINE_ELEMENT (track_element));
+      ges_track_remove_element (track, track_element);
     }
     UNLOCK_DYN (timeline);
   }
diff --git a/ges/ges-uri-clip.c b/ges/ges-uri-clip.c
index 0252d3d6..133692d3 100644
--- a/ges/ges-uri-clip.c
+++ b/ges/ges-uri-clip.c
@@ -293,7 +293,9 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset)
   GES_TIMELINE_ELEMENT (uriclip)->asset = asset;
 
   if (layer) {
-    for (tmp = GES_CONTAINER_CHILDREN (self); tmp; tmp = tmp->next) {
+    GList *children = ges_container_get_children (GES_CONTAINER (self), TRUE);
+
+    for (tmp = children; tmp; tmp = tmp->next) {
       if (GES_IS_SOURCE (tmp->data)) {
         GESTrack *track = ges_track_element_get_track (tmp->data);
 
@@ -303,10 +305,13 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset)
           video_source = gst_object_ref (tmp->data);
 
         ges_track_remove_element (track, tmp->data);
+        ges_container_remove (GES_CONTAINER (self), tmp->data);
       }
     }
+    g_list_free_full (children, g_object_unref);
 
     gst_object_ref (clip);
+
     ges_layer_remove_clip (layer, clip);
     res = ges_layer_add_clip (layer, clip);
 
@@ -314,9 +319,9 @@ extractable_set_asset (GESExtractable * self, GESAsset * asset)
       if (GES_IS_SOURCE (tmp->data)) {
         GESTrack *track = ges_track_element_get_track (tmp->data);
 
-        if (track->type == GES_TRACK_TYPE_AUDIO)
+        if (track->type == GES_TRACK_TYPE_AUDIO && audio_source)
           ges_track_element_copy_properties (audio_source, tmp->data);
-        else if (track->type == GES_TRACK_TYPE_VIDEO)
+        else if (track->type == GES_TRACK_TYPE_VIDEO && video_source)
           ges_track_element_copy_properties (video_source, tmp->data);
       }
     }
diff --git a/tests/check/ges/basic.c b/tests/check/ges/basic.c
index 6da18bb3..039190db 100644
--- a/tests/check/ges/basic.c
+++ b/tests/check/ges/basic.c
@@ -119,8 +119,6 @@ GST_START_TEST (test_ges_scenario)
   ASSERT_OBJECT_REFCOUNT (layer, "layer", 1);
   tmp_layer = ges_clip_get_layer (GES_CLIP (source));
   fail_unless (tmp_layer == NULL);
-  trackelements = GES_CONTAINER_CHILDREN (source);
-  fail_unless (trackelements == NULL);  /* No unreffing then */
   gst_object_unref (source);
 
   GST_DEBUG ("Removing track from the timeline");
@@ -499,9 +497,9 @@ GST_START_TEST (test_ges_timeline_remove_track)
   tmp = ges_layer_get_clips (layer);
   assert_equals_int (g_list_length (tmp), 3);
   g_list_foreach (tmp, (GFunc) gst_object_unref, NULL);
-  g_list_free (tmp);
 
-  check_destroyed (G_OBJECT (timeline), G_OBJECT (layer), t1, t2, t3, NULL);
+  gst_check_objects_destroyed_on_unref (G_OBJECT (timeline),
+      G_OBJECT (layer), t1, t2, t3, NULL);
 }
 
 GST_END_TEST;
diff --git a/tests/check/python/test_clip.py b/tests/check/python/test_clip.py
index 9bbce02d..9c91a142 100644
--- a/tests/check/python/test_clip.py
+++ b/tests/check/python/test_clip.py
@@ -98,3 +98,31 @@ class TestTitleClip(unittest.TestCase):
         children2 = clip2.get_children(True)
         self.assertNotEqual(children2[0].props.priority,
                             children2[1].props.priority)
+class TestTrackElements(unittest.TestCase):
+    def test_add_to_layer_with_effect_remove_add(self):
+        timeline = GES.Timeline.new_audio_video()
+        self.assertEqual(len(timeline.get_tracks()), 2)
+        layer = timeline.append_layer()
+
+        test_clip = GES.TestClip()
+        self.assertEqual(test_clip.get_children(True), [])
+        self.assertTrue(layer.add_clip(test_clip))
+        audio_source = test_clip.find_track_element(None, GES.AudioSource)
+        self.assertIsNotNone(audio_source)
+
+        self.assertTrue(test_clip.set_child_property("volume", 0.0))
+        self.assertEqual(audio_source.get_child_property("volume")[1], 0.0)
+
+        effect = GES.Effect.new("agingtv")
+        test_clip.add(effect)
+
+        children = test_clip.get_children(True)
+        layer.remove_clip(test_clip)
+        self.assertEqual(test_clip.get_children(True), children)
+
+        self.assertTrue(layer.add_clip(test_clip))
+        self.assertEqual(test_clip.get_children(True), children)
+
+        audio_source = test_clip.find_track_element(None, GES.AudioSource)
+        self.assertFalse(audio_source is None)
+        self.assertEqual(audio_source.get_child_property("volume")[1], 0.0)