> Another resubmit.
> 
> > - Passing references to m_scriptFileNames and m_scriptFunctionPrefixes is
> not
> >   good.
> 
> Fixed.
> 
> >   I say we just accept one
> >   signal for executing a script function: callMidiScriptFunction(QString
> name,
> >   QList<QVariant> arguments) and consolidate all the execute(...) methods
> into
> >   one execute(QString function, QList<QVariant> arguments); method.
> 
> I agree, though right now the only thing that signals the MidiScriptEngine is
> MidiMapping and the existing two signals are all it needs. So I'm thinking we
> should defer this consolidation unless/until it ever becomes an issue. (Also,
> can you even send null QLists for situations where we want to execute a
> parameter-less function or a non-function piece of script code?)
> 
> > MidiScriptEngine::gracefulShutdown()
> > - Need to hold the engine lock before calling m_connectedControls.clear()
> 
> Fixed.
> 
> > MidiScriptEngine::loadScriptFile()
> > - As I mentioned, convert this to take m_rScriptFileNames as an argument
> > instead of sharing it between the classes.
> 
> Done, works fine.
> 
> > MidiScriptEngine::stopAllTimers()
> > - Please add comment to the top that the caller should hold the script lock.
> 
> Did you one better and asserted that the lock is held (kind of redundant since
> stopTimers() also checks, but whatever.)
> 
> > MidiScriptEngine::timerEvent()
> > - Need to hold the lock to user m_timers.
> 
> Fixed.
> 
> > - qWarnings and qCritical - I really don't think these should anywhere in
> the
> >   script engine. Maybe it's ok for a script function that only happens on
> >   initialization -- but in the end no DJ cares about a syntax error more
> than
> >   s/he cares about Mixxx not going down in the middle of their set.
> 
> Agreed. I examined the various things that were qCriticals and discovered that
> QScriptEngine::canEvaluate() has been obsoleted. So those qCriticals have been
> removed. I added the checkSyntax() system in their place and I have it pop up
> qCriticals with the error details only if --midiDebug is enabled, otherwise it
> qDebugs the details and pops up a friendly qWarning which the user can
> acknowledge and continue to mix (though with the controller in a degraded
> state.) As for loops, only the first warning dialog comes up; all others are
> supressed until the first is acknowledged. (So the user will see one each time
> he does something that calls an offending function.) However, if the syntax
> error is discovered on file load, that warning dialog shows up only once (each
> time the script file is (re)loaded) and informs the user that the offending
> script will be disabled.
> 
> One thing to note about allowing the user to continue on a script error: the
> offending function aborts on the statement that caused the exception, so the
> rest of the function doesn't execute and could leave the controller and script
> global variables in an indeterminate state (for example a mode change could
> have happened but the LEDs didn't get updated to show it.)

Sounds like you should give an option to reload the script when there's an exception. I'd rather have my controller lose whatever mode it's in and reset than stop me from using it completely in the middle of my set.