Merge lp://qastaging/~ywwg/mixxx/atomic-master-sync into lp://qastaging/~mixxxdevelopers/mixxx/trunk

Non-engine code:

src/dlg*
- looks fine, enables 3/4

src/mixxx*
- looks fine, enables 3/4

src/vinylcontrol
- looks fine, enables 3/4

src/controlobject.cpp
- the warning should stay enabled ideally, it's a good clue for when users send us logs

src/controlvaluedelegate.cpp
- looks fine

src/playermanager*

- Hm, typically we decided deck orientation on whether the number was even odd. That's nice because you don't have to know the total number of decks to decide which ones go left/right. Why the change?

- The skin-num-decks isn't totally complete. For example, WTrackTableView context menu for load-to-deck doesn't use it.

- Need to do some bounds checking on the double value before casting it to int (pretty sure casting a negative floating point number to an unsigned integer is undefined behavior) or make m_skin_decks signed.

Nice work sofar! Here are some quick comments, mostly nits since I haven't fully wrapped my head around the actual logic.

* EngineSync::addDeck() -- connections should be Qt::DirectConnection

* EngineSync::disableDeckMaster() fail gracefully (and complain loudly) instead of Q_ASSERT. Every "should never happen" programming error manages to happen and bring down the party someday =).

* EngineSync::setMaster() -- redundant return; statement on last line (and in some other functions)

* EngineSync::setInternalMaster(void) -- void in argument list is redundant imo

* EngineSync::setDeckMaster(QString)

- deck == NULL should only be done if deck is a pointer. This will implicitly convert NULL into a QString and compare that with deck. That's fine because NULL will turn into an empty QString but it's probably not what you intended.

- prefer deck.isEmpty() instead of deck == ""

* EngineSync::slotSyncRateSliderChanged(double newBpm)

- is newBpm a BPM or a rate? m_pMasterBpm gets set to newBpm

* EngineSync::slotMasterBpmChanged

- handle error instead of Q_ASSERT

* EngineSync::slotDeckStateChanged

- unfortunately sender() can be NULL and it does happen in practice. This is usually in conjunction with MIDI script. Don't Q_ASSERT here :).

* EngineSync::updateSamplesPerBeat

- minor nit -- static_cast<double>(m_iFoo * 10.0) -- static_cast is redundant because the inner expression is already a double because an int * double is a double.

* enginesync.h

- Indentation is slightly off from the Mixxx standard. Access declarations (public/private/protected) should have 2-space indent and don't count as an indentation for all member/method declarations (so all members/methods ends up 4-spaces indented).

- Destructor should be virtual

- Generally, to reduce dependence across header files I prefer to forward declare Mixxx classes. (I don't forward declare Qt classes).

So, in this case if you aren't using anything other than a pointer in the header file you can forward declare:

ControlObject, ControlPushbutton, ControlPotmeter, EngineMaster, EngineBuffer.

- Another general thing is that pointer stars should go with the type instead of the variable name. For this reason, I avoid multiple variable declaration statements:

ControlObject *m_pSourceBeatDistance, *m_pMasterBeatDistance;

Because it isn't possible to attach the star to the type in this case. It's not super strict but this is the convention in most places in Mixxx.

* LoopingControl

- I'd prefer to not add accessor mehods to LoopingControl and instead use the control system to share the value.

* RateControl::setEngineMaster
- Why the duplicate m_pEngineMaster variable w/ EngineControl?

* RateControl::slotSyncMasterChanged

- I've seen this in a couple places, if (dValue) { } relies on implicit conversion from double to boolean. Instead, be explicit (and to stick with convention elsewhere, only positive values count for a trigger from a control if (dValue > 0) { ... }.

* EngineChannel -- LGTM

* EngineMaster -- reduce copypasta in deciding whether to process a deck with an inline utility function?

BpmControl / RateControl / EngineSync / EngineBuffer is clearly The ...

Regarding ASSERTs, we really should make a timeline to start disabling QDebug in release builds which also disables asserts. I'll make a blueprint for that now.

I'll write up a proper design document today. The Big Idea is that all decks report bpm and position information, and the EngineSync listens to whoever is master and writes that information to the master sync COs. The slaves then only have to listen to that one set of COs and they'll get the correct information no matter which deck is master.

> Non-engine code:
>
>
> src/controlobject.cpp
> - the warning should stay enabled ideally, it's a good clue for when users
> send us logs

Oh right. Normally I suppress this because it causes a lot of spew. I'll remove that.

>
>
> src/playermanager*
>
> - Hm, typically we decided deck orientation on whether the number was even
> odd. That's nice because you don't have to know the total number of decks to
> decide which ones go left/right. Why the change?

This is something I'm really not sure about. With a four deck mixer, the pots are numbered:

1, 2, 3, 4

So 1 and 2 are on the left, and 3 and 4 are on the right. Calling 3 a left deck is totally counter-intuitive to me. However most 4-channel mixers have A/B switches so that any deck can be aligned left right or center.

>
> - The skin-num-decks isn't totally complete. For example, WTrackTableView
> context menu for load-to-deck doesn't use it.

OK. Do you agree with the general idea that Mixxx should know how many decks the UI has? That way it wouldn't load tracks into an invisible 3rd deck (since right now we don't support removing decks once they are created).

>
> - Need to do some bounds checking on the double value before casting it to int
> (pretty sure casting a negative floating point number to an unsigned integer
> is undefined behavior) or make m_skin_decks signed.

ok

"Calling 3 a left deck is totally counter-intuitive to me."

Sure, for a 4-channel mixer, but many 4-channel controllers use 1 & 3 on the left, and 2 & 4 on the right (like the Terminal Mix 2/4, VMS4, etc.)

RJ and I discussed the 4 deck naming issue. It appears there are many way of number decks, including on controllers:

ABCD
CABD
ACDB

We will have to support whatever the user wants. but that is out of scope for this patch -- I will remove that change.

Moving this discussion to: https://github.com/mixxxdj/mixxx/pull/1

just kidding! actually https://github.com/mixxxdj/mixxx/pull/2