[Bug 701287] gnonlin: API should be revisited before1.X first versions

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=701287
  GStreamer | gnonlin | git

--- Comment #6 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2013-06-18 00:56:16 UTC ---
> gnl: Remove trailling whitespaces and tabs in .c files

You have change tabs into space in few macros, but maybe it would be nice to
add the missing spaces now to get all those trailing backslash to be aligned,
while doing style patches.

E.g.:

-#define COMP_FLUSHING_UNLOCK(comp) G_STMT_START {            \
-    GST_LOG_OBJECT (comp, "unlocking flushing_lock from thread %p",    \
-            g_thread_self());                    \
-    g_mutex_unlock (&comp->priv->flushing_lock);                \
+#define COMP_FLUSHING_UNLOCK(comp) G_STMT_START {               \
+    GST_LOG_OBJECT (comp, "unlocking flushing_lock from thread %p", \
+        g_thread_self());                                                     
                       \
+    g_mutex_unlock (&comp->priv->flushing_lock);                              
   \


> gnl: First implementation of the commit based API

You move the implement outside of GnlObject later, would be nice to squash
those commits. You could probably move "gnl: Move the commit signal from
gnlobject to gnlcomposition" near that commit, and then squash, that should
reduce conflict I think.

 CHECK_AND_EMIT and SET_PENDING_VALUE, you could indent the \ properly. Also, I
don't get why CHECK_AND_EMIT need to emit, why not just marking as
"need_commit" and signal later ... We don't really care if it's updated twice
before commit, or does GES care ? Otherwise, maybe you'd prefer if the
properties always report the latest (the future value), making the commit the
other way around.

object->start = object->pending_start = 0;
+  object->duration = object->pending_duration = 0;

No need to set members to 0, all GObject a memset with 0.

+  /* We need to inhibit notify emition when the user changes properties, it
+   * will be emitted only when comiting */
+  for (i = 0; i < G_N_ELEMENTS (notifies_to_inhibit); i++)
+    g_signal_connect (object, notifies_to_inhibit[i],
+        G_CALLBACK (_discard_notify_cb), NULL);

see comment about CHECK_AND_EMIT, could make this not needed at all.

> gnlcomposition: Remove uneeded indents

You seem to be doing way more then that, I have no idea, can't see if anything
changed here, all lines are affected, and + lines does not look like - lines.

> gnlcomposition: Remove support for gaps

Adds two white spaces, I don' get why.

> composition: More reliably compute outgoing segment base time

This is unrelated change (not fully trivial)

> gnlcomposition: Remove the FLUSH_LOCK, us atomic operations instead

This is also unrelated. Also I'm not 100% convince that this is correct
(clearly not trivial).

> gnlcomposition: suffix callbacks with _cb

That one is unrelated but fall into the trivial case.

I think more people should have a look. Cleaning up would definatly help
reviewers. There seems to be some left over, maybe you could document what you
haven't address (some of it seems mentionned in the commit logs).

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.