[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 #8 from Thibault Saunier <thibault.saunier at collabora.com> 2013-06-21 01:28:24 UTC ---
(In reply to comment #6)
> > 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.

DONE

> 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.
DONE

>  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.

This has been discussed on IRC, the idea is that a property should emit a
notify
signal only when the value is changed, which will happen only when commiting.
So we inhibit the emission of the signal until the value is actually changed. I
am concidering adding signals for pending value changes, emitting the pending
value in the signal itself, like:

    g_signal_emit ("pending-start", object->pending_start);

> > 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.

I reworded the commit message expliciting more what is actually done (it is
really only about review the indentation startegy)

> > gnlcomposition: Remove support for gaps
>
> Adds two white spaces, I don' get why.

Not sure what you mean here?

>
> > 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 removed those commit from the beanch, it is now at:

  https://github.com/pitivi/gnonlin/commits/misc_improvements

> 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).

That would be great, but I think it is important to try to get that stuff in
asap as we will need quite some debugging, and testing as it is pretty invasive
changes.

-- 
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.