Merge lp://qastaging/~mixxxdevelopers/mixxx/features_controllerAbstraction into lp://qastaging/~mixxxdevelopers/mixxx/trunk

Maybe I missed the discussion on this but while I agree that the GUI mapping support totally sucks do we really want to remove it without replacing it?

For 1.11 I think we're just going to remove it. The general reasoning is
that since we haven't really had anyone complaining about how hard it is to
use like about 2 years of it being there.. the only conclusion must be that
close to nobody uses it.

Code-wise it was very complicated too and really screwed up the MIDI
mapping implementation since they were deeply intertwined. I'd rather nuke
it in 1.11 and come up with a really nice one for 1.12 or create an
external mapping program.

On Sun, Mar 18, 2012 at 9:26 PM, Bill Good <email address hidden> wrote:

> Maybe I missed the discussion on this but while I agree that the GUI
> mapping support totally sucks do we really want to remove it without
> replacing it?
> --
>
> https://code.launchpad.net/~mixxxdevelopers/mixxx/features_controllerAbstraction/+merge/98121
> You proposed lp:~mixxxdevelopers/mixxx/features_controllerAbstraction for
> merging.
>

We could add back the MIDI mapping wizard if you like, since there's already a message signal and a learning flag in place in MidiController. MC just needs a slot to get data from the wizard, then the wizard would need to be adjusted to work with these signals/slots. I'll check into this further.

Hey Sean,

I realize you haven't put the finishing touches on the branch yet. I'm sorry in advance because this review is going to seem a little brutal but I'm convinced that we need to do things right. The main problem I saw with the MIDI subsystem in 1.10 is that everything was totally commingled. This made it impossible to write unit tests for a given class because we had swiss-army-knife classes that did everything. We need to move away from this.

I want to get you this feedback as soon as possible, so here it is. I may have more comments to share later.

* Please make sure to remove commented code referring to non-existing stuff (e.g. all commented lines you left in build/depends.py that refer to old files that don't exist anymore)

* Make sure to delete "res/controllers/Stanton SCS.1m VUMETERS ONLY.xml"

* Please delete src/controllers/OLDmidi soon because it makes reviewing your branch hard (the diff is gigantic).

* In defs_controllers.h, make helper functions instead of these macros. (DEFAULT_DEVICE_PRESET, DEFAULT_MIDI_DEVICE_PRESET in particular. Ideally PRESETS_PATH and LPRESETS_PATH as well).

* Please add "virtual" to every destructor declaration.

* It's good that the old MidiMapping class died because it was a mutable-shared-state-logic-monster. That said, storing the mappings in the controller is almost as bad.
   - Let's pull all mappings out into a Mapping/MidiMapping class. This class should be plain-old-data, not a QObject (e.g. no signals/slots) and no methods except the minimum needed for Controller to look up the script files, MidiController to look up a mapped control in receive() and to look up output mappings where it creates the MidiOutputHandlers.

* Take all serialization/deserialization code and put it into a MappingParser/MidiMappingParser helper class which creates a Mapping/MidiMapping class from a given path.

* Delete loadPreset / savePreset / buildDomElement methods in Controller/MidiController .. replace them with a setMapping() command. A controller that doesn't have a mapping yet can be considered invalid.

* Controllers shouldn't be concerned with saving a preset. Instead that should be the controller preferences dialog or the ControllerManager's job to deserialize the mapping and call Controller::setMapping().

* Need to remove all methods that deal with a char*/length data buffer e.g. Controller::send, Controller::receivePointer. Instead we should be using QByteArray for everything.

* Remove length argument from Controller::sendBa since a QByteArray knows its own length. Also, rename sendBa to send(). Also make the argument a const QByteArray.

* I'd prefer we get rid of the Controller::send(QList<int>) method and instead require people to use QByteArrays but I'm not sure if this is easy from scripts so maybe we can let it be. Please change the implementation of Controller::send(QList<int>, int) to build a QByteArray and call send(QByteArray).

* In ControllerManager, please remove the m_pPMEnumerator, m_pHSSEnumerator, m_pHIDEnumerator, and m_pOSCEnumerator declarations. Instead use a QList of ControllerEnumerator* objects. Add an "addControllerEnumerator()" method to ControllerManager, an...

> * Please make sure to remove commented code referring to non-existing stuff
> * Make sure to delete "res/controllers/Stanton SCS.1m VUMETERS ONLY.xml"
> * Please delete src/controllers/OLDmidi soon because it makes reviewing your

These are among the reasons why I didn't propose a merge yet, silly. :)

> * In defs_controllers.h, make helper functions instead of these macros.

What benefit would that give? The macros are straightforward and fast.

> * Please add "virtual" to every destructor declaration.

As a matter of course? What will that do? I only do it to destructors that will be reimplemented.

> shared-state-logic-monster. That said, storing the mappings in the controller
> is almost as bad.

I do this because I view a Controller as a device, with all properties particular to that device (e.g. a mapping) contained in the one object. Then due to the time-sensitive nature of MIDI processing, I imagine local object data is faster to access.

> * Take all serialization/deserialization code and put it into a
> MappingParser/MidiMappingParser helper class which creates a
> Mapping/MidiMapping class from a given path.

That sounds like overkill. If we do in fact make a MidiMapping class, shouldn't the same class that holds the data be responsible for all operations on that data?

> * Need to remove all methods that deal with a char*/length data buffer e.g.
> Controller::send, Controller::receivePointer. Instead we should be using
> QByteArray for everything.

Not all of them can be removed without duplicating code or complicating the classes, since the device APIs send and expect data this way. If we try, know that interfacing raw bytes between JS and C++ is a very delicate issue. I've had numerous problems with bytes getting mangled in the past.
Plus I imagine primitive data types to be slightly faster. Here again, what would be the benefit of switching to QByteArrays?

> * I'd prefer we get rid of the Controller::send(QList<int>) method and instead
> require people to use QByteArrays

We can't. This is how QtScript passes data byte arrays to C++.

> * In ControllerManager, please remove the m_pPMEnumerator, m_pHSSEnumerator,
> m_pHIDEnumerator, and m_pOSCEnumerator declarations.

Thanks for the suggested methods. This was bothering me too but I couldn't think of a neat way to fix it.

> * midi-mappings-scripts.js You're changing the names of a lot of common script
> stuff. Won't this break MIDI-scripts in the wild?

Yes, but the only things that will be incompatible are MidiButtonState and MidiLedState. A README telling to search-n-replace will fix them right up. I left backwards-compatible calls for everything else.

We should think long and hard before breaking people's scripts, even if a simple search-and-replace will fix it. I'd prefer to leave scripts stable and only limit breakage to major releases (2.0, etc). I'm not super-familiar with this branch, but is this breakage necessary?

On Mon, Mar 19, 2012 at 4:14 PM, Sean M. Pappalardo
<email address hidden>wrote:

> > * Please make sure to remove commented code referring to non-existing
> stuff
> > * Make sure to delete "res/controllers/Stanton SCS.1m VUMETERS ONLY.xml"
> > * Please delete src/controllers/OLDmidi soon because it makes reviewing
> your
>
> These are among the reasons why I didn't propose a merge yet, silly. :)
>
> > * In defs_controllers.h, make helper functions instead of these macros.
>
> What benefit would that give? The macros are straightforward and fast.
>

Macros should only be used for constants. They're no faster than an inline
function. These particular macros refer to member variables of a class and
have logic in them. This obscures the logic within the class they are used
in because it is hidden behind what looks like a constant. If they have
member variables then they're specific to a class so why not make them a
private inline helper function of that class?

>
> > * Please add "virtual" to every destructor declaration.
>
> As a matter of course? What will that do? I only do it to destructors that
> will be reimplemented.
>
>
It's just something that every class should have. It should be as automatic
as making your member variables private. Some day in the future someone
will subclass your class -- guaranteed. Be nice to them -- it might even be
your future self. :)

> > shared-state-logic-monster. That said, storing the mappings in the
> controller
> > is almost as bad.
>
> I do this because I view a Controller as a device, with all properties
> particular to that device (e.g. a mapping) contained in the one object.
> Then due to the time-sensitive nature of MIDI processing, I imagine local
> object data is faster to access.
>
>
It's fine for the controller to have a mapping. That mapping should be
encapsulated into one class so you can pass them around and vary the
implementation of the mapping independent of the class that consumes the
mapping (the Controller).

Accessing a member variable versus the member variable of another class is
possibly the difference in 1 or two assembly instructions -- if any
difference. If you care about this in the name of speed then I'm surprised
you use so many virtual functions (they are about 2x slower than a regular
function call). This sort of difference is totally meaningless from a Mixxx
performance perspective. We're talking picoseconds here!

Don't let tens or hundreds of assembly instructions get in the way of
writing good code. For the long-term health of the project it's worth it to
structure the project in small, bite-sized and testable classes. This is
extra work, I know.

> > * Take all serialization/deserialization code and put it into a
> > MappingParser/MidiMappingParser helper class which creates a
> > Mapping/MidiMapping class from a given path.
>
> That sounds like overkill. If we do in fact make a MidiMapping class,
> shouldn't the same class that holds the data be responsible for all
> operations on that data?
>

The mapping parsing / serializing code is long enough to warrant living in
its own utility class. It is long and obscures the purpose of whatever
class it lives in (origin...

> Macros should only be used for constants.

My bad, I forgot there were member variables in two of them. Don't know what I was thinking when I did that.

> difference. If you care about this in the name of speed then I'm surprised
> you use so many virtual functions (they are about 2x slower than a regular
> function call).

:O I had no idea! Is that 2x slowdown a matter of picoseconds too?

> The mapping parsing / serializing code is long enough to warrant living in
> its own utility class. It is long and obscures the purpose of whatever
> class it lives in (originally MidiMapping, now *Controller).

What bothers me about separating it is that there is no generic ControllerMapping class (at least not yet,) so MidiMappings are specific to MIDI controllers, so why not make them an integral part? I see your point though and will make the change.

> 1.10. Can we keep the ButtonState and LedState as they are for MIDI and
> mark them deprecated?

Actually, leaving them without the "Midi" prefix should be fine, since non-MIDI controllers will need to set the values differently anyway.

LOL, I removed the OLDmidi directory and the diff got bigger. (Not surprised.)

RJ, while I work on the rest, I need you to take a look at ControllerEngine::connectControl() and figure out why using COTs breaks automatic reactions. They work fine in trunk so I have no idea.

Hello again. Looking at your class-separation requests more closely now, I still have reservations:

> - Let's pull all mappings out into a Mapping/MidiMapping class. This class
> should be plain-old-data, not a QObject (e.g. no signals/slots) and no methods

I'm sorry, I can't see the benefit of doing that. It would just add complexity and make it harder for *Controller to be expanded in the future such as when the GUI signals it to add/delete/change mappings. (Besides, a preset is just two QLists. Specific *Controllers add two QHashes for mappings.)

> * Take all serialization/deserialization code and put it into a
> MappingParser/MidiMappingParser helper class

Here too, moving those functions to another class would add needless complexity because they have to access data that's specific to a Controller. I understand the desire to have classes not be too long, but don't let that get in the way of logical design. In this case, everything that's in *Controller is specific to that type of device. (Yes, that includes the parsing code, since it's specific to the mapping storage format, which is specific to the device type.) Trying to separate it across multiple classes would cause us to have shared-state logic monsters again and make it much more difficult to follow for those new to the code. Since it sounds like you're interested in visual separation, I have moved the functions that handle XML into separate .cpp files and have simply #included these in the main *Controller ones.

> * Controllers shouldn't be concerned with saving a preset. Instead that should
> be the controller preferences dialog or the ControllerManager's job

The ControllerManager _is_ in charge of saving presets. The Controllers just contain the code to do so, because it's specific to each Controller type (as stated above,) and the Manager isn't concerned with those details (or it would be micro-managing.)

In short, there's nothing else that can be factored out.

If your concerns are motivated by unit-testing issues, why can't we just write separate tests for different aspects of a class?

On Apr 7, 2012 7:38 AM, "Sean M. Pappalardo" <email address hidden> wrote:
>
> Hello again. Looking at your class-separation requests more closely now,
I still have reservations:
>
> > - Let's pull all mappings out into a Mapping/MidiMapping class. This
class
> > should be plain-old-data, not a QObject (e.g. no signals/slots) and no
methods
>
> I'm sorry, I can't see the benefit of doing that. It would just add
complexity and make it harder for *Controller to be expanded in the future
such as when the GUI signals it to add/delete/change mappings. (Besides, a
preset is just two QLists. Specific *Controllers add two QHashes for
mappings.)

The benefit is encapsulation of the mapping implementation.

Add/delete/change of a mapping from the GUI is the reason the old midi
system is so terrible. The only way to load a mapping going forward will be
to call Controller::loadMapping(ControllerMapping).

>
> > * Take all serialization/deserialization code and put it into a
> > MappingParser/MidiMappingParser helper class
>
> Here too, moving those functions to another class would add needless
complexity because they have to access data that's specific to a
Controller. I understand the desire to have classes not be too long, but
don't let that get in the way of logical design. In this case, everything
that's in *Controller is specific to that type of device. (Yes, that
includes the parsing code, since it's specific to the mapping storage
format, which is specific to the device type.) Trying to separate it across
multiple classes would cause us to have shared-state logic monsters again
and make it much more difficult to follow for those new to the code. Since
it sounds like you're interested in visual separation, I have moved the
functions that handle XML into separate .cpp files and have simply
#included these in the main *Controller ones.

Putting this in a separate helper will allow us to test it and it will
simplify the ControllerMapping class considerably.

>
> > * Controllers shouldn't be concerned with saving a preset. Instead that
should
> > be the controller preferences dialog or the ControllerManager's job
>
> The ControllerManager _is_ in charge of saving presets. The Controllers
just contain the code to do so, because it's specific to each Controller
type (as stated above,) and the Manager isn't concerned with those details
(or it would be micro-managi

I think saving presets should be owned by the controller preferences dialog
and not even be part of the controller manager actually.

>
> In short, there's nothing else that can be factored out.
>
> If your concerns are motivated by unit-testing issues, why can't we just
write separate tests for different aspects of a class?

Well the general reason why this is is that you can't isolate the logic you
aim to test when it's all mixed together in one class.

I feel bad coming down hard but these issues are non-negotiable. In the
past I haven't held the MIDI subsystem to as high a standard as say the
engine but now that you are rewriting it I'd like to change that. "It
works" is not good enough for infrastructure code (which this is going to
be more than ever as we add more controller types) so...