Code review comment for lp:~mixxxdevelopers/mixxx/atomic-co

On May 16, 2013 7:46 PM, "Daniel Schürmann" <email address hidden> wrote:
>
> Review: Needs Fixing
>
> Here some results:
>
> * I think the CO is registered twice: In ControlObject::ControlObject()
and in ControlDoublePrivate::getControl()

On mobile - sorry for brevity.

We have to keep this until we get rid of CO::getControl

> * it is not clear where the home of m_sqCOHash is, for me it should move
to control double private

Agree but see above.

> * all static functions should have the leading comment // static

Thx will change

> * control.cpp/h should be renamed to controldoubleprivate.cpp/h

I think all control implementations should go in control.h so there's one
header to include. I could see this not being a public part of the control
system so maybe that makes sense.

> * DRY violation in overloaded ControlObject::ControlObject()

Will fix with a common init method

> * setMidiParameter needs a pSender

The sender in this case is the midi controller and the midi controller uses
raw COs instead of creating a COT so its not correct to do this how you did
... When MController gets changed to use COT then that should be the sender

> * gcc complains about the missing virtual destructors in the *Behaviour
classes

K can fix

> * i do not like to distingish between valueChanged(v) and
valueChangedByThis(v), because it is most likely that if an object wants to
receive valueChangedByThis(v) it will also receive valueChanged(v).

Yes but that's because by default code is not safe until carefully reviewed
for infinite loop prevention. Once it is reviewed it is natural we would
like to respond to both internal and external. There may be cases in the
future where you would only want to react to internally sourced changes.

> For this propose we should implement a new proxy which sends out pSender,
or allow to use ControlDoublePrivate directly

I've explained why we need this signal -- it's safer for writing code
because it avoids infinite feedback loops and it allows you to distinguish
between local and remote value changes (which is already used today in
mixxx).

Can you explain why you think we should not do this?

>
> please not the merge request with additional changes
> --
> https://code.launchpad.net/~mixxxdevelopers/mixxx/atomic-co/+merge/153696
> You are reviewing the proposed merge of
lp:~mixxxdevelopers/mixxx/atomic-co into lp:mixxx.