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.

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

ControllerEngine Refactoring #3463

Merged
merged 79 commits into from
Dec 20, 2020
Merged
Show file tree
Hide file tree
Changes from 78 commits
Commits
Show all changes
79 commits
Select commit Hold shift + click to select a range
fed2e1b
split ControllerEngine functionality into ControllerEngineJSProxy
Be-ing Jul 6, 2020
9c04291
reorganize controller scripting code
Be-ing Jul 6, 2020
24408ab
controllers: clean up some #includes
Be-ing Jul 6, 2020
979a243
split ControllerScriptHandler into base and subclasses
Be-ing Jul 6, 2020
b3d6ff8
remove hacks for JS modules in legacy controller system
Be-ing Jul 6, 2020
72e849f
ControllerScriptEngineBase: remove outdated comment
Be-ing Jul 12, 2020
0968a9d
Controller: replace legacy NULL with nullptr
Be-ing Jul 12, 2020
77ca43f
ControllerScriptModuleEngine: keep build working with Qt < 5.12
Be-ing Jul 12, 2020
8153b3f
cleanup handling of incoming HID/USB Bulk/MIDI sysex data
Be-ing Jul 12, 2020
9363b49
MidiController: rename confusingly overloaded "receive" method
Be-ing Jul 12, 2020
e25bf12
ControllerScriptEngine: generate wrappers for input callbacks
Be-ing Jul 13, 2020
e8e2487
HIDController: avoid deep copying input data
Be-ing Jul 13, 2020
26b51ec
Controller: pass received input QByteArray by reference
Be-ing Jul 13, 2020
e99cef3
BulkController: avoid deep copying input data
Be-ing Jul 13, 2020
f6f7050
ControllerScriptModuleEngine: pass raw ArrayBuffer to input callback
Be-ing Jul 13, 2020
b4d2d21
Hss1394Controller: fix build
Be-ing Jul 14, 2020
197c0d0
Merge remote-tracking branch 'upstream/master' into controllerengine_…
Be-ing Jul 16, 2020
62fc40c
Merge remote-tracking branch 'upstream/master' into controllerengine_…
Be-ing Sep 1, 2020
2046add
change explicit comparisons to nullptr to implicit
Be-ing Sep 1, 2020
a691d32
HSS1394Controller: replace C cast with reinterpret_cast
Be-ing Sep 1, 2020
487f8f9
ControllerScriptEngineLegacy: make iterator const
Be-ing Sep 1, 2020
2b6eb9f
rename ControllerScriptInterface to ControllerScriptInterfaceLegacy
Be-ing Sep 1, 2020
fa16b57
ControllerScriptInterfaceLegacy: clang-format fixes
Be-ing Sep 1, 2020
427ba5e
assert for missing ControlObjects with --controllerDebug CLI option
Be-ing Sep 1, 2020
caf3d68
remove legacy ControllerEngine file brought back from merge
Be-ing Sep 1, 2020
6c1a6b6
ControllerScriptEngineLegacy: remove QString initialization to ""
Be-ing Sep 1, 2020
df4f377
ControllerScriptEngineBase: fix typo in comment
Be-ing Sep 2, 2020
8ec7ed8
fix ControllerEngine tests
Be-ing Sep 2, 2020
0d39501
rename ControllerEngineTest to ControllerScriptEngineLegacyTest
Be-ing Sep 2, 2020
13f86a5
ControllerScriptEngineLegacyTest clang-format fixes
Be-ing Sep 2, 2020
e028e3e
Merge remote-tracking branch 'upstream/master' into controllerengine_…
Be-ing Oct 5, 2020
5c1ace9
Merge remote-tracking branch 'upstream/main' into controllerengine_re…
Be-ing Oct 24, 2020
bdf497b
ControllerScriptEngineBase: make m_scriptWatcher private
Be-ing Oct 24, 2020
3383aef
ControllerScriptEngineBase: don't call virtual destructor
Be-ing Oct 24, 2020
19b9aaf
ControllerScriptEngineBase: fix removing files from watcher
Be-ing Oct 24, 2020
4505c86
ControllerScriptEngineBase: pass QFileInfo& to watchScriptFile
Be-ing Oct 24, 2020
95b83ea
make ControllerScriptEngine classes' constructors explicit
Be-ing Oct 24, 2020
e4bdfdd
mark child classes' destructors override
Be-ing Oct 24, 2020
269de0d
ControllerScriptEngineBase: log error when unable to watch file
Be-ing Oct 24, 2020
d72618f
move script watching from ControllerScriptEngineBase to child classes
Be-ing Oct 25, 2020
d677f4b
ControllerScriptEngineLegacy: stop watching old script files
Be-ing Oct 25, 2020
5b02715
remove class names from debug messages
Be-ing Oct 25, 2020
424c94b
Controller: remove unnecessary local variable
Be-ing Oct 25, 2020
d50b710
HSS1394 & Bulk controllers: don't use QByteArray::fromRawData
Be-ing Oct 25, 2020
fa8771f
MidiController: formatting
Be-ing Oct 25, 2020
c96a2c2
ControllerScriptEngineBase: remove superfluous QString() wrapper
Be-ing Oct 25, 2020
cb41ffd
ControllerScriptEngineBase: make isTesting const
Be-ing Oct 25, 2020
a80ca2c
ControllerScriptEngineBase: remove obsolete friend declaration
Be-ing Oct 25, 2020
f7b0ddc
ControllerScriptEngineLegacy: use std::as_const for looped container
Be-ing Oct 25, 2020
0112554
ControllerScriptEngineLegacy: reserve known list size
Be-ing Oct 25, 2020
65de8c5
ControllerScriptEngineLegacy: remove undefined declaration
Be-ing Oct 25, 2020
94d9423
ControllerScriptEngineLegacy: remove obsolete friend class declarations
Be-ing Oct 25, 2020
388b4e3
ControllerScriptEngineLegacy: make jsEngine() hack private and const
Be-ing Oct 25, 2020
57c0219
ControllerScriptEngineLegacy: remove obsolete forward declarations
Be-ing Oct 25, 2020
e504678
ControllerScriptEngineLegacy: remove unneeded #include
Be-ing Oct 25, 2020
e35e90f
ControllerScriptEngineLegacy: use QStringBuilder
Be-ing Oct 25, 2020
5b81d28
fix legacy controller preset validation tests
Be-ing Oct 26, 2020
98d9e37
fix PortMidiController tests
Be-ing Oct 26, 2020
4e744e0
Merge remote-tracking branch 'upstream/main' into controllerengine_re…
Be-ing Oct 29, 2020
6a78b67
Merge remote-tracking branch 'upstream/main' into controllerengine_re…
Be-ing Oct 30, 2020
0e6119c
ControllerScriptEngineBase: add missing override
Be-ing Oct 30, 2020
145cf8e
rename receiveShortMessage/Sysex -> receivedShortMessage/Sysex
Be-ing Oct 30, 2020
7f2187d
ControllerScriptModuleEngine: remove input handling
Be-ing Oct 30, 2020
30692a1
ControllerScriptEngineBase: remove confusing (obsolete?) comment
Be-ing Nov 1, 2020
86bebb6
ControllerScriptEngineBase/Legacy: use shared_ptr for QJSEngine
Be-ing Nov 2, 2020
4227e22
ControllerScriptEngineLegacy: rename local variable
Be-ing Nov 3, 2020
b8e2430
Merge remote-tracking branch 'upstream/main' into controllerengine_re…
Be-ing Nov 3, 2020
20fbcb0
Merge remote-tracking branch 'upstream/main' into controllerengine_re…
Be-ing Nov 30, 2020
b5cf59d
controllers: fix clazy warnings
Be-ing Dec 1, 2020
1911580
Merge remote-tracking branch 'upstream/main' into controllerengine_re…
Be-ing Dec 8, 2020
b5fafcc
Merge remote-tracking branch 'upstream/main' into controllerengine_re…
Be-ing Dec 11, 2020
57b7406
remove accidentally commited mixxx.log.1 file
Be-ing Dec 11, 2020
3a40266
Merge branch 'main' of github.com:mixxxdj/mixxx into controllerengine…
Holzhaus Dec 17, 2020
221dab8
ControllerScriptEngineBase: Add comment regarding input handler errors
Holzhaus Dec 17, 2020
66f447c
ControllerScriptEngineBase: Fix inverted logic in scriptErrorDialog
Holzhaus Dec 17, 2020
7e770cf
ControllerScriptEngineLegacy: Abort initialization if base init failed
Holzhaus Dec 17, 2020
de29f92
ControllerScriptEngineBase: Move JS array buffer wrapping to legacy
Holzhaus Dec 17, 2020
6c8cb90
ControllerScriptEngineLegacy: Remove temp variable for initialize() r…
Holzhaus Dec 17, 2020
4c95980
ControllerDebug: Clean up method names and add a way to disable testing
Holzhaus Dec 18, 2020
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
16 changes: 9 additions & 7 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -414,12 +414,14 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
src/controllers/dlgprefcontrollerdlg.ui
src/controllers/dlgprefcontrollers.cpp
src/controllers/dlgprefcontrollersdlg.ui
src/controllers/engine/controllerengine.cpp
src/controllers/engine/controllerenginejsproxy.cpp
src/controllers/engine/colormapper.cpp
src/controllers/engine/colormapperjsproxy.cpp
src/controllers/engine/scriptconnection.cpp
src/controllers/engine/scriptconnectionjsproxy.cpp
src/controllers/scripting/controllerscriptenginebase.cpp
src/controllers/scripting/controllerscriptmoduleengine.cpp
src/controllers/scripting/colormapper.cpp
src/controllers/scripting/colormapperjsproxy.cpp
src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp
src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp
src/controllers/scripting/legacy/scriptconnection.cpp
src/controllers/scripting/legacy/scriptconnectionjsproxy.cpp
src/controllers/keyboard/keyboardeventfilter.cpp
src/controllers/learningutils.cpp
src/controllers/midi/midicontroller.cpp
Expand Down Expand Up @@ -1388,7 +1390,7 @@ add_executable(mixxx-test
src/test/compatibility_test.cpp
src/test/configobject_test.cpp
src/test/controller_preset_validation_test.cpp
src/test/controllerengine_test.cpp
src/test/controllerscriptenginelegacy_test.cpp
src/test/controlobjecttest.cpp
src/test/coverartcache_test.cpp
src/test/coverartutils_test.cpp
Expand Down
3 changes: 2 additions & 1 deletion src/control/controlobjectscript.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@

#include <QtDebug>

#include "controllers/controllerdebug.h"
#include "moc_controlobjectscript.cpp"

ControlObjectScript::ControlObjectScript(const ConfigKey& key, QObject* pParent)
: ControlProxy(key, pParent, ControlFlag::NoAssertIfMissing) {
: ControlProxy(key, pParent, ControllerDebug::shouldAssertForInvalidControlObjects()) {
}

bool ControlObjectScript::addScriptConnection(const ScriptConnection& conn) {
Expand Down
3 changes: 1 addition & 2 deletions src/control/controlobjectscript.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@
#include <QVector>

#include "control/controlproxy.h"
#include "controllers/controllerdebug.h"
#include "controllers/engine/scriptconnection.h"
#include "controllers/scripting/legacy/scriptconnection.h"

// this is used for communicate with controller scripts
class ControlObjectScript : public ControlProxy {
Expand Down
4 changes: 2 additions & 2 deletions src/controllers/bulk/bulkcontroller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ void BulkReader::run() {
if (result >= 0) {
Trace process("BulkReader process packet");
//qDebug() << "Read" << result << "bytes, pointer:" << data;
QByteArray outData((char*)data, transferred);
emit incomingData(outData, mixxx::Time::elapsed());
QByteArray byteArray(reinterpret_cast<char*>(data), transferred);
emit incomingData(byteArray, mixxx::Time::elapsed());
}
}
qDebug() << "Stopped Reader";
Expand Down
45 changes: 12 additions & 33 deletions src/controllers/controller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#include "util/screensaver.h"

Controller::Controller()
: m_pEngine(nullptr),
: m_pScriptEngineLegacy(nullptr),
m_bIsOutputDevice(false),
m_bIsInputDevice(false),
m_bIsOpen(false),
Expand All @@ -29,31 +29,30 @@ ControllerJSProxy* Controller::jsProxy() {
void Controller::startEngine()
{
controllerDebug(" Starting engine");
if (m_pEngine != nullptr) {
if (m_pScriptEngineLegacy) {
qWarning() << "Controller: Engine already exists! Restarting:";
stopEngine();
}
m_pEngine = new ControllerEngine(this);
m_pScriptEngineLegacy = new ControllerScriptEngineLegacy(this);
}

void Controller::stopEngine() {
controllerDebug(" Shutting down engine");
if (m_pEngine == nullptr) {
if (!m_pScriptEngineLegacy) {
qWarning() << "Controller::stopEngine(): No engine exists!";
return;
}
m_pEngine->gracefulShutdown();
delete m_pEngine;
m_pEngine = nullptr;
delete m_pScriptEngineLegacy;
m_pScriptEngineLegacy = nullptr;
}

bool Controller::applyPreset(bool initializeScripts) {
bool Controller::applyPreset() {
qDebug() << "Applying controller preset...";

const ControllerPreset* pPreset = preset();

// Load the script code into the engine
if (m_pEngine == nullptr) {
if (!m_pScriptEngineLegacy) {
qWarning() << "Controller::applyPreset(): No engine exists!";
return false;
}
Expand All @@ -64,19 +63,8 @@ bool Controller::applyPreset(bool initializeScripts) {
return true;
}

bool success = m_pEngine->loadScriptFiles(scriptFiles);
if (success && initializeScripts) {
m_pEngine->initializeScripts(scriptFiles);
}

// QFileInfo does not have a isValid/isEmpty/isNull method to check if it
// actually contains a reference, so we check if the filePath is empty as a
// workaround.
// See https://stackoverflow.com/a/45652741/1455128 for details.
if (initializeScripts && !pPreset->moduleFileInfo().filePath().isEmpty()) {
m_pEngine->loadModule(pPreset->moduleFileInfo());
}
return success;
m_pScriptEngineLegacy->setScriptFiles(scriptFiles);
return m_pScriptEngineLegacy->initialize();
}

void Controller::startLearning() {
Expand Down Expand Up @@ -113,7 +101,7 @@ void Controller::triggerActivity()
}
}
void Controller::receive(const QByteArray& data, mixxx::Duration timestamp) {
if (m_pEngine == nullptr) {
if (!m_pScriptEngineLegacy) {
//qWarning() << "Controller::receive called with no active engine!";
// Don't complain, since this will always show after closing a device as
// queued signals flush out
Expand Down Expand Up @@ -145,14 +133,5 @@ void Controller::receive(const QByteArray& data, mixxx::Duration timestamp) {
controllerDebug(message);
}

foreach (QString function, m_pEngine->getScriptFunctionPrefixes()) {
if (function == "") {
continue;
}
function.append(".incomingData");
QJSValue incomingDataFunction = m_pEngine->wrapFunctionCode(function, 2);
m_pEngine->executeFunction(incomingDataFunction, data);
}

m_pEngine->handleInput(data, timestamp);
m_pScriptEngineLegacy->handleIncomingData(data);
}
23 changes: 8 additions & 15 deletions src/controllers/controller.h
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
#pragma once

#include <QElapsedTimer>
#include <QTimerEvent>

#include "controllers/controllerpreset.h"
#include "controllers/controllerpresetfilehandler.h"
#include "controllers/controllerpresetinfo.h"
#include "controllers/controllerpresetvisitor.h"
#include "controllers/controllervisitor.h"
#include "controllers/engine/controllerengine.h"
#include "controllers/scripting/legacy/controllerscriptenginelegacy.h"
#include "util/duration.h"

class ControllerJSProxy;

/// Base class representing a physical (or software) controller.
///
/// This is a base class representing a physical (or software) controller. It
/// must be inherited by a class that implements it on some API. Note that the
/// subclass' destructor should call close() at a minimum.
Expand Down Expand Up @@ -85,13 +84,7 @@ class Controller : public QObject, ConstControllerPresetVisitor {
// this if they have an alternate way of handling such data.)
virtual void receive(const QByteArray& data, mixxx::Duration timestamp);

/// Apply the preset to the controller.
/// Initializes both controller engine and static output mappings.
///
/// @param initializeScripts Can be set to false to skip script
/// initialization for unit tests.
/// @return Returns whether it was successful.
virtual bool applyPreset(bool initializeScripts = true);
virtual bool applyPreset();

// Puts the controller in and out of learning mode.
void startLearning();
Expand All @@ -108,17 +101,17 @@ class Controller : public QObject, ConstControllerPresetVisitor {

// To be called in sub-class' open() functions after opening the device but
// before starting any input polling/processing.
void startEngine();
virtual void startEngine();

// To be called in sub-class' close() functions after stopping any input
// polling/processing but before closing the device.
void stopEngine();
virtual void stopEngine();

// To be called when receiving events
void triggerActivity();

inline ControllerEngine* getEngine() const {
return m_pEngine;
inline ControllerScriptEngineLegacy* getScriptEngine() const {
return m_pScriptEngineLegacy;
}
inline void setDeviceName(const QString& deviceName) {
m_sDeviceName = deviceName;
Expand Down Expand Up @@ -155,7 +148,7 @@ class Controller : public QObject, ConstControllerPresetVisitor {
// Returns a pointer to the currently loaded controller preset. For internal
// use only.
virtual ControllerPreset* preset() = 0;
ControllerEngine* m_pEngine;
ControllerScriptEngineLegacy* m_pScriptEngineLegacy;

// Verbose and unique device name suitable for display.
QString m_sDeviceName;
Expand Down
1 change: 1 addition & 0 deletions src/controllers/controllerdebug.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

//static
bool ControllerDebug::s_enabled = false;
bool ControllerDebug::s_testing = false;

//static
bool ControllerDebug::enabled() {
Expand Down
16 changes: 16 additions & 0 deletions src/controllers/controllerdebug.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#include <QDebug>

#include "control/control.h"

// Specifies whether or not we should dump incoming data to the console at
// runtime. This is useful for end-user debugging and script-writing.
class ControllerDebug {
Expand All @@ -23,10 +25,24 @@ class ControllerDebug {
s_enabled = false;
}

static void enableTesting() {
s_enabled = true;
s_testing = true;
}

static ControlFlags shouldAssertForInvalidControlObjects() {
Copy link
Member

Choose a reason for hiding this comment

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

The function name is a question that can be answered with true or false but not with individual flags.
I would prefer to make this bool, because the flags are an implementation detail of code out of sight.
Alternative we need a new name.

if (s_enabled && !s_testing) {
return ControlFlag::None;
Copy link
Member

Choose a reason for hiding this comment

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

None means assert all assumptions. What is the reasoning that we don't want that during tests? Is this correct:

s_testing = true s_testing = false
s_enabled = true ignore assert
s_enabled = false ignore ignore

Please add a comment why.
Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

We may also consider to rename ControlFlag::None to
ControlFlag::AssertAll; Or AssertExeptions:None or such.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please add a comment why.

This was done because some tests are expected to fail. I just pushed the new commits that disable controller testing for these tests.

}

return ControlFlag::AllowMissingOrInvalid;
}

private:
ControllerDebug() = delete;

static bool s_enabled;
static bool s_testing;
};

// Usage: controllerDebug("hello" << "world");
Expand Down
9 changes: 0 additions & 9 deletions src/controllers/controllerpreset.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,6 @@ class ControllerPreset {
return m_scripts;
}

void setModuleFileInfo(QFileInfo fileInfo) {
m_moduleFileInfo = std::move(fileInfo);
}

QFileInfo moduleFileInfo() const {
return m_moduleFileInfo;
}

inline void setDirty(bool bDirty) {
m_bDirty = bDirty;
}
Expand Down Expand Up @@ -203,7 +195,6 @@ class ControllerPreset {
QString m_mixxxVersion;

QList<ScriptFileInfo> m_scripts;
QFileInfo m_moduleFileInfo;
};

typedef QSharedPointer<ControllerPreset> ControllerPresetPointer;
14 changes: 0 additions & 14 deletions src/controllers/controllerpresetfilehandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,20 +142,6 @@ void ControllerPresetFileHandler::addScriptFilesToPreset(
preset->addScriptFile(filename, functionPrefix, file);
scriptFile = scriptFile.nextSiblingElement("file");
}

QString moduleFileName = controller.firstChildElement("module").text();

if (moduleFileName.isEmpty()) {
return;
}

QFileInfo moduleFileInfo(preset->dirPath().absoluteFilePath(moduleFileName));
if (!moduleFileInfo.isFile()) {
qWarning() << "Controller Module is not a file:" << moduleFileInfo.absoluteFilePath();
return;
}

preset->setModuleFileInfo(moduleFileInfo);
}

bool ControllerPresetFileHandler::writeDocument(
Expand Down
Loading