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

Ability for controller to share data at runtime #12199

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,7 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
src/controllers/controlpickermenu.cpp
src/controllers/legacycontrollermappingfilehandler.cpp
src/controllers/legacycontrollermapping.cpp
src/controllers/controllershareddata.cpp
src/controllers/delegates/controldelegate.cpp
src/controllers/delegates/midibytedelegate.cpp
src/controllers/delegates/midichanneldelegate.cpp
Expand Down Expand Up @@ -2099,6 +2100,7 @@ add_executable(mixxx-test
src/test/controller_mapping_settings_test.cpp
src/test/controllers/controller_columnid_regression_test.cpp
src/test/controllerscriptenginelegacy_test.cpp
src/test/controllershareddata_test.cpp
src/test/controlobjecttest.cpp
src/test/controlobjectaliastest.cpp
src/test/controlobjectscripttest.cpp
Expand Down
37 changes: 35 additions & 2 deletions res/controllers/engine-api.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ declare interface ScriptConnection {

declare namespace engine {
type SettingValue = string | number | boolean;
type SharedDataValue = string | number | boolean | object | array | undefined;
acolombier marked this conversation as resolved.
Show resolved Hide resolved
/**
* Gets the value of a controller setting
* The value is either set in the preferences dialog,
Expand Down Expand Up @@ -72,6 +73,28 @@ declare namespace engine {
*/
function setParameter(group: string, name: string, newValue: number): void;

/**
* Gets the shared runtime data.
* @returns Runtime shared data value
*/
function getSharedData(): SharedDataValue;
Comment on lines +76 to +80
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
/**
* Gets the shared runtime data.
* @returns Runtime shared data value
*/
function getSharedData(): SharedDataValue;
/**
* Gets the shared runtime data.
* @returns Runtime shared data value - or undefined if not yet defined
*/
function getSharedVariable(variablename:string): SharedDataValue;

We should not share one single complex blob. This makes it really difficult to understand for developers and we can't print meaningful error messages, when two mappings do no match together.
The later can especially happen, when you edit the variable type at runtime of Mixxx, because only one mapping is reloaded at a time.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should not share one single complex blob.

What if, in your namespace foobar, you want to share just a TypedArray or a string, like in the tests? I think adding variables don't add any values to the feature and just another level.

we can't print meaningful error messages, when two mappings do no match together.

This isn't possible anyway. I thought we agreed that introducing namespace was a good enough option to prevent conflict.
I also don't see how this would work anyway either since any controllers part of a namespace can get/edit values as well: this means that it is acceptable for MappingA to do get("MappingBRunning") (or get().MappingBRunning currently) and find no values.

Copy link
Member

Choose a reason for hiding this comment

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

Namespacing is good enough to prevent unintended iterference of completly unrelated mappings.
But this does not address compatibility between the related scripts using the same namespace.

Copy link
Member

Choose a reason for hiding this comment

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

I think this request is consistent with my original namespacing proposal above: #12199 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

How your solution will compatibility between the related scripts compare to the current one? Do you mean I should also implement the API change mentioned here?

Copy link
Member

Choose a reason for hiding this comment

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

This would be one approach. But also without an explicit declaration, it would be possible to print warnings like:
Line 1234 - Shared variable "deckSwitch" changed type in call of engine.setSharedVariable from number to string

Copy link
Member

Choose a reason for hiding this comment

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

This is all javascript, there is no schema / static typing, nor is it really necessary. If there is no technical reason why variables could not change their type I don't see a good reason to introduce one (especially when it would only result in extra complexity).

Copy link
Member Author

Choose a reason for hiding this comment

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

While I understand you are using type change as a way to detect a potential problem, this lefts out other usecase where a type change wouldn't occur but yet doesn't occur - eg wrong unit.
I think it all boils down to understanding what you are sharing, and making sure producers and consumers expect the same data AND don't conflict with each other.
I think this type of detection is a nice to have, but I cannot really see how this is required. Realistically, only a hand full of official mappings will use this features, so I think it is a matter of making sure this is well document (e.g using shared context class for example).
If you really feel like there should be a key for data set/get, I will add it, but lets agree first this is the last significant change I'm being asked to do.

Copy link
Member

@Swiftb0y Swiftb0y May 14, 2024

Choose a reason for hiding this comment

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

for the record, I don't think a schema is necessary. exchanging an untyped/dynamic object is completely fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JoergAtGithub is that a hard requirement in your opinion, or are you happy to go with the untyped dynamic data as it is currently?


/**
* Override the the shared runtime data with a new value.
*
* It is suggested to make additive changes (e.g add new attribute to existing object) in order to ease integration with other controller mapping
* @param newValue Runtime shared data value to be set
*/
function setSharedData(newValue: SharedDataValue): void;

/**
* Sets the control value specified with normalized range of 0..1
* @param group Group of the control e.g. "[Channel1]"
* @param name Name of the control e.g. "play_indicator"
* @param newValue Value to be set, normalized to a range of 0..1
*/
function setParameter(group: string, name: string, newValue: number): void;

/**
* Normalizes a specified value using the range of the given control,
* to the range of 0..1
Expand Down Expand Up @@ -112,6 +135,7 @@ declare namespace engine {
function getDefaultParameter(group: string, name: string): number;

type CoCallback = (value: number, group: string, name: string) => void
type RuntimeSharedDataCallback = (value: SharedDataValue) => void

/**
* Connects a specified Mixxx Control with a callback function, which is executed if the value of the control changes
Expand All @@ -121,9 +145,18 @@ declare namespace engine {
* @param group Group of the control e.g. "[Channel1]"
* @param name Name of the control e.g. "play_indicator"
* @param callback JS function, which will be called every time, the value of the connected control changes.
* @returns Returns script connection object on success, otherwise 'undefined''
* @returns Returns script connection object on success, otherwise 'undefined'
*/
function makeConnection(group: string, name: string, callback: CoCallback): ScriptConnection | undefined;

/**
* Register callback function to be triggered when the shared data is updated
*
* Note that local update will also trigger the callback. Make sure to make your callback safe against recursion.
* @param callback JS function, which will be called every time, the shared controller value changes.
* @returns Returns script connection object on success, otherwise 'undefined'
*/
function makeConnection(group: string, name: string, callback: CoCallback): ScriptConnection |undefined;
function makeSharedDataConnection(callback: RuntimeSharedDataCallback): ScriptConnection | undefined;

/**
* Connects a specified Mixxx Control with a callback function, which is executed if the value of the control changes
Expand Down
8 changes: 7 additions & 1 deletion src/controllers/controller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <QJSEngine>
#include <algorithm>

#include "controllers/controllershareddata.h"
#include "controllers/scripting/legacy/controllerscriptenginelegacy.h"
#include "moc_controller.cpp"
#include "util/cmdlineargs.h"
Expand Down Expand Up @@ -62,7 +63,7 @@ void Controller::stopEngine() {
m_pScriptEngineLegacy = nullptr;
}

bool Controller::applyMapping() {
bool Controller::applyMapping(std::shared_ptr<ControllerSharedData> runtimeData) {
qCInfo(m_logBase) << "Applying controller mapping...";

const std::shared_ptr<LegacyControllerMapping> pMapping = cloneMapping();
Expand All @@ -84,6 +85,11 @@ bool Controller::applyMapping() {
m_pScriptEngineLegacy->setScriptFiles(scriptFiles);

m_pScriptEngineLegacy->setSettings(pMapping->getSettings());

const auto& ns = pMapping->sharedDataNamespace();
if (!ns.isEmpty() && runtimeData != nullptr) {
m_pScriptEngineLegacy->setSharedData(runtimeData->namespaced(ns));
}
return m_pScriptEngineLegacy->initialize();
}

Expand Down
4 changes: 2 additions & 2 deletions src/controllers/controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

class ControllerJSProxy;
class ControllerScriptEngineLegacy;
class ControllerSharedData;

/// 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
Expand Down Expand Up @@ -70,7 +71,7 @@ class Controller : public QObject {
// this if they have an alternate way of handling such data.)
virtual void receive(const QByteArray& data, mixxx::Duration timestamp);

virtual bool applyMapping();
virtual bool applyMapping(std::shared_ptr<ControllerSharedData> runtimeData);
virtual void slotBeforeEngineShutdown();

// Puts the controller in and out of learning mode.
Expand Down Expand Up @@ -137,7 +138,6 @@ class Controller : public QObject {
const RuntimeLoggingCategory m_logOutput;

private: // but used by ControllerManager

virtual int open() = 0;
virtual int close() = 0;
// Requests that the device poll if it is a polling device. Returns true
Expand Down
20 changes: 17 additions & 3 deletions src/controllers/controllermanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
#include "controllers/controller.h"
#include "controllers/controllerlearningeventfilter.h"
#include "controllers/controllermappinginfoenumerator.h"
#include "controllers/controllershareddata.h"
#include "controllers/defs_controllers.h"
#include "controllers/scripting/legacy/controllerscriptenginelegacy.h"
#include "moc_controllermanager.cpp"
#include "util/cmdlineargs.h"
#include "util/compatibility/qmutex.h"
Expand Down Expand Up @@ -93,7 +95,8 @@ ControllerManager::ControllerManager(UserSettingsPointer pConfig)
// its own event loop.
m_pControllerLearningEventFilter(new ControllerLearningEventFilter()),
m_pollTimer(this),
m_skipPoll(false) {
m_skipPoll(false),
m_pRuntimeData(std::make_shared<ControllerSharedData>(this)) {
qRegisterMetaType<std::shared_ptr<LegacyControllerMapping>>(
"std::shared_ptr<LegacyControllerMapping>");

Expand Down Expand Up @@ -300,7 +303,12 @@ void ControllerManager::slotSetUpDevices() {
qWarning() << "There was a problem opening" << name;
continue;
}
pController->applyMapping();
VERIFY_OR_DEBUG_ASSERT(pController->getScriptEngine()) {
qWarning() << "Unable to acquire the controller engine. Has the "
"controller open successfully?";
continue;
}
pController->applyMapping(m_pRuntimeData);
}

pollIfAnyControllersOpen();
Expand Down Expand Up @@ -392,7 +400,13 @@ void ControllerManager::openController(Controller* pController) {
// If successfully opened the device, apply the mapping and save the
// preference setting.
if (result == 0) {
pController->applyMapping();
VERIFY_OR_DEBUG_ASSERT(pController->getScriptEngine()) {
qWarning() << "Unable to acquire the controller engine. Has the "
"controller open successfully?";
return;
}

pController->applyMapping(m_pRuntimeData);

// Update configuration to reflect controller is enabled.
m_pConfig->setValue(
Expand Down
2 changes: 2 additions & 0 deletions src/controllers/controllermanager.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class Controller;
class ControllerLearningEventFilter;
class MappingInfoEnumerator;
class LegacyControllerMapping;
class ControllerSharedData;
class ControllerEnumerator;

/// Function to sort controllers by name
Expand Down Expand Up @@ -86,4 +87,5 @@ class ControllerManager : public QObject {
QSharedPointer<MappingInfoEnumerator> m_pMainThreadUserMappingEnumerator;
QSharedPointer<MappingInfoEnumerator> m_pMainThreadSystemMappingEnumerator;
bool m_skipPoll;
std::shared_ptr<ControllerSharedData> m_pRuntimeData;
};
21 changes: 21 additions & 0 deletions src/controllers/controllershareddata.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#include <controllers/controllershareddata.h>

#include "moc_controllershareddata.cpp"

ControllerNamespacedSharedData* ControllerSharedData::namespaced(const QString& ns) {
return new ControllerNamespacedSharedData(this, ns);
}

ControllerNamespacedSharedData::ControllerNamespacedSharedData(
ControllerSharedData* parent, const QString& ns)
: QObject(parent), m_namespace(ns) {
connect(parent,
&ControllerSharedData::updated,
this,
[this](const QString& ns, const QVariant& value) {
if (ns != m_namespace) {
return;
}
emit updated(value);
});
}
67 changes: 67 additions & 0 deletions src/controllers/controllershareddata.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
#pragma once

#include <QVariant>

#include "util/assert.h"

class ControllerNamespacedSharedData;

/// ControllerSharedData is a wrapper that allows controllers script runtimes
/// to share arbitrary data via a the JavaScript interface. Controllers don't
/// access this object directly, and instead uses the
/// ControllerNamespacedSharedData wrapper to isolate a specific namespace and
/// prevent potential clash
class ControllerSharedData : public QObject {
Q_OBJECT
public:
ControllerSharedData(QObject* parent)
: QObject(parent), m_value() {
}

QVariant get(const QString& ns) const {
return m_value.value(ns);
}

/// @brief Create a a namespace wrapper that can be used by a controller.
/// The caller is owning the wrapper
/// @param ns The namespace to restrict access to
/// @return The pointer to the newly allocated wrapper
ControllerNamespacedSharedData* namespaced(const QString& ns);

public slots:
void set(const QString& ns, const QVariant& value) {
m_value[ns] = value;
emit updated(ns, m_value[ns]);
}

signals:
void updated(const QString& ns, const QVariant& value);

private:
QHash<QString, QVariant> m_value;
};

/// ControllerNamespacedSharedData is a wrapper that restrict access to a given
/// namespace. It doesn't hold any data and can safely be deleted at all time,
/// but only provide the namespace abstraction for controller to interact with
/// via a the JavaScript interface
class ControllerNamespacedSharedData : public QObject {
Q_OBJECT
public:
ControllerNamespacedSharedData(ControllerSharedData* parent, const QString& ns);

QVariant get() const {
return static_cast<ControllerSharedData*>(parent())->get(m_namespace);
}

public slots:
void set(const QVariant& value) {
static_cast<ControllerSharedData*>(parent())->set(m_namespace, value);
}

signals:
void updated(const QVariant& value);

private:
QString m_namespace;
};
11 changes: 11 additions & 0 deletions src/controllers/legacycontrollermapping.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class LegacyControllerMapping {
: m_productMatches(other.m_productMatches),
m_bDirty(other.m_bDirty),
m_deviceId(other.m_deviceId),
m_sharedDataNamespace(other.m_sharedDataNamespace),
m_filePath(other.m_filePath),
m_name(other.m_name),
m_author(other.m_author),
Expand Down Expand Up @@ -161,6 +162,15 @@ class LegacyControllerMapping {
return m_deviceId;
}

void setSharedDataNamespace(QString sharedDataNamespace) {
m_sharedDataNamespace = std::move(sharedDataNamespace);
setDirty(true);
}

const QString& sharedDataNamespace() const {
return m_sharedDataNamespace;
}

void setFilePath(const QString& filePath) {
m_filePath = filePath;
setDirty(true);
Expand Down Expand Up @@ -277,6 +287,7 @@ class LegacyControllerMapping {
bool m_bDirty;

QString m_deviceId;
QString m_sharedDataNamespace;
QString m_filePath;
QString m_name;
QString m_author;
Expand Down
6 changes: 6 additions & 0 deletions src/controllers/legacycontrollermappingfilehandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,12 @@ void LegacyControllerMappingFileHandler::addScriptFilesToMapping(

QString deviceId = controller.attribute("id", "");
mapping->setDeviceId(deviceId);
// Empty namespace is forbidden. If a controller wants to use shared data,
// they must specify an non-empty string.
QString sharedDataNamespace = controller.attribute("namespace", "");
if (!sharedDataNamespace.isEmpty()) {
mapping->setSharedDataNamespace(deviceId);
}

// See TODO in LegacyControllerMapping::DeviceDirection - `direction` should
// only be used as a workaround till the bulk integration gets refactored
Expand Down
4 changes: 2 additions & 2 deletions src/controllers/midi/midicontroller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@ bool MidiController::matchMapping(const MappingInfo& mapping) {
return false;
}

bool MidiController::applyMapping() {
bool MidiController::applyMapping(std::shared_ptr<ControllerSharedData> runtimeData) {
// Handles the engine
bool result = Controller::applyMapping();
bool result = Controller::applyMapping(std::move(runtimeData));

// Only execute this code if this is an output device
if (isOutputDevice()) {
Expand Down
3 changes: 2 additions & 1 deletion src/controllers/midi/midicontroller.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "controllers/softtakeover.h"

class MidiOutputHandler;
class ControllerSharedData;

class MidiInputHandleJSProxy final : public QObject {
Q_OBJECT
Expand Down Expand Up @@ -81,7 +82,7 @@ class MidiController : public Controller {
void slotBeforeEngineShutdown() override;

private slots:
bool applyMapping() override;
bool applyMapping(std::shared_ptr<ControllerSharedData>) override;

void learnTemporaryInputMappings(const MidiInputMappings& mappings);
void clearTemporaryInputMappings();
Expand Down
4 changes: 4 additions & 0 deletions src/controllers/scripting/controllerscriptenginebase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -218,3 +218,7 @@ void ControllerScriptEngineBase::errorDialogButton(
void ControllerScriptEngineBase::throwJSError(const QString& message) {
m_pJSEngine->throwError(message);
}

void ControllerScriptEngineBase::setSharedData(ControllerNamespacedSharedData* runtimeData) {
acolombier marked this conversation as resolved.
Show resolved Hide resolved
m_runtimeData = std::unique_ptr<ControllerNamespacedSharedData>(runtimeData);
}
Loading