Created attachment 368906 [details] [review]
Added paste functionality to GESTimeline class

Review of attachment 368906 [details] [review]:

Looks almost all right :-)

::: ges/ges-timeline-element.h
@@ -138,2 +138,4 @@
   GESTimeline *timeline;
   gchar *name;
+  /* For copy-paste operation */
+  GESTimelineElement *copied_from;

This break the ABI (it would require to add `gpointer _ges_reserved[GES_PADDING_LARGE -1];` in the same structure), but you should rather had an internal method to get it as I do not think that should be API

::: ges/ges-timeline.h
@@ -159,2 +159,4 @@
 GST_EXPORT
 gboolean ges_timeline_is_empty (GESTimeline * timeline);
+GST_EXPORT
+GESTimelineElement * ges_timeline_paste (GESTimeline * timeline,

I would call it `ges_timeline_paste_elemt`.

Created attachment 368927 [details] [review]
Added paste functionality to GESTimeline class

(In reply to Thibault Saunier from comment #2)
> Review of attachment 368906 [details] [review] [review]:
> 
> Looks almost all right :-)
> 
> ::: ges/ges-timeline-element.h
> @@ -138,2 +138,4 @@
>    GESTimeline *timeline;
>    gchar *name;
> +  /* For copy-paste operation */
> +  GESTimelineElement *copied_from;
> 
> This break the ABI (it would require to add `gpointer
> _ges_reserved[GES_PADDING_LARGE -1];` in the same structure), but you should
> rather had an internal method to get it as I do not think that should be API
>
> Made the appropriate changes.
> 
> ::: ges/ges-timeline.h
> @@ -159,2 +159,4 @@
>  GST_EXPORT
>  gboolean ges_timeline_is_empty (GESTimeline * timeline);
> +GST_EXPORT
> +GESTimelineElement * ges_timeline_paste (GESTimeline * timeline,
> 
> I would call it `ges_timeline_paste_elemt`.
> Renamed function to 'ges_timeline_paste_element'

Review of attachment 368927 [details] [review]:

::: ges/ges-timeline-element.h
@@ -196,2 +196,3 @@
                                             GObject **child, GParamSpec **pspec);
   GESTrackType (*get_track_types)          (GESTimelineElement * self);
+  GESTimelineElement * (*get_copied_from) (GESTimelineElement * self);

This is not what I meant :-) - You should add in ges/ges-internal.h:

> G_GNUC_INTERNAL GESTimelineElement* ges_timeline_element_get_copied_from (GESTimelineElement *self);

and implement that in ges-timeline-element.c

Created attachment 368951 [details] [review]
Added paste functionality to GESTimeline class

Review of attachment 368951 [details] [review]:

::: ges/ges-timeline.c
@@ +3782,3 @@
+  res = element_class->paste (element, copied_from, paste_position);
+
+  g_clear_object (&copied_from);

You do not own the reference here, GESTimelineElement does, you will need to add a way to clean that ref, I guess we can state that ges_timeline_element_get_copied_from gives it reference and sets it back to NULL (ie. in that method result priv->copied_from to NULL).

Created attachment 368980 [details] [review]
Added paste functionality to GESTimeline class

Review of attachment 368980 [details] [review]:

It is almost good and I am being a bit annoying but let's get it **right** ;)

::: ges/ges-timeline-element.c
@@ +1863,3 @@
+ges_timeline_element_get_copied_from (GESTimelineElement * self)
+{
+  GESTimelineElement * copied_from_ref = gst_object_ref(self->priv->copied_from);

I am pretty much sure it is not `gst-indent` complient, could you check that please?

You could have done:

```
copied_from = self->priv->copied_from;
self->priv->copied_from = NULL;
return copied_from;
```

Review of attachment 368980 [details] [review]:

::: ges/ges-timeline.c
@@ +3743,3 @@
+ * Paste @element inside the timeline. @element must have been
+ * created using ges_timeline_element_copy with recurse=TRUE set,
+ * @paste_position: The position in the timeline the element should

How "it will fail"? Describe instead exactly what happens.

It's too bad you can't paste an object you created yourself.. which are not copied.

@@ +3745,3 @@
+ * otherwise it will fail.
+ *
+ * be pasted to, meaning it will become the start of @element

Is this better? "The @element copy which has been pasted"

@@ +3761,3 @@
+
+  if (!copied_from) {
+ * Returns: (transfer none): Paste @element copying the element

*has not been copied recursively

@@ +3767,3 @@
+
+  if (!element_class->paste) {
+ges_timeline_paste_element (GESTimeline * timeline, GESTimelineElement * element,

Should we print element or element_class?

::: ges/ges-timeline.h
@@ +161,3 @@
+GST_EXPORT
+GESTimelineElement * ges_timeline_paste_element (GESTimeline * timeline,
+  GESTimelineElement * element, GstClockTime paste_position, gint layer_priority);

Why "paste_position" and not "position"?

(In reply to Thibault Saunier from comment #9)
> Review of attachment 368980 [details] [review] [review]:
> 
> It is almost good and I am being a bit annoying but let's get it **right** ;)
> 
> ::: ges/ges-timeline-element.c
> @@ +1863,3 @@
> +ges_timeline_element_get_copied_from (GESTimelineElement * self)
> +{
> +  GESTimelineElement * copied_from_ref =
> gst_object_ref(self->priv->copied_from);
> 
> I am pretty much sure it is not `gst-indent` complient, could you check that
> please?
> 
> You could have done:
> 
> ```
> copied_from = self->priv->copied_from;
> self->priv->copied_from = NULL;
> return copied_from;
> ```

Yea, it is not gst-indent compliant.

Review of attachment 368980 [details] [review]:

::: ges/ges-timeline.c
@@ +3743,3 @@
+ * Paste @element inside the timeline. @element must have been
+ * created using ges_timeline_element_copy with recurse=TRUE set,
+ * otherwise it will fail.

Well, to paste you need to copy... that's just the way it works, and it obviously makes sense.

@@ +3767,3 @@
+
+  if (!element_class->paste) {
+    GST_ERROR_OBJECT (element, "No paste vmethod implemented");

It is fine this way.

Created attachment 368983 [details] [review]
Added paste functionality to GESTimeline class

Attachment 368983 [details] pushed as e944739 - Added paste functionality to GESTimeline class

Do you need it to be backported to 1.12 for Pitivi 1.0?

I am not sure. @aleb can comment better on that :)