Merge lp://qastaging/~daschuer/mixxx/atomic-co into lp://qastaging/~mixxxdevelopers/mixxx/atomic-co

We have to keep valueChangedFromEngine. It's important to be able to react
to internal sets of controls. For example this breaks BpmControl reacting
to engine rate changes. Try hitting the sync button. The BPM text won't
change.
On May 16, 2013 7:49 PM, "Daniel Schürmann" <email address hidden> wrote:

> Daniel Schürmann has proposed merging lp:~daschuer/mixxx/atomic-co into
> lp:~mixxxdevelopers/mixxx/atomic-co.
>
> Requested reviews:
> Mixxx Development Team (mixxxdevelopers)
>
> For more details, see:
> https://code.launchpad.net/~daschuer/mixxx/atomic-co/+merge/164281
>
> See commit log
> --
> https://code.launchpad.net/~daschuer/mixxx/atomic-co/+merge/164281
> Your team Mixxx Development Team is requested to review the proposed merge
> of lp:~daschuer/mixxx/atomic-co into lp:~mixxxdevelopers/mixxx/atomic-co.
>
> === modified file 'mixxx/src/control/control.cpp'
> --- mixxx/src/control/control.cpp 2013-05-16 01:40:06 +0000
> +++ mixxx/src/control/control.cpp 2013-05-16 23:48:28 +0000
> @@ -77,9 +77,12 @@
> return m_value.getValue();
> }
>
> -void ControlDoublePrivate::reset(QObject* pSender) {
> +void ControlDoublePrivate::reset() {
> double defaultValue = m_defaultValue.getValue();
> - set(defaultValue, pSender);
> + // NOTE: pSender = NULL is important. The originator of this action
> does
> + // not know the resulting value so it makes sense that we should emit
> a
> + // general valueChanged() signal even though we know the originator.
> + set(defaultValue, NULL);
> }
>
> void ControlDoublePrivate::set(const double& value, QObject* pSender) {
> @@ -106,9 +109,9 @@
> return m_pBehavior.fetchAndStoreRelaxed(pBehavior);
> }
>
> -void ControlDoublePrivate::setWidgetParameter(double dParam, QObject*
> pSetter) {
> +void ControlDoublePrivate::setWidgetParameter(double dParam, QObject*
> pSender) {
> ControlNumericBehavior* pBehavior = m_pBehavior;
> - set(pBehavior ? pBehavior->widgetParameterToValue(dParam) : dParam,
> pSetter);
> + set(pBehavior ? pBehavior->widgetParameterToValue(dParam) : dParam,
> pSender);
> }
>
> double ControlDoublePrivate::getWidgetParameter() const {
> @@ -116,12 +119,12 @@
> return pBehavior ? pBehavior->valueToWidgetParameter(get()) : get();
> }
>
> -void ControlDoublePrivate::setMidiParameter(MidiOpCode opcode, double
> dParam) {
> +void ControlDoublePrivate::setMidiParameter(MidiOpCode opcode, double
> dParam, QObject* pSender) {
> ControlNumericBehavior* pBehavior = m_pBehavior;
> if (pBehavior) {
> - pBehavior->setValueFromMidiParameter(opcode, dParam, this);
> + pBehavior->setValueFromMidiParameter(opcode, dParam, this,
> pSender);
> } else {
> - set(dParam, NULL);
> + set(dParam, pSender);
> }
> }
>
>
> === modified file 'mixxx/src/control/control.h'
> --- mixxx/src/control/control.h 2013-05-16 01:40:06 +0000
> +++ mixxx/src/control/control.h 2013-05-16 23:48:28 +0000
> @@ -32,11 +32,11 @@
> }
>
> // Sets the control value.
> - void set(const double& value, QObject* pSetter);
> + void set(const double& value, QObject* pSender);
> // Gets the control value.
> double get...

For me this is an issue from BpmControl.
In slotControlBeatSync it sets a value to m_pRateSlider and rely on the call of slotAdjustBpm.
Simply calling slotAdjustBpm manually will fix this.

I think we must have a close look to
mixxx/src/engine/ratecontrol.cpp
mixxx/src/engine/enginebuffer.cpp
mixxx/src/engine/bpmcontrol.cpp'
to find out similar issues and than it should be safe.

I like to do these changes, because it makes the code better understandable.

Our whole discussion convinced me, that it is the best approach:
NEVER receive a loopback value change signal.

This means all engine logic will become entangled. Take pitch bend for
example. Will you call slotadjustbpm from RateController? This will get bad
very quickly.
On May 17, 2013 2:12 AM, "Daniel Schürmann" <email address hidden> wrote:

> Review: Needs Fixing
>
> For me this is an issue from BpmControl.
> In slotControlBeatSync it sets a value to m_pRateSlider and rely on the
> call of slotAdjustBpm.
> Simply calling slotAdjustBpm manually will fix this.
>
> I think we must have a close look to
> mixxx/src/engine/ratecontrol.cpp
> mixxx/src/engine/enginebuffer.cpp
> mixxx/src/engine/bpmcontrol.cpp'
> to find out similar issues and than it should be safe.
>
> I like to do these changes, because it makes the code better
> understandable.
>
> Our whole discussion convinced me, that it is the best approach:
> NEVER receive a loopback value change signal.
>
>
> --
> https://code.launchpad.net/~daschuer/mixxx/atomic-co/+merge/164281
> Your team Mixxx Development Team is subscribed to branch
> lp:~mixxxdevelopers/mixxx/atomic-co.
>

Ah, I think I have got the point!

In the old model, we had a signal per thread and a thread based valve Logic.

Now, it is moved to CO/COT

So if RateControl sets "rate" it is done by his own private member m_pRateSlider which carry the valve logic.
BpmControl has an other private m_pRateSlider with its own valve logic.
So BpmControll will receive all updates from RateControl, (unless there is no bug ;-))

So we have now the benefit that two objects in the same thread can communicate via COs by a common interface :-))
Great!

Sure -- even the engine could use COTs now, but this is a more significant change than should be dealt with in this patch and something that IMO should wait until Control 2.0 is done (or even until the CO/COT are out of the engine completely a-la engine-control-refactor). I'm going to merge lp:~mixxxdevelopers/mixxx/atomic-co now.

For me CO and COT behave almost similar already. So the change is nothing more than run a refactor-rename. :-)