Merge lp:~mixxxdevelopers/mixxx/features_beatcontrol into lp:~mixxxdevelopers/mixxx/trunk

Proposed by Phillip Whelan
Status: Merged
Merged at revision: 2747
Proposed branch: lp:~mixxxdevelopers/mixxx/features_beatcontrol
Merge into: lp:~mixxxdevelopers/mixxx/trunk
Diff against target: 0 lines
To merge this branch: bzr merge lp:~mixxxdevelopers/mixxx/features_beatcontrol
Reviewer Review Type Date Requested Status
RJ Skerry-Ryan Approve
Review via email:

Description of the change

This branch introduces the new beatcontrol CO's mentioned on the Wiki page:

To post a comment you must log in.
Revision history for this message
RJ Skerry-Ryan (rryan) wrote :
Download full text (4.5 KiB)

Hey Phil,

looks good -- here are my comments:

* Why add EngineBuffer::prependControl? I don't see it used anywhere. Also, prependControl does not connect all the signals appendControl does so if you keep it please add all the appropriate signal connections.

* Why does EngineBuffer need to keep a pointer to QuantizeControl? Can't it just addControl and be done w/ it?

* All EngineControl's are have a trackLoaded and trackUnloaded slot that is virtual -- instead of manually connecting the reader to the EngineControls, just use those 2 slots. They're automatically hooked up when you addControl a control. (New as of a few nights ago)

* I think a merging mishap happened somewhere. Your diff includes allocation of a 2nd Reader and connecting of its signals. Reader is created and connected in the first few lines of the EngineBuffer constructor now (and should be created before anything else, so plz keep that one).

* In ~LoopingControl -- no need to declare variables separately from their use (it isn't C :P)

* Stuff like this in CueControl and LoopingControl:

+ if ( m_pQuantizeEnabled->get()) {
+ m_iLoopStartSample = m_pQuantizeBeat->get();
+ }
+ else {
+ m_iLoopStartSample = m_iCurrentSample;
+ }

Could probably be tightened up into 1 line with the ternary operator.

* In LoopingControl::LoopingControl():

sizeof(s_dBeatSizes) / sizeof(&s_dBeatSizes[0]))

I think this only works on 64-bit. It should instead be:

sizeof(s_dBeatSizes) / sizeof(s_dBeatSizes[0])

The original statement is dividing the size of the array in bytes by the size of a pointer-type in bytes, while the second statement divides the array size in bytes by the size of a double in bytes. On 64-bit, double and pointer type happens to have the same length. On 32-bit that isn't true.

* LoopingControl::slotUpdatedTrackBeats -- check m_pTrack for NULL before dereferencing it. As far as I can tell, the if statement and return statement do nothing, so please remove. Shouldn't the new Beats signals be connected at this point? Also disconnect the old one's signals if there was an old one otherwise you'll still get its signals.

* LoopingControl::slotTrackLoaded -- check tio before dereferencing it. If tio is invalid, disconnect and clear the current m_pBeats. Currently it looks like Beats are never disconnected from LoopingControl so if you load a track to one deck and then load it to a second deck, then load a new track into the first deck, changes to the original track's beats will affect both decks.

* LoopingControl::slotBeatLoop -- should probably add initializers to variables, otherwise they have random data as their value.

* LoopingControl::slotBeatLoop -- We tenuously follow the Google C++ Style Guide (OK, I do for the most part.. and on some days Albert does too.. etc). I know your preferred if statement style is:
  if ( condition )

  I don't really care that much, as long as you keep it consistent (I wouldn't say anything otherwise) But the diff for this branch has all three of:

  if (condition)
  if ( condition)
  if ( condition )

  IMO if you're going to use a spacing and brace style that's different from the ...


review: Needs Fixing
2553. By Phillip Whelan

Remove prependControl and refactor appendControl to addControl (reverts a change from trunk).

2554. By Phillip Whelan

Remove the redundant construction of CachingReader in EngineBuffer. Refactor LoopingControl and QuantizeControl to let addContorl hook up the track loaded signals.

2555. By Phillip Whelan

Use ternary operator to make the syntax for quantization simpler.

2556. By Phillip Whelan

FIX: correctly calculate the sizeof for the beat sizes (for creating the beatloop_X CO's).

2557. By Phillip Whelan

FIX: give default values for loop_in and loop_out in slotBeatLoop.

2558. By Phillip Whelan

Merging from upstream.

2559. By Phillip Whelan

Improve slotBeatLoop, the function which generates beat loops, to handle numbers with decimals that are larger than 1 or lesser than -1.

2560. By Phillip Whelan

FIX: correctly handle loops with decimals, finally.

Revision history for this message
RJ Skerry-Ryan (rryan) wrote :

looks good to me -- merge away!

review: Approve
2561. By Phillip Whelan

Merging from trunk before merging into it.

Preview Diff



People subscribed via source and target branches