Code review comment for lp://staging/~mixxxdevelopers/mixxx/engine-control-refactor

Revision history for this message
Sean M. Pappalardo (pegasus-renegadetech) wrote :

I'm not qualified to review this so I just have some concerns & questions:

- The Qt docs attach warnings to specifying signal-slot connection types (e.g. Qt::DirectConnection,) saying that Qt automatically uses an appropriate one. (If the signal and slot are in the same thread, Qt::DirectConnection is used automatically.) They say if you find yourself needing to force this, then the threading model is suspect. http://qt-project.org/doc/qt-4.8/threads-qobject.html#signals-and-slots-across-threads
- Please explain your warning about CallbackControl::valueChanged*() as that seems to contradict the above (unless CC is expected to run in the thread of the slot, which would use DirectConnection anyway.)
- What does "callback-safe" mean?
- EngineClipping constructor: the comment for m_ctrlClipping still speaks of ControlPotMeter but the new code uses ControlObject. Which is correct?
- I know you didn't change this but while I'm looking at it, in EnginePassthrough::receiveBuffer: we should change the buffer overflow and underflow detection to assert on the conditional so if it happens, we know what failed. (Asserting on false isn't very informative.) For extra points, use Q_ASSERT_X and add "buffer over/underflow" text.

« Back to merge proposal