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

features_looping contains the looping feature that is a target for Mixxx 1.8 development. The feature is now stable within the branch, and the outline and outlineNetbook skins have been updated to support the new feature. Additionally, this branch rewrites the old Mixxx EngineBuffer and Reader to be more robust and organized. It has received a good deal of testing via Phil, since he relies on it for his pitchbend branch. It builds fine on Windows, Mac and Linux. In my estimation it is ready for merging into trunk, since there is no more feature work to be done on it as per the original feature specification.

enginebuffer.cpp:383
        // TODO(rryan) : review this touch sensitive business with the Mixxx
        // team to see if this is what we actually want.
===
I guess we're just going to take the "see if anyone complains that we broke something" approach for that one? I don't think either of us has a controller that uses that touch wheelie thing.

enginebufferscalelinear.cpp:151
(not quoting the line here, read it and you'll understand :))
===
Is that experiment still relevant?
Can you add some more comments to the loops in scale()?

loopingcontrol.cpp
===
Can you please comment getTrigger and nextTrigger? Should we keep those qDebugs in there?

ratecontrol.cpp:102
    // FIXME: This should be dependent on sample rate/block size or something
    m_pJogFilter->setFilterLength(5);
===
Yeah, this looks really awkward to fix. All of these accumulators (Rotary.cpp?) are dependent on the block size. I wonder if we should just hack some static setBufferSize() function into Rotary...
Can you write a short paragraph describing what RateControl does at the top of ratecontrol.cpp, just so the next guys that come along know what it does and why we need it?

readaheadmanager.cpp
===
Can you please write a description of what this class does and why at the top of the file? I also think the functions in this file need comments above them (eg. what getSoonestTrigger() is doing doesn't look super obvious to the untrained eye). :)

cachingreader.cpp
===
Lines 15-20: You talk a lot about how to calculate the chunk length, but you didn't define what a "chunk" is. Can you explain it (for the next guys that come along)?
This file also wouldn't hurt from a general explanation at the top...

Lines 69-78: I think you might need a Q_ASSERT in there making sure kChunkLength is less than memory_to_use...

Line 89: Toss some comments in here explaining how the initialization of your m_chunks and m_freeChunks lists work.

Can you add comments above the functions in this file too?

cachingreader.h
===
Please comment the Hint and Chunk structs including their data fields.

Quick note on commenting -- I usually opt to comment the .h file because
it is the specification of the class. Any comments I put in the .cpp are
implementation specific. So wherever you asked for comments on the
functions, I always put them in the .h.

I addressed all your comments below:

Albert Santoni wrote:
> Review: Needs Fixing
> enginebuffer.cpp:383
> // TODO(rryan) : review this touch sensitive business with the Mixxx
> // team to see if this is what we actually want.
> ===
> I guess we're just going to take the "see if anyone complains that we broke something" approach for that one? I don't think either of us has a controller that uses that touch wheelie thing.
>
Yea -- the xponent mappings (and another) use this. It needs reworking,
but for now its just going to stay in EngineBuffer. Once PITS/VE is
switchable via a CO, it could easily be managed by an EngineControl.
>
> enginebufferscalelinear.cpp:151
> (not quoting the line here, read it and you'll understand :))
> ===
> Is that experiment still relevant?
> Can you add some more comments to the loops in scale()?
>
>
No it's not really still relevant.
> loopingcontrol.cpp
> ===
> Can you please comment getTrigger and nextTrigger? Should we keep those qDebugs in there?
>
>
Done
> ratecontrol.cpp:102
> // FIXME: This should be dependent on sample rate/block size or something
> m_pJogFilter->setFilterLength(5);
> ===
> Yeah, this looks really awkward to fix. All of these accumulators (Rotary.cpp?) are dependent on the block size. I wonder if we should just hack some static setBufferSize() function into Rotary...
> Can you write a short paragraph describing what RateControl does at the top of ratecontrol.cpp, just so the next guys that come along know what it does and why we need it?
>
>
Done
> readaheadmanager.cpp
> ===
> Can you please write a description of what this class does and why at the top of the file? I also think the functions in this file need comments above them (eg. what getSoonestTrigger() is doing doesn't look super obvious to the untrained eye). :)
>
>
Done (getSoonestTrigger is broken and unused -- I marked it as such)
> cachingreader.cpp
> ===
> Lines 15-20: You talk a lot about how to calculate the chunk length, but you didn't define what a "chunk" is. Can you explain it (for the next guys that come along)?
> This file also wouldn't hurt from a general explanation at the top...
>
>
Done
> Lines 69-78: I think you might need a Q_ASSERT in there making sure kChunkLength is less than memory_to_use...
>
>
Good call :)
> Line 89: Toss some comments in here explaining how the initialization of your m_chunks and m_freeChunks lists work.
>
>
Done
> Can you add comments above the functions in this file too?
>
>
>
Done
> cachingreader.h
> ===
> Please comment the Hint and Chunk structs including their data fields.
>
>
Done

Looks great RJ! Please merge into trunk whenever you can.

Thanks again for all your hard work! You're going to make a lot of DJs happy with looping. :)
Albert