Improve Controller Script Error Reporting

src/controllers/controller.cpp

@@ -68,7 +68,7 @@ bool Controller::applyPreset(bool initializeScripts) {
     }
 
     bool success = m_pEngine->loadScriptFiles(scriptFiles);
-    if (initializeScripts) {
+    if (success && initializeScripts) {
         m_pEngine->initializeScripts(scriptFiles);
     }
     return success;

src/controllers/controllerengine.cpp

@@ -34,7 +34,7 @@ ControllerEngine::ControllerEngine(
         : m_pEngine(nullptr),
           m_pController(controller),
           m_pConfig(pConfig),
-          m_bPopups(false),
+          m_bPopups(true),
           m_pBaClass(nullptr) {
     // Handle error dialog buttons
     qRegisterMetaType<QMessageBox::StandardButton>("QMessageBox::StandardButton");
@@ -66,13 +66,7 @@ ControllerEngine::~ControllerEngine() {
         m_scratchFilters[i] = nullptr;
     }
 
-    // Delete the script engine, first clearing the pointer so that
-    // other threads will not get the dead pointer after we delete it.
-    if (m_pEngine != nullptr) {
-        QScriptEngine *engine = m_pEngine;
-        m_pEngine = nullptr;
-        engine->deleteLater();
-    }
+    uninitializeScriptEngine();
 }
 
 /* -------- ------------------------------------------------------
@@ -82,6 +76,10 @@ Output:  -
 -------- ------------------------------------------------------ */
 void ControllerEngine::callFunctionOnObjects(QList<QString> scriptFunctionPrefixes,
                                              const QString& function, QScriptValueList args) {
+    if (m_pEngine == nullptr) {
+        return;
+    }
+
     const QScriptValue global = m_pEngine->globalObject();
 
     for (const QString& prefixName : scriptFunctionPrefixes) {
@@ -113,6 +111,10 @@ Output:  QScriptValue of JS snippet wrapped in an anonymous function
 ------------------------------------------------------------------- */
 QScriptValue ControllerEngine::wrapFunctionCode(const QString& codeSnippet,
                                                 int numberOfArgs) {
+    VERIFY_OR_DEBUG_ASSERT(m_pEngine != nullptr) {
+        return QScriptValue();
+    }
+
     QScriptValue wrappedFunction;
 
     auto i = m_scriptWrappedFunctionCache.constFind(codeSnippet);
@@ -134,6 +136,10 @@ QScriptValue ControllerEngine::wrapFunctionCode(const QString& codeSnippet,
 }
 
 QScriptValue ControllerEngine::getThisObjectInFunctionCall() {
+    VERIFY_OR_DEBUG_ASSERT(m_pEngine != nullptr) {
+        return QScriptValue();
+    }
+
     QScriptContext *ctxt = m_pEngine->currentContext();
     // Our current context is a function call. We want to grab the 'this'
     // from the caller's context, so we walk up the stack.
@@ -150,6 +156,10 @@ Input:   -
 Output:  -
 -------- ------------------------------------------------------ */
 void ControllerEngine::gracefulShutdown() {
+    if (m_pEngine == nullptr) {
+        return;
+    }
+
     qDebug() << "ControllerEngine shutting down...";
 
     // Stop all timers
@@ -222,6 +232,16 @@ void ControllerEngine::initializeScriptEngine() {
     engineGlobalObject.setProperty("ByteArray", m_pBaClass->constructor());
 }
 
+void ControllerEngine::uninitializeScriptEngine() {
+    // Delete the script engine, first clearing the pointer so that
+    // other threads will not get the dead pointer after we delete it.
+    if (m_pEngine != nullptr) {
+        QScriptEngine* engine = m_pEngine;
+        m_pEngine = nullptr;
+        engine->deleteLater();
+    }
+}
+
 /* -------- ------------------------------------------------------
    Purpose: Load all script files given in the supplied list
    Input:   List of script paths and file names to load
@@ -244,9 +264,15 @@ bool ControllerEngine::loadScriptFiles(const QList<ControllerPreset::ScriptFileI
     connect(&m_scriptWatcher, SIGNAL(fileChanged(QString)),
             this, SLOT(scriptHasChanged(QString)));
 
-    emit initialized();
+    bool success = result && m_scriptErrors.isEmpty();
+    if (!success) {
+        gracefulShutdown();
+        uninitializeScriptEngine();
+    } else {
+        emit initialized();
+    }
 
-    return result && m_scriptErrors.isEmpty();
+    return success;
 }
 
 // Slot to run when a script file has changed
@@ -259,17 +285,12 @@ void ControllerEngine::scriptHasChanged(const QString& scriptFilename) {
                this, SLOT(scriptHasChanged(QString)));
 
     gracefulShutdown();
-
-    // Delete the script engine, first clearing the pointer so that
-    // other threads will not get the dead pointer after we delete it.
-    if (m_pEngine != nullptr) {
-        QScriptEngine *engine = m_pEngine;
-        m_pEngine = nullptr;
-        engine->deleteLater();
-    }
+    uninitializeScriptEngine();
 
     initializeScriptEngine();
-    loadScriptFiles(m_lastScriptFiles);
+    if (!loadScriptFiles(m_lastScriptFiles)) {
+        return;
+    }
 
     qDebug() << "Re-initializing scripts";
     initializeScripts(m_lastScriptFiles);
@@ -311,31 +332,41 @@ bool ControllerEngine::evaluate(const QString& filepath) {
     return evaluate(QFileInfo(filepath));
 }
 
-bool ControllerEngine::syntaxIsValid(const QString& scriptCode) {
+bool ControllerEngine::syntaxIsValid(const QString& scriptCode, const QString& filename) {
     if (m_pEngine == nullptr) {
         return false;
     }
 
     QScriptSyntaxCheckResult result = m_pEngine->checkSyntax(scriptCode);
-    QString error = "";
+    QString error;
     switch (result.state()) {
         case (QScriptSyntaxCheckResult::Valid): break;
         case (QScriptSyntaxCheckResult::Intermediate):
-            error = "Incomplete code";
+            error = tr("Incomplete code");
             break;
         case (QScriptSyntaxCheckResult::Error):
-            error = "Syntax error";
+            error = tr("Syntax error");
             break;
     }
-    if (error!="") {
-        error = QString("%1: %2 at line %3, column %4 of script code:\n%5\n")
-                .arg(error,
-                     result.errorMessage(),
-                     QString::number(result.errorLineNumber()),
-                     QString::number(result.errorColumnNumber()),
-                     scriptCode);
+    if (!error.isEmpty()) {
+        if (filename.isEmpty()) {
+            error = QString(tr("%1 at line %2, column %3: %4"))
+                            .arg(error,
+                                    QString::number(result.errorLineNumber()),
+                                    QString::number(result.errorColumnNumber()),
+                                    result.errorMessage()) +
+                    QStringLiteral("\n\n") + tr("Code:") + QStringLiteral("\n") + scriptCode;
+        } else {
+            error = QString(tr("%1 at line %2, column %3 in file %4: %5"))
+                            .arg(error,
+                                    QString::number(result.errorLineNumber()),
+                                    QString::number(result.errorColumnNumber()),
+                                    filename,
+                                    result.errorMessage());
+        }
 
-        scriptErrorDialog(error);
+        qWarning() << "ControllerEngine:" << error;
+        scriptErrorDialog(error, true);
         return false;
     }
     return true;
@@ -346,8 +377,8 @@ Purpose: Evaluate & run script code
 Input:   'this' object if applicable, Code string
 Output:  false if an exception
 -------- ------------------------------------------------------ */
-bool ControllerEngine::internalExecute(QScriptValue thisObject,
-                                       const QString& scriptCode) {
+bool ControllerEngine::internalExecute(
+        QScriptValue thisObject, const QString& scriptCode) {
     // A special version of safeExecute since we're evaluating strings, not actual functions
     //  (execute() would print an error that it's not a function every time a timer fires.)
     if (m_pEngine == nullptr) {
@@ -378,8 +409,9 @@ Purpose: Evaluate & run script code
 Input:   'this' object if applicable, Code string
 Output:  false if an exception
 -------- ------------------------------------------------------ */
-bool ControllerEngine::internalExecute(QScriptValue thisObject, QScriptValue functionObject,
-                                       QScriptValueList args) {
+bool ControllerEngine::internalExecute(QScriptValue thisObject,
+        QScriptValue functionObject,
+        QScriptValueList args) {
     if (m_pEngine == nullptr) {
         qDebug() << "ControllerEngine::execute: No script engine exists!";
         return false;
@@ -394,8 +426,7 @@ bool ControllerEngine::internalExecute(QScriptValue thisObject, QScriptValue fun
     // If it's not a function, we're done.
     if (!functionObject.isFunction()) {
         qDebug() << "ControllerEngine::internalExecute:"
-                 << functionObject.toVariant()
-                 << "Not a function";
+                 << functionObject.toVariant() << "Not a function";
         return false;
     }
 
@@ -410,12 +441,12 @@ bool ControllerEngine::internalExecute(QScriptValue thisObject, QScriptValue fun
 }
 
 bool ControllerEngine::execute(QScriptValue functionObject,
-                               unsigned char channel,
-                               unsigned char control,
-                               unsigned char value,
-                               unsigned char status,
-                               const QString& group,
-                               mixxx::Duration timestamp) {
+        unsigned char channel,
+        unsigned char control,
+        unsigned char value,
+        unsigned char status,
+        const QString& group,
+        mixxx::Duration timestamp) {
     Q_UNUSED(timestamp);
     if (m_pEngine == nullptr) {
         return false;
@@ -429,8 +460,9 @@ bool ControllerEngine::execute(QScriptValue functionObject,
     return internalExecute(m_pEngine->globalObject(), functionObject, args);
 }
 
-bool ControllerEngine::execute(QScriptValue function, const QByteArray data,
-                               mixxx::Duration timestamp) {
+bool ControllerEngine::execute(QScriptValue function,
+        const QByteArray data,
+        mixxx::Duration timestamp) {
     Q_UNUSED(timestamp);
     if (m_pEngine == nullptr) {
         return false;
@@ -454,25 +486,28 @@ bool ControllerEngine::checkException() {
     if (m_pEngine->hasUncaughtException()) {
         QScriptValue exception = m_pEngine->uncaughtException();
         QString errorMessage = exception.toString();
-        QString line = QString::number(m_pEngine->uncaughtExceptionLineNumber());
-        QStringList backtrace = m_pEngine->uncaughtExceptionBacktrace();
+        QString line =
+                QString::number(m_pEngine->uncaughtExceptionLineNumber());
         QString filename = exception.property("fileName").toString();
 
         QStringList error;
         error << (filename.isEmpty() ? "" : filename) << errorMessage << line;
-        m_scriptErrors.insert((filename.isEmpty() ? "passed code" : filename), error);
+        m_scriptErrors.insert(
+                (filename.isEmpty() ? "passed code" : filename), error);
 
-        QString errorText = tr("Uncaught exception at line %1 in file %2: %3")
-                .arg(line, (filename.isEmpty() ? "" : filename), errorMessage);
-
-        if (filename.isEmpty())
-            errorText = tr("Uncaught exception at line %1 in passed code: %2")
-                    .arg(line, errorMessage);
+        QString errorText;
+        if (filename.isEmpty()) {
+            errorText = tr("Uncaught exception at line %1 in passed code.").arg(line);
+        } else {
+            errorText = tr("Uncaught exception at line %1 in file %2.").arg(line, filename);
+        }
 
-        scriptErrorDialog(ControllerDebug::enabled() ?
-                QString("%1\nBacktrace:\n%2")
-                .arg(errorText, backtrace.join("\n")) : errorText);
+        // Add backtrace to the error details
+        errorText = errorText + QStringLiteral("\n\n") + tr("Backtrace:") +
+                QStringLiteral("\n  ") +
+                m_pEngine->uncaughtExceptionBacktrace().join("\n  ");
 
+        scriptErrorDialog(errorText);
         m_pEngine->clearExceptions();
         return true;
     }
@@ -485,16 +520,42 @@ bool ControllerEngine::checkException() {
     Input:   Detailed error string
     Output:  -
     -------- ------------------------------------------------------ */
-void ControllerEngine::scriptErrorDialog(const QString& detailedError) {
+void ControllerEngine::scriptErrorDialog(
+        const QString& detailedError, bool bFatalError) {
     qWarning() << "ControllerEngine:" << detailedError;
-    ErrorDialogProperties* props = ErrorDialogHandler::instance()->newDialogProperties();
+
+    if (!m_bPopups) {
+        return;
+    }
+
+    ErrorDialogProperties* props =
+            ErrorDialogHandler::instance()->newDialogProperties();
+
+    QString additionalErrorText;
+    if (bFatalError) {
+        additionalErrorText =
+                tr("The functionality provided by this controller mapping will "
+                   "be disabled until the issue has been resolved.");
+    } else {
+        additionalErrorText =
+                tr("You can ignore this error for this session but "
+                   "you may experience erratic behavior.") +
+                QString("<br>") +
+                tr("Try to recover by resetting your controller.");
+    }
+
     props->setType(DLG_WARNING);
-    props->setTitle(tr("Controller script error"));
-    props->setText(tr("A control you just used is not working properly."));
-    props->setInfoText("<html>"+tr("The script code needs to be fixed.")+
-        "<p>"+tr("For now, you can: Ignore this error for this session but you may experience erratic behavior.")+
-        "<br>"+tr("Try to recover by resetting your controller.")+"</p>"+"</html>");
-    props->setDetails(detailedError);
+    props->setTitle(tr("Controller Preset Error"));
+    props->setText(QString(tr("The preset for your controller \"%1\" is not "
+                              "working properly."))
+                           .arg(m_pController->getName()));
+    props->setInfoText(QStringLiteral("<html>") +
+            tr("The script code needs to be fixed.") + QStringLiteral("<p>") +
+            additionalErrorText + QStringLiteral("</p></html>"));
+
+    // Add "Details" text and set monospace font since they may contain
+    // backtraces and code.
+    props->setDetails(detailedError, true);
     props->setKey(detailedError);   // To prevent multiple windows for the same error
 
     // Allow user to suppress further notifications about this particular error
@@ -951,9 +1012,10 @@ bool ControllerEngine::evaluate(const QFileInfo& scriptFile) {
             // Set up error dialog
             ErrorDialogProperties* props = ErrorDialogHandler::instance()->newDialogProperties();
             props->setType(DLG_WARNING);
-            props->setTitle("Controller script file problem");
-            props->setText(QString("There was a problem opening the controller script file %1.").arg(filename));
-            props->setInfoText(input.errorString());
+            props->setTitle(tr("Controller Mapping File Problem"));
+            props->setText(tr("The mapping for controller \"%1\" cannot be opened.").arg(m_pController->getName()));
+            props->setInfoText(tr("The functionality provided by this controller mapping will be disabled until the issue has been resolved."));
+            props->setDetails(QString(tr("File: %1\n\n")).arg(filename) + input.errorString());
 
             // Ask above layer to display the dialog & handle user response
             ErrorDialogHandler::instance()->requestErrorDialog(props);
@@ -967,35 +1029,7 @@ bool ControllerEngine::evaluate(const QFileInfo& scriptFile) {
     input.close();
 
     // Check syntax
-    QScriptSyntaxCheckResult result = m_pEngine->checkSyntax(scriptCode);
-    QString error = "";
-    switch (result.state()) {
-        case (QScriptSyntaxCheckResult::Valid): break;
-        case (QScriptSyntaxCheckResult::Intermediate):
-            error = "Incomplete code";
-            break;
-        case (QScriptSyntaxCheckResult::Error):
-            error = "Syntax error";
-            break;
-    }
-    if (error != "") {
-        error = QString("%1 at line %2, column %3 in file %4: %5")
-                    .arg(error,
-                         QString::number(result.errorLineNumber()),
-                         QString::number(result.errorColumnNumber()),
-                         filename, result.errorMessage());
-
-        qWarning() << "ControllerEngine:" << error;
-        if (m_bPopups) {
-            ErrorDialogProperties* props = ErrorDialogHandler::instance()->newDialogProperties();
-            props->setType(DLG_WARNING);
-            props->setTitle("Controller script file error");
-            props->setText(QString("There was an error in the controller script file %1.").arg(filename));
-            props->setInfoText("The functionality provided by this script file will be disabled.");
-            props->setDetails(error);
-
-            ErrorDialogHandler::instance()->requestErrorDialog(props);
-        }
+    if (!syntaxIsValid(scriptCode, filename)) {
         return false;
     }