Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax strictness of ControllerScriptInterfaceLegacy methods. #11474

Merged
merged 5 commits into from
Jun 4, 2023
Merged

Relax strictness of ControllerScriptInterfaceLegacy methods. #11474

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f540abc
feat(cli): add `--controller-pedantic` cli option
Swiftb0y Apr 12, 2023
063a39e
feat(controllers): make `--controller-pedantic` accessible via `Contr…
Swiftb0y Apr 12, 2023
7ab811c
fix(controllers): only throw when connecting to non-existing CO when …
Swiftb0y Apr 12, 2023
7d35534
refactor(controllers): use logOrThrowError for more error messages.
Swiftb0y Apr 12, 2023
b09187e
rename controller-pedantic to controller-abort-on-warning
Swiftb0y Jun 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/controllers/scripting/controllerscriptenginebase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@
#include "errordialoghandler.h"
#include "mixer/playermanager.h"
#include "moc_controllerscriptenginebase.cpp"
#include "util/cmdlineargs.h"

ControllerScriptEngineBase::ControllerScriptEngineBase(
Controller* controller, const RuntimeLoggingCategory& logger)
: m_bDisplayingExceptionDialog(false),
m_pJSEngine(nullptr),
m_pController(controller),
m_logger(logger),
m_bPedantic(false),
m_bTesting(false) {
// Handle error dialog buttons
qRegisterMetaType<QMessageBox::StandardButton>("QMessageBox::StandardButton");
Expand All @@ -23,6 +25,8 @@ bool ControllerScriptEngineBase::initialize() {
return false;
}

m_bPedantic = CmdlineArgs::Instance().getControllerPedantic();

// Create the Script Engine
m_pJSEngine = std::make_shared<QJSEngine>(this);

Expand Down
6 changes: 6 additions & 0 deletions src/controllers/scripting/controllerscriptenginebase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ class ControllerScriptEngineBase : public QObject {
void showScriptExceptionDialog(const QJSValue& evaluationResult, bool bFatal = false);
void throwJSError(const QString& message);

bool isPedantic() const {
return m_bPedantic;
}

inline void setTesting(bool testing) {
m_bTesting = testing;
};
Expand All @@ -51,6 +55,8 @@ class ControllerScriptEngineBase : public QObject {
Controller* m_pController;
const RuntimeLoggingCategory m_logger;

bool m_bPedantic;

bool m_bTesting;

protected slots:
Expand Down
83 changes: 53 additions & 30 deletions src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ ControlObjectScript* ControllerScriptInterfaceLegacy::getControlObjectScript(
double ControllerScriptInterfaceLegacy::getValue(const QString& group, const QString& name) {
ControlObjectScript* coScript = getControlObjectScript(group, name);
if (coScript == nullptr) {
qCWarning(m_logger) << "Unknown control" << group << name
<< ", returning 0.0";
logOrThrowError(QStringLiteral("Unknown control (%1, %2) returning 0.0").arg(group, name));
return 0.0;
}
return coScript->get();
Expand All @@ -120,8 +119,9 @@ double ControllerScriptInterfaceLegacy::getValue(const QString& group, const QSt
void ControllerScriptInterfaceLegacy::setValue(
const QString& group, const QString& name, double newValue) {
if (util_isnan(newValue)) {
qCWarning(m_logger) << "script setting [" << group << ","
<< name << "] to NotANumber, ignoring.";
logOrThrowError(QStringLiteral(
"Script tried setting (%1, %2) to NotANumber (NaN)")
.arg(group, name));
return;
}

Expand All @@ -141,8 +141,7 @@ void ControllerScriptInterfaceLegacy::setValue(
double ControllerScriptInterfaceLegacy::getParameter(const QString& group, const QString& name) {
ControlObjectScript* coScript = getControlObjectScript(group, name);
if (coScript == nullptr) {
qCWarning(m_logger) << "Unknown control" << group << name
<< ", returning 0.0";
logOrThrowError(QStringLiteral("Unknown control (%1, %2) returning 0.0").arg(group, name));
return 0.0;
}
return coScript->getParameter();
Expand All @@ -151,8 +150,9 @@ double ControllerScriptInterfaceLegacy::getParameter(const QString& group, const
void ControllerScriptInterfaceLegacy::setParameter(
const QString& group, const QString& name, double newParameter) {
if (util_isnan(newParameter)) {
qCWarning(m_logger) << "script setting [" << group << ","
<< name << "] to NotANumber, ignoring.";
logOrThrowError(QStringLiteral(
"Script tried setting (%1, %2) to NotANumber (NaN)")
.arg(group, name));
return;
}

Expand All @@ -170,16 +170,16 @@ void ControllerScriptInterfaceLegacy::setParameter(
double ControllerScriptInterfaceLegacy::getParameterForValue(
const QString& group, const QString& name, double value) {
if (util_isnan(value)) {
qCWarning(m_logger) << "script setting [" << group << ","
<< name << "] to NotANumber, ignoring.";
logOrThrowError(QStringLiteral(
"Script tried setting (%1, %2) to NotANumber (NaN)")
.arg(group, name));
return 0.0;
}

ControlObjectScript* coScript = getControlObjectScript(group, name);

if (coScript == nullptr) {
qCWarning(m_logger) << "Unknown control" << group << name
<< ", returning 0.0";
logOrThrowError(QStringLiteral("Unknown control (%1, %2) returning 0.0").arg(group, name));
return 0.0;
}

Expand All @@ -197,8 +197,7 @@ double ControllerScriptInterfaceLegacy::getDefaultValue(const QString& group, co
ControlObjectScript* coScript = getControlObjectScript(group, name);

if (coScript == nullptr) {
qCWarning(m_logger) << "Unknown control" << group << name
<< ", returning 0.0";
logOrThrowError(QStringLiteral("Unknown control (%1, %2) returning 0.0").arg(group, name));
return 0.0;
}

Expand All @@ -210,8 +209,7 @@ double ControllerScriptInterfaceLegacy::getDefaultParameter(
ControlObjectScript* coScript = getControlObjectScript(group, name);

if (coScript == nullptr) {
qCWarning(m_logger) << "Unknown control" << group << name
<< ", returning 0.0";
logOrThrowError(QStringLiteral("Unknown control (%1, %2) returning 0.0").arg(group, name));
return 0.0;
}

Expand Down Expand Up @@ -240,19 +238,19 @@ QJSValue ControllerScriptInterfaceLegacy::makeConnectionInternal(
// The test setups do not run all of Mixxx, so ControlObjects not
// existing during tests is okay.
if (!m_pScriptEngineLegacy->isTesting()) {
m_pScriptEngineLegacy->throwJSError(
"script tried to connect to "
"ControlObject (" +
group + ", " + name + ") which is non-existent.");
logOrThrowError(
QStringLiteral("script tried to connect to ControlObject "
"(%1, %2) which is non-existent.")
.arg(group, name));
}
return QJSValue();
}

if (!callback.isCallable()) {
m_pScriptEngineLegacy->throwJSError("Tried to connect (" + group + ", " + name +
")" +
" to an invalid callback. Make sure that your code contains no "
"syntax errors.");
logOrThrowError(QStringLiteral(
"Tried to connect (%1, %2) to an invalid callback. Make sure "
"that your code contains no syntax errors.")
.arg(group, name));
return QJSValue();
}

Expand Down Expand Up @@ -293,6 +291,10 @@ void ControllerScriptInterfaceLegacy::triggerScriptConnection(
ControlObjectScript* coScript =
getControlObjectScript(connection.key.group, connection.key.item);
if (coScript == nullptr) {
logOrThrowError(QStringLiteral(
"Script tried to trigger (%1, %2) which is non-existent.")
.arg(connection.key.group,
connection.key.item));
return;
}

Expand All @@ -311,6 +313,11 @@ QJSValue ControllerScriptInterfaceLegacy::connectControl(const QString& group,
const QString& name,
const QJSValue& passedCallback,
bool disconnect) {
logOrThrowError(QStringLiteral(
"Script tried to connect to (%1, %2) using `connectControl` which "
"is deprecated. Use `makeConnection` instead!")
Comment on lines +317 to +318
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Script tried to connect to (%1, %2) using `connectControl` which "
"is deprecated. Use `makeConnection` instead!")
"Script tried to connect to (%1, %2) using `connectControl` which "
"is deprecated. Use `makeConnection` or `makeUnbufferedConnection` instead!")

.arg(group, name));

// The passedCallback may or may not actually be a function, so when
// the actual callback function is found, store it in this variable.
QJSValue actualCallbackFunction;
Expand Down Expand Up @@ -427,18 +434,33 @@ QJSValue ControllerScriptInterfaceLegacy::connectControl(const QString& group,

void ControllerScriptInterfaceLegacy::trigger(const QString& group, const QString& name) {
ControlObjectScript* coScript = getControlObjectScript(group, name);
if (coScript != nullptr) {
coScript->emitValueChanged();
if (coScript == nullptr) {
logOrThrowError(QStringLiteral(
"Script tried to trigger (%1, %2) which is non-existent.")
.arg(group, name));
return;
}
coScript->emitValueChanged();
}

void ControllerScriptInterfaceLegacy::logOrThrowError(const QString& errorMessage) const {
if (m_pScriptEngineLegacy->isPedantic()) {
m_pScriptEngineLegacy->throwJSError(errorMessage);
} else {
qCWarning(m_logger) << errorMessage;
}
}

void ControllerScriptInterfaceLegacy::log(const QString& message) {
qCDebug(m_logger) << "engine.log is deprecated. Use console.log instead.";
logOrThrowError(QStringLiteral("`engine.log` is deprecated. Use `console.log` instead!"));
qCDebug(m_logger) << message;
}
int ControllerScriptInterfaceLegacy::beginTimer(
int intervalMillis, QJSValue timerCallback, bool oneShot) {
if (timerCallback.isString()) {
logOrThrowError(
QStringLiteral("passed a string to `engine.beginTimer`, please "
"pass a function instead!"));
timerCallback = m_pScriptEngineLegacy->jsEngine()->evaluate(timerCallback.toString());
} else if (!timerCallback.isCallable()) {
QString sErrorMessage(
Expand Down Expand Up @@ -467,7 +489,7 @@ int ControllerScriptInterfaceLegacy::beginTimer(
info.oneShot = oneShot;
m_timers[timerId] = info;
if (timerId == 0) {
qCWarning(m_logger) << "Script timer could not be created";
logOrThrowError(QStringLiteral("Script timer could not be created"));
} else if (oneShot) {
qCDebug(m_logger) << "Starting one-shot timer:" << timerId;
} else {
Expand All @@ -478,8 +500,9 @@ int ControllerScriptInterfaceLegacy::beginTimer(

void ControllerScriptInterfaceLegacy::stopTimer(int timerId) {
if (!m_timers.contains(timerId)) {
qCWarning(m_logger) << "Killing timer" << timerId
<< ": That timer does not exist!";
logOrThrowError(QStringLiteral(
"Tried to kill Timer \"%1\" that does not exists")
.arg(timerId));
return;
}
qCDebug(m_logger) << "Killing timer:" << timerId;
Expand Down
1 change: 1 addition & 0 deletions src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class ControllerScriptInterfaceLegacy : public QObject {
bool skipSuperseded = false);
QHash<ConfigKey, ControlObjectScript*> m_controlCache;
ControlObjectScript* getControlObjectScript(const QString& group, const QString& name);
void logOrThrowError(const QString& errorMessage) const;

SoftTakeoverCtrl m_st;

Expand Down
13 changes: 13 additions & 0 deletions src/util/cmdlineargs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
CmdlineArgs::CmdlineArgs()
: m_startInFullscreen(false), // Initialize vars
m_controllerDebug(false),
m_controllerPedantic(false),
m_developer(false),
m_safeMode(false),
m_useVuMeterGL(true),
Expand Down Expand Up @@ -193,6 +194,17 @@ bool CmdlineArgs::parse(const QStringList& arguments, CmdlineArgs::ParseMode mod
parser.addOption(controllerDebug);
parser.addOption(controllerDebugDeprecated);

const QCommandLineOption controllerPedantic(
QStringLiteral("controller-pedantic"),
forUserFeedback ? QCoreApplication::translate("CmdlineArgs",
"The controller mapping will issue more "
"aggressive warnings and errors when "
"detecting misuse of controller APIs. "
"New Controller Mappings should be "
"developed with this option enabled!")
: QString());
parser.addOption(controllerPedantic);

const QCommandLineOption developer(QStringLiteral("developer"),
forUserFeedback ? QCoreApplication::translate("CmdlineArgs",
"Enables developer-mode. Includes extra log info, stats on "
Expand Down Expand Up @@ -332,6 +344,7 @@ bool CmdlineArgs::parse(const QStringList& arguments, CmdlineArgs::ParseMode mod

m_useVuMeterGL = !(parser.isSet(disableVuMeterGL) || parser.isSet(disableVuMeterGLDeprecated));
m_controllerDebug = parser.isSet(controllerDebug) || parser.isSet(controllerDebugDeprecated);
m_controllerPedantic = parser.isSet(controllerPedantic);
m_developer = parser.isSet(developer);
m_safeMode = parser.isSet(safeMode) || parser.isSet(safeModeDeprecated);
m_debugAssertBreak = parser.isSet(debugAssertBreak) || parser.isSet(debugAssertBreakDeprecated);
Expand Down
4 changes: 4 additions & 0 deletions src/util/cmdlineargs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ class CmdlineArgs final {
bool getControllerDebug() const {
return m_controllerDebug;
}
bool getControllerPedantic() const {
return m_controllerPedantic;
}
bool getDeveloper() const { return m_developer; }
bool getSafeMode() const { return m_safeMode; }
bool useColors() const {
Expand Down Expand Up @@ -73,6 +76,7 @@ class CmdlineArgs final {
QList<QString> m_musicFiles; // List of files to load into players at startup
bool m_startInFullscreen; // Start in fullscreen mode
bool m_controllerDebug;
bool m_controllerPedantic; // Controller Engine will be stricter
bool m_developer; // Developer Mode
bool m_safeMode;
bool m_useVuMeterGL;
Expand Down