ges: Avoid GESTimelineElement naming conflicts

When users (can be formatters) set timeline element names in the
default 'namespace' we need to update our counter to avoid setting
twice the same name on TimelineElements so afterward there is no
problem adding them in the GESTimeline

+ add a testcase to check that new code and fix leaks on the
existing testcases.

+ Sensibly enhance debugs

ges/ges-timeline-element.c

@@ -258,14 +258,15 @@ ges_timeline_element_class_init (GESTimelineElementClass * klass)
   klass->trim = NULL;
 }
 
-static gboolean
+static void
-_set_name_default (GESTimelineElement * self)
+_set_name (GESTimelineElement * self, const gchar * wanted_name)
 {
   const gchar *type_name;
+  gchar *lowcase_type;
   gint count;
-  gchar *name;
   GQuark q;
   guint i, l;
+  gchar *name = NULL;
 
   if (!object_name_counts) {
     g_datalist_init (&object_name_counts);
@@ -273,38 +274,57 @@ _set_name_default (GESTimelineElement * self)
 
   q = g_type_qname (G_OBJECT_TYPE (self));
   count = GPOINTER_TO_INT (g_datalist_id_get_data (&object_name_counts, q));
-  g_datalist_id_set_data (&object_name_counts, q, GINT_TO_POINTER (count + 1));
 
   /* GstFooSink -> foosink<N> */
   type_name = g_quark_to_string (q);
   if (strncmp (type_name, "GES", 3) == 0)
     type_name += 3;
-  /* give the 20th "uriclip" element and the first "uriclip2" (if needed in the future)
+
-   * different names */
+  lowcase_type = g_strdup (type_name);
-  l = strlen (type_name);
+  l = strlen (lowcase_type);
-  if (l > 0 && g_ascii_isdigit (type_name[l - 1])) {
+  for (i = 0; i < l; i++)
-    name = g_strdup_printf ("%s-%d", type_name, count);
+    lowcase_type[i] = g_ascii_tolower (lowcase_type[i]);
+
+  if (wanted_name == NULL) {
+    /* give the 20th "uriclip" element and the first "uriclip2" (if needed in the future)
+     * different names */
+    l = strlen (type_name);
+    if (l > 0 && g_ascii_isdigit (type_name[l - 1])) {
+      name = g_strdup_printf ("%s-%d", lowcase_type, count++);
+    } else {
+      name = g_strdup_printf ("%s%d", lowcase_type, count++);
+    }
   } else {
-    name = g_strdup_printf ("%s%d", type_name, count);
+    /* If the wanted name uses the same 'namespace' as default, make
+     * sure it does not badly interfere with our counting system */
+
+    if (g_str_has_prefix (wanted_name, lowcase_type)) {
+      guint64 tmpcount =
+          g_ascii_strtoull (&wanted_name[strlen (lowcase_type)], NULL, 10);
+
+      if (tmpcount > count) {
+        count = tmpcount + 1;
+        GST_DEBUG_OBJECT (self, "Using same naming %s but updated count to %i",
+            name, count);
+      } else if (tmpcount < count) {
+        name = g_strdup_printf ("%s%d", lowcase_type, count);
+        GST_DEBUG_OBJECT (self, "Name %s already allocated, giving: %s instead",
+            wanted_name, name);
+      } else {
+        count++;
+        GST_DEBUG_OBJECT (self, "Perfect name, just bumping object count");
+      }
+    }
+
+    if (name == NULL)
+      name = g_strdup (wanted_name);
   }
 
-  l = strlen (name);
+  g_free (lowcase_type);
-  for (i = 0; i < l; i++)
+  g_datalist_id_set_data (&object_name_counts, q, GINT_TO_POINTER (count));
-    name[i] = g_ascii_tolower (name[i]);
 
   g_free (self->name);
-  if (G_UNLIKELY (self->parent != NULL))
-    goto had_parent;
-
   self->name = name;
-
-  return TRUE;
-
-had_parent:
-  {
-    GST_WARNING ("parented objects can't be renamed");
-    return FALSE;
-  }
 }
 
 /*********************************************
@@ -408,12 +428,18 @@ ges_timeline_element_set_timeline (GESTimelineElement * self,
 
   if (timeline == NULL) {
     if (self->timeline) {
-      if (!timeline_remove_element (self->timeline, self))
+      if (!timeline_remove_element (self->timeline, self)) {
+        GST_INFO_OBJECT (self, "Could not remove from"
+            " currently set timeline %" GST_PTR_FORMAT, self->timeline);
         return FALSE;
+      }
     }
   } else {
-    if (!timeline_add_element (timeline, self))
+    if (!timeline_add_element (timeline, self)) {
+      GST_INFO_OBJECT (self, "Could not add to timeline %" GST_PTR_FORMAT,
+          self);
       return FALSE;
+    }
   }
 
   self->timeline = timeline;
@@ -992,12 +1018,14 @@ ges_timeline_element_get_name (GESTimelineElement * self)
 gboolean
 ges_timeline_element_set_name (GESTimelineElement * self, const gchar * name)
 {
-  gboolean result, readd_to_timeline = FALSE;
+  gboolean result = TRUE, readd_to_timeline = FALSE;
 
   g_return_val_if_fail (GES_IS_TIMELINE_ELEMENT (self), FALSE);
 
-  if (name != NULL && !g_strcmp0(name, self->name))
+  if (name != NULL && !g_strcmp0 (name, self->name)) {
+    GST_DEBUG_OBJECT (self, "Same name!");
     return TRUE;
+  }
 
   /* parented objects cannot be renamed */
   if (self->timeline != NULL) {
@@ -1012,13 +1040,7 @@ ges_timeline_element_set_name (GESTimelineElement * self, const gchar * name)
     readd_to_timeline = TRUE;
   }
 
-  if (name != NULL) {
+  _set_name (self, name);
-    g_free (self->name);
-    self->name = g_strdup (name);
-    result = TRUE;
-  } else {
-    result = _set_name_default (self);
-  }
 
   if (readd_to_timeline)
     timeline_add_element (self->timeline, self);

ges/ges-timeline.c

@@ -2394,9 +2394,14 @@ pad_removed_cb (GESTrack * track, GstPad * pad, TrackPrivate * tr_priv)
 gboolean
 timeline_add_element (GESTimeline * timeline, GESTimelineElement * element)
 {
+  GESTimelineElement *same_name =
+      g_hash_table_lookup (timeline->priv->all_elements,
+      element->name);
+
   GST_DEBUG_OBJECT (timeline, "Adding element: %s", element->name);
-  if (g_hash_table_contains (timeline->priv->all_elements, element->name)) {
+  if (same_name) {
-    GST_WARNING_OBJECT (timeline, "Already in %s the timeline", element->name);
+    GST_ERROR_OBJECT (timeline, "%s Already in the timeline %" GST_PTR_FORMAT,
+        element->name, same_name);
     return FALSE;
   }
 

tests/check/ges/basic.c

@@ -708,7 +708,7 @@ GST_END_TEST;
 
 GST_START_TEST (test_ges_timeline_element_name)
 {
-  GESClip *clip;
+  GESClip *clip, *clip1, *clip2, *clip3;
   GESAsset *asset;
   GESTimeline *timeline;
   GESLayer *layer;
@@ -721,9 +721,40 @@ GST_START_TEST (test_ges_timeline_element_name)
 
   asset = ges_asset_request (GES_TYPE_TEST_CLIP, NULL, NULL);
   clip = ges_layer_add_asset (layer, asset, 0, 0, 10, GES_TRACK_TYPE_UNKNOWN);
+  fail_unless_equals_string (GES_TIMELINE_ELEMENT_NAME (clip), "testclip0");
+
+
+  clip1 = GES_CLIP (ges_test_clip_new ());
+  fail_unless_equals_string (GES_TIMELINE_ELEMENT_NAME (clip1), "testclip1");
+
+  ges_timeline_element_set_name (GES_TIMELINE_ELEMENT (clip1), "testclip1");
+  fail_unless_equals_string (GES_TIMELINE_ELEMENT_NAME (clip1), "testclip1");
+
+  /* Check that trying to set to a name that is already used leads to
+   * a change in the name */
+  ges_timeline_element_set_name (GES_TIMELINE_ELEMENT (clip), "testclip1");
+  fail_unless_equals_string (GES_TIMELINE_ELEMENT_NAME (clip), "testclip2");
+
+  ges_timeline_element_set_name (GES_TIMELINE_ELEMENT (clip1), "testclip4");
+  fail_unless_equals_string (GES_TIMELINE_ELEMENT_NAME (clip1), "testclip4");
+
+  clip2 = GES_CLIP (ges_test_clip_new ());
+  fail_unless_equals_string (GES_TIMELINE_ELEMENT_NAME (clip2), "testclip5");
+  ges_timeline_element_set_name (GES_TIMELINE_ELEMENT (clip2), NULL);
+  fail_unless_equals_string (GES_TIMELINE_ELEMENT_NAME (clip2), "testclip6");
+
+  clip3 = GES_CLIP (ges_test_clip_new ());
+  ges_timeline_element_set_name (GES_TIMELINE_ELEMENT (clip3),
+      "Something I want!");
+  fail_unless_equals_string (GES_TIMELINE_ELEMENT_NAME (clip3),
+      "Something I want!");
+
   gst_object_unref (asset);
 
-  fail_unless_equals_string (GES_TIMELINE_ELEMENT_NAME (clip), "testclip0");
+  gst_object_unref (clip1);
+  gst_object_unref (clip2);
+  gst_object_unref (clip3);
+  gst_object_unref (timeline);
 }
 
 GST_END_TEST;