Merge lp://staging/~mixxxdevelopers/mixxx/features_direct_bind into lp://staging/~mixxxdevelopers/mixxx/trunk

The runtime evaluation option and file reloading are in the works for after this is merged.

Thanks! -- here are my comments sofar:

* ControllerEngine::callFunctionOnObjects should check isFunction() before call()'ing the QScriptValue

* ControllerEngine::resolveFunction should check isFunction() in addition to isValid()

* In ControllerEngine::internalExecute(QScriptValue, QScriptValue) return false if !isFunction() to match execute(QScriptValue, QScriptValueList)

* In ControllerEngine you changed a bunch of debug outputs to say MidiScriptEngine instead of ControllerEngine -- please change those back :)

* In connectControl() in the first if(disconnect) path you set cb.key = key which seems redundant with a few lines above.

* ControllerEngineConnectionScriptValue::disconnect() shadows QObject::disconnect(). We probably shouldn't do that.

* connectControl() connects the valueChanged() signal from the ControlObject, not the ControlObjectThread which causes priority-inversion between the Engine and the Qt main-thread. This was a bug that was fixed in the control-system rewrite so please re-add that.

* MidiController::bindScriptFunction() calling preset() and casting the result -- that's for use by the superclass which doesn't know about MidiControllerPresets. You can just use m_preset. (though see my comment below)

* I think the m_scriptBindings cache in Controller/MidiController isn't really necessary. Instead of the cache living in MixxxControl/Controller and having to pre-cache every function we think we might want to call, why not put the cache in ControllerEngine::resolveFunction. Change ControllerEngine::resolveFunction(QString) to take a boolean shouldCache flag that tells it to first look in the QScriptValue cache and then just call resolveFunction() every time we want to call a method. I think this would be a lot cleaner and this branch would become almost totally contained to ControllerEngine and it would be just as fast as what is written now (and a little more robust to cache misses since it will cache on first use).

* thanks for the tests :)

Any update Phil? If you're too hosed I can make these changes myself --
it'd be great to get these in 1.11.

On Fri, May 11, 2012 at 2:14 PM, RJ Ryan <email address hidden> wrote:

> Review: Needs Fixing
>
> Thanks! -- here are my comments sofar:
>
> * ControllerEngine::callFunctionOnObjects should check isFunction() before
> call()'ing the QScriptValue
>
> * ControllerEngine::resolveFunction should check isFunction() in addition
> to isValid()
>
> * In ControllerEngine::internalExecute(QScriptValue, QScriptValue) return
> false if !isFunction() to match execute(QScriptValue, QScriptValueList)
>
> * In ControllerEngine you changed a bunch of debug outputs to say
> MidiScriptEngine instead of ControllerEngine -- please change those back :)
>
> * In connectControl() in the first if(disconnect) path you set cb.key =
> key which seems redundant with a few lines above.
>
> * ControllerEngineConnectionScriptValue::disconnect() shadows
> QObject::disconnect(). We probably shouldn't do that.
>
> * connectControl() connects the valueChanged() signal from the
> ControlObject, not the ControlObjectThread which causes priority-inversion
> between the Engine and the Qt main-thread. This was a bug that was fixed in
> the control-system rewrite so please re-add that.
>
> * MidiController::bindScriptFunction() calling preset() and casting the
> result -- that's for use by the superclass which doesn't know about
> MidiControllerPresets. You can just use m_preset. (though see my comment
> below)
>
> * I think the m_scriptBindings cache in Controller/MidiController isn't
> really necessary. Instead of the cache living in MixxxControl/Controller
> and having to pre-cache every function we think we might want to call, why
> not put the cache in ControllerEngine::resolveFunction. Change
> ControllerEngine::resolveFunction(QString) to take a boolean shouldCache
> flag that tells it to first look in the QScriptValue cache and then just
> call resolveFunction() every time we want to call a method. I think this
> would be a lot cleaner and this branch would become almost totally
> contained to ControllerEngine and it would be just as fast as what is
> written now (and a little more robust to cache misses since it will cache
> on first use).
>
> * thanks for the tests :)
> --
>
> https://code.launchpad.net/~mixxxdevelopers/mixxx/features_direct_bind/+merge/104529
> You are reviewing the proposed merge of
> lp:~mixxxdevelopers/mixxx/features_direct_bind into lp:mixxx.
>

I'll see if I can get to them tomorrow. Been a bit busy lately with RL.