QML-based components API for controllers #13459

christophehenry · 2024-07-10T11:48:41Z

Swiftb0y

this is roughly going into the direction I'm thinking. Keep it up.

Swiftb0y · 2024-07-10T11:53:43Z

res/controllers/Denon-DN-S3700.qml

@@ -0,0 +1,17 @@
+import QtQuick


we don't actually want QtQuick (since thats the GUI framework). You instead just want the language basics QtQml. You may also add a version (not sure whats appropriate, in the preexisting qml files we have in mixxx we use 2.12, but thats quite old (I think from Qt 5.12 still)).

Suggested change

import QtQuick

import QtQml

christophehenry · 2024-07-11T21:34:12Z

Ok, so the QML loading logic is really entangled with the screen handling logic. So now I'm thinking: Why not dropt the XML entirely and handle all the metadata in QML? That could be done using a MixxxController QML component. Then, when we detect and parse a QML file in controllers, if it contains a single MixxxController, then it's a controller, execute it!

src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp

JoergAtGithub · 2024-07-11T22:06:26Z

So now I'm thinking: Why not drop the XML entirely and handle all the metadata in QML?

That should be the target! One semantic syntax, with use of a programming language, where it is not possible to express it in general semantic language.

Important is, that our MIDI learning wizard can read all necessary information from this semantic information.

Swiftb0y · 2024-07-12T09:43:46Z

The potential problem I have with that is that loading a QML module just to read its metadata may be too slow considering you need to parse >100 mappings on startup. I also don't like that it couples the metadata to a particular execution engine, making it impossible to easily try out new execution engines. The biggest issue is that there is no public QML (de-)serialization library. So manipulating the document as it would be required for a controller mapping wizard would not be possible. All in all, I'm not in favor of dropping the separate metadata manifest for now.

christophehenry · 2024-07-12T10:12:21Z

What if We create a custom C++ class? That should be well faster, no? All QML object have a C++ class counterpart, I can't believe Qt would release a new language if its heavy to parse.

Edti: Also @Swiftb0y, you said on Zulip:

we'd need to generate type definitions from the C++ APIs, etc

Maybe this can be the occasion to do so. Can someone point me in the correct direction to do it?

Swiftb0y · 2024-07-12T10:36:09Z

I can't believe Qt would release a new language if its heavy to parse.

Sure, we can't be sure until we measured, but the other two concerns I've voiced still apply.

we'd need to generate type definitions from the C++ APIs, etc

Maybe this can be the occasion to do so. Can someone point me in the correct direction to do it?

Not sure, what you trying to achieve? I think these type definitions don't give us much more than better editor integration.

Edit: if you still want to explore that, from my limited research, you'll want to generate a qmldir file, which could be done automatically using CMake.

JoergAtGithub · 2024-07-12T11:03:53Z

The parsing of the mappings at every startup (we do it 2x at for each XML file at each startup btw.) seems to be wrong to me anyway. As we need only very few data from these files at startup.
I think we should do this only at the very first startup, and then store the few needed values in a global file or database. If we have cached this information, we just need to check for modified files at the next startup.

Swiftb0y · 2024-07-12T11:08:33Z

Sure... filehash should be sufficient to determine if something changed. But thats a separate topic IMO.

JoergAtGithub · 2024-07-12T11:37:16Z

Since it would solve all the performance related concerns, it should be addressed first (not in this PR of cause).
Then we can define the new QML mapping syntax, without restrictions from the legacy implementation.

Swiftb0y · 2024-07-12T12:18:55Z

sure... but again, performance wasn't my main concern. It was actually the fact that

we can't make the metadata parsing independent from a QMLEngine and
QML documents are essentially read-only. If we want to manipulate its content persistently (like its necessary for a mapping wizard), we would need to serialize it into a separate file in a format that we can actually manipulate.

daschuer · 2024-07-13T11:53:45Z

It looks like we have conflicting or unclear requirements here. We need to decide the way forward to not end up with a blocked PR or double work.
We need to decide which of them are mandatory optional.

Let's summarize (please correct me):

User Requirements

Good performance, responsive scratching (Real-Time)
Easy reassign buttons and sliders
Easy map a new controller from the scratch

Mapping Developer

Everything at one place, one file, folder, pack
Easy to share
Reuse common programming skills
High abstraction level
Complete API documentation
Good IDE support .. code completion.
On the fly edits, no Mixxx restart.
Stable: Compatible to future Mixxx versions

Did I miss something? Please complete.

we can't make the metadata parsing independent from a QMLEngine

That is correct if we use only one parser. I can image to workaround that by a stripped Mixxx QML parser. reading a file hash, is in this context the minimum version of such parser, but we can read also other info.

We would need to serialize it into a separate file in a format that we can actually manipulate.

I can imagine QT does exactly that. Maybe we can copy from them and use it for our second parseing.

The other topic is how the data stream is handled. I like to keep the connection factory and the dispatcher in the C++ domain, controlled by QML XML or JS to share concepts between the mapping types. I can also imagine to read the mapping by XML and write them as QML or such for an easy transition.

Another point that bugs me is the weak scratching stability and the programming skills required to map the jogg wheel. Both can be targeted by handling the jog in wheel in the C++ domain and configure it from QML XML or JS.

Does that make sense?

Swiftb0y · 2024-07-13T20:11:06Z

I'm sorry but I don't think it makes sense to discuss this feature further in text form. The problem at hand is simply to complicated and widening the scope by bringing discussing the metadata format and currently irrelevant features such as scratching does not help! I will try to find some time and write down my thoughts in a sorted fashion for https://github.com/mixxxdj/proposals/.
@christophehenry I would advise you to pause any work on this for now. We can resume once we've resolved the "too many cooks in the kitchen" problem brewing right now.

christophehenry · 2024-07-13T20:27:18Z

It's better if I continue to work on this, which has the benefit to familiarise myself with the code.

Swiftb0y · 2024-07-13T20:57:42Z

Sure... just be prepared that it probably won't get merged until we all agree on a design.

christophehenry · 2024-07-13T21:12:15Z

Or else: as a starting point, I can keep the XML and just design a rough QML API based on componentsJS. Then we can iterate and refine over this.

Then only, we can discuss dropping the XML. Is that acceptable?

Swiftb0y · 2024-07-14T09:11:51Z

Or else: as a starting point, I can keep the XML

Yeah. I would keep the XML for now. Changing the metadata format is orthogonal to a potential QML API.

and just design a rough QML API based on componentsJS.

I don't think it makes sense to prototype a QML API outside a QML environment since the fact that that reactive bindings are available would change the API a lot. Or am I misunderstanding you here?

christophehenry · 2024-07-14T19:46:30Z

I mean: I will keep the main design concepts of ComponentsJS to design the QML API, changing a few things for more flexibility.

Swiftb0y · 2024-07-14T20:48:10Z

Yes, that would work. I think the main challenge is that with QML, we would need to insert a layer that adapts between the explicit message sending model to the implicit reactive binding model. For instance we would need to differentiate between when a binding was triggered (which can happen pretty much spuriously in a reactive system) for no good reason and when we actually do want to send a message. For that, I would introduce the concept of idempotence (each component should be by default idempotent, meaning sending the same message again would not introduce a visual difference and thus spontanous messages that are the same as the one before can be ignored). In the long-term I also want to experiment with abstracting the MIDI-onlyness out of the components, but thats a slightly different topic, though the idempotence concept would be added to the midi part of that abstraction).

TLDR; in order to properly work with reactive bindings and reduce overhead, components should swallow duplicate messages by default (inbound and outbound; separably configurable). Does that make sense?

daschuer · 2024-07-15T07:49:16Z

I took the momentum and put my post into a proposals document:
mixxxdj/proposals#6

I think we need a separate documents to define single aspects. The requirements document shall make sure that all changes contributes to a common goal. From this point of view @christophehenry can carry on implementing a POC and also keeps his momentum.

Does the current list of requirements match your ideas by the way?

TLDR; in order to properly work with reactive bindings and reduce overhead, components should swallow duplicate messages by default (inbound and outbound; separably configurable). Does that make sense?

That does not always work, because we do not always receive a release event to track double tabs. I think the CO system already covers all aspects we need to consider with these reactive bindings.
This is controlled by

mixxx/src/control/controlobject.cpp

Line 13 in d61841c

bool bIgnoreNops,

Swiftb0y · 2024-07-15T10:00:00Z

I think we need a separate documents to define single aspects.

I fully agree.

Does the current list of requirements match your ideas by the way?

Not yet. I will prepare a proposal as well today.

That does not always work, because we do not always receive a release event to track double tabs.

Yes, I'm aware that it won't always work, but it should in the majority of cases and it should be easy to turn of on a per-component basis. I also don't think relying on / referring to COs works here because components may not always be bound to components. It also doesn't always work (for example the track_loaded CO will get signaled when loading a new track but its value doesn't change, thus there is also no reason to send a midi message if the action on the controller is idempotent (eg just controlling a feedback led)). Anyways, this is going far too deep into unnecessary detail again.

christophehenry · 2024-07-15T13:04:46Z

So following r1677677964, I propose we drop <screens> from the XML in favor of declaring them in MixxxController. From a user standpoint, this can be done like this :

MixxxController {
    screens: {
        screenId: Item {
          …
        } 
    } 
}

Or using a dedicated class like this:

MixxxController {
    screens: [
        Screen {
            screenId: …
            Item {
                … 
            } 
        } 
    ] 
}

I'm rather in favor of a dedicated class so that all the screen-related code can be extracted from controllerscriptenginelegacy.cpp into this new class.

Edit: or even:

MixxxController {
    Screen {
        screenId: …
        Item {
            … 
        } 
    } 
}

I think it's possible using a signal on screenId to bind the screen on instanciation.

christophehenry · 2024-07-15T13:24:24Z

res/controllers/Denon-DN-S3700.qml

+    function init() {
+        console.error(controller.controllerId, controller.debugMode);
+    }
+    function shutdown() {
+        console.error(`Shutting down ${controller.controllerId} with debug mode ${controller.debugMode}`);
+    }


Actually… Is that even needed? In QML, there's Component.onCompleted and Component.onDestruction.

These hooks will be invoked on component instantiation and destruction. Assuming this was inspired from the existing screen rendering, init and shutdown are called at different time.

init is called once the controller has started. It is the direct equivalent to the same hook in Javascript. Unlike the constructor (equivalent to Component.onCompleted), it is invoked once the engine is running.

shutdown is called when the engine is about to shutdown. This allows the controller to perform some animation or screen clean up, which is required for some devices (e.g restoring the splash screen, playing a shutdown animation, clearing the screen to a static colour, ...). In case of Component.onDestruction, as the name suggest, you don't have a component to render anymore.

In case of "pure" controller (without UI rendering), I think it makes sense to ditch these function, but if we want to eventually merge the engines together, they will have to remain.
I believe you might be able to replace them with signals instead tho (e.g MixxxController::onStart, MixxxController::onShutdown)

acolombier

I would suggest you use the Dummy Device Screen.hid.xml (Doc for Linux) provided as part of the code to make sure you aren't breaking the UI rendering. I haven't tested it yet, but from a quick code review, I believe there is a few things that won't work.

acolombier · 2024-07-15T17:14:53Z

res/controllers/Denon-DN-S3700.qml

+    function init() {
+        console.error(controller.controllerId, controller.debugMode);
+    }
+    function shutdown() {
+        console.error(`Shutting down ${controller.controllerId} with debug mode ${controller.debugMode}`);
+    }


These hooks will be invoked on component instantiation and destruction. Assuming this was inspired from the existing screen rendering, init and shutdown are called at different time.

init is called once the controller has started. It is the direct equivalent to the same hook in Javascript. Unlike the constructor (equivalent to Component.onCompleted), it is invoked once the engine is running.

shutdown is called when the engine is about to shutdown. This allows the controller to perform some animation or screen clean up, which is required for some devices (e.g restoring the splash screen, playing a shutdown animation, clearing the screen to a static colour, ...). In case of Component.onDestruction, as the name suggest, you don't have a component to render anymore.

In case of "pure" controller (without UI rendering), I think it makes sense to ditch these function, but if we want to eventually merge the engines together, they will have to remain.
I believe you might be able to replace them with signals instead tho (e.g MixxxController::onStart, MixxxController::onShutdown)

acolombier · 2024-07-15T17:22:29Z

src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp

+        QListIterator<std::shared_ptr<mixxx::qml::MixxxController>> controllers(m_mixxxController);
+        bool controllersSuccess = true;
+        while (controllers.hasNext()) {


Please use a for-loop instead

Suggested change

QListIterator<std::shared_ptr<mixxx::qml::MixxxController>> controllers(m_mixxxController);

bool controllersSuccess = true;

while (controllers.hasNext()) {

bool controllersSuccess = true;

for (auto controller: m_mixxxController) {

acolombier · 2024-07-15T17:25:12Z

src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp

+    QFile file = QFile(qmlScript.file.absoluteFilePath());
+    if (!file.exists()) {
+        qCWarning(m_logger) << "Unable to load the QML file:" << qmlScript.file.absoluteFilePath()


I believe scene is the right terminology. I would rather we keep it instead of the cryptic file

acolombier · 2024-07-15T17:26:56Z

src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp

@@ -863,34 +867,40 @@ bool ControllerScriptEngineLegacy::evaluateScriptFile(const QFileInfo& scriptFil
 }

 #ifdef MIXXX_USE_QML
-std::shared_ptr<QQuickItem> ControllerScriptEngineLegacy::loadQMLFile(
+bool ControllerScriptEngineLegacy::instanciateQMLComponent(


I'm not sure moving the scene binding logic in this function helps much to be honest. Perhaps you should move the logic from line 460 to a new function, but I would suggest not making loadQMLFile more complex.

acolombier · 2024-07-15T17:27:59Z

src/controllers/legacycontrollermappingfilehandler.cpp

+            QString identifier = scriptFile.attribute(
+                    "identifier", scriptFile.attribute("functionprefix", ""));


In case of the screens, it makes no sense to default to functionprefix

christophehenry · 2024-07-15T21:39:20Z

I believe there is a few things that won't work.

I'm pretty sure there is a few things that won't work. This is me right now:

But I believe the screens code can be greatly simplified if separated in a QML component which is what I started to do. There's no logic yet bit res/controllers/Denon-DN-S3700.qml shows how it would look like. I'd like your opinion. @JoergAtGithub and @Swiftb0y too.

acolombier · 2024-07-15T22:19:07Z

I'm not sure this will change much since there is close to no logic with the root item (thus why interpreted as a QQuickItem)
One gain you may get is the hooks to be replaced to signal when asynchronous, and nicer callback for the transform function, which may simplify the whole method discovery.
I'm not sure there is much to simplify on the scene binding and rendering engine init.
It's worth to point out that, as it stands, the QML scene cannot be used to perform interactions with the controller, other than UI rendering. This is due to the fact the rendering is throttled to a certain (max) rate, but also cannot be done in the controllers thread since this would impact performance, so latency.

The reason why all of this is like this is... legacy :)

I would suggest to watch the proposals currently getting discussed, as I'm not entirely sure it makes sense to make QML support in the legacy engine, since there is a lot of limitations there, and it is likely not going to yield to great result because of those limitations.

christophehenry · 2024-07-16T05:44:42Z

This is due to the fact the rendering is throttled to a certain (max) rate, but also cannot be done in the controllers thread since this would impact performance, so latency.

Damn. I was hoping to move that code into the MixxxScreen class…

christophehenry · 2024-07-16T08:35:25Z

Uh… Can I have some help on the build error?

Swiftb0y · 2024-07-16T09:33:35Z

src/qml/mixxxscreen.h

+#include <QImage>
+#include <QObject>
+#include <QSize>
+#include <QtQmlIntegration>


this header doesn't exist. Where did you get this from?

I don't remember. It provides QML_ELEMENT. But it appears <QtQml/qqmlregistration.h> does the same.

It is described here.

Ah, I think QtQmlIntegration is only a folder containing more headers. I think QtQml/qqmlregistration.h is better.

christophehenry · 2024-07-16T13:37:42Z

CMakeLists.txt

@@ -2776,6 +2776,9 @@ if(QML)
    src/qml/qmlplayerproxy.cpp
    src/qml/qmlvisibleeffectsmodel.cpp
    src/qml/qmlwaveformoverview.cpp
+    src/qml/mixxcomponentlifecycle.cpp


I'm not sure this here is useful. MixxxComponentLifecycle has no implementation since it's an interface but compilation fails without it.

yeah, I think thats due to the MOC. Just keep it.

…xxScreen

christophehenry · 2024-07-19T12:59:13Z

src/qml/mixxxscreen.cpp

+    if (typedIdx >= 0) {
+        auto transformMethod = meta->method(typedIdx);
+        if (!transformMethod.isValid()) {
+            // TODO


How should I handle logging here?

How do I get the engine from a QML element? I can see that QQuickView has an engine() which I suppose is because it takes the QQmlEngine has ctor argument but I don't understand where it gets this argument from. In my code, if I try put any other ctor than QObject's default, element stops being creatable.

christophehenry · 2024-07-19T13:00:46Z

src/qml/mixxxscreen.cpp

+            transform = [transformMethod, this](const QByteArray input,
+                                const QDateTime& timestamp) -> QVariant {
+                return transform(transformMethod, input, timestamp, true);
+            };


This should be a bit more efficient than previous implementation since transform function is searched only once trhough relfection. But then again, I know very few C++ so I may awefully wrong.

christophehenry · 2024-07-19T14:56:12Z

src/qml/mixxxscreen.h

+    // Transform function
+    std::function<QVariant(const QByteArray, const QDateTime&)> transform =
+            [](const QByteArray input, const QDateTime& timestamp) {
+                return QVariant(input);


I'd better use std::expected here with an error type enum instead here.

…rmFrame and call method

christophehenry · 2024-07-26T17:28:19Z

src/qml/mixxxscreen.h

+    };
+    Q_ENUM(ColorEndian)
+
+    explicit MixxxScreen(MixxxController* parent = nullptr)


So I did the test and parent here is nullptr. How can I force a QML component to be instanciated inside a MixxxController.

… components

christophehenry · 2024-07-26T19:47:10Z

So code in the last commit is not good, but it's a glance of the solution I have in mind to allow MixxxScreens to declare themselves to the ScriptControllerEngine. The basic idea is that ScriptControllerEngine exposes a small API via an interface to the QML components that they can retrieve via the QQmlEngine.

If someone has a more elegant solution, I take it.

christophehenry · 2024-11-21T18:08:07Z

Let's put this on pause for now.

github-actions bot added the controller mappings label Jul 10, 2024

Swiftb0y reviewed Jul 10, 2024

View reviewed changes

christophehenry mentioned this pull request Jul 10, 2024

ES6+ ComponentsJS #13437

Closed

QML-based components API for controllers

b79fcbd

Dirty but working

39d2020

christophehenry force-pushed the qml-components-api branch from d051ebb to 39d2020 Compare July 11, 2024 21:44

github-actions bot added the controller backend label Jul 11, 2024

christophehenry commented Jul 11, 2024

View reviewed changes

src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp Outdated Show resolved Hide resolved

MixxxController QML component

c4eba93

github-actions bot added build qml labels Jul 15, 2024

Refactor QML instanciation

2a03ebf

christophehenry force-pushed the qml-components-api branch from 6bbc49b to 2a03ebf Compare July 15, 2024 12:16

christophehenry commented Jul 15, 2024

View reviewed changes

acolombier reviewed Jul 15, 2024

View reviewed changes

Implement screens

edd2254

christophehenry force-pushed the qml-components-api branch from c751ebf to edd2254 Compare July 15, 2024 21:13

Swiftb0y reviewed Jul 16, 2024

View reviewed changes

christophehenry force-pushed the qml-components-api branch 3 times, most recently from 51fea62 to 1d248a2 Compare July 16, 2024 11:46

christophehenry commented Jul 16, 2024

View reviewed changes

Transform component's init and shutdown to signals

2e34045

christophehenry force-pushed the qml-components-api branch from 1d248a2 to 2e34045 Compare July 18, 2024 16:07

Start moving scren rendering from ControllerScriptEngineLegacy to Mix…

c469ca4

…xxScreen

christophehenry commented Jul 19, 2024

View reviewed changes

christophehenry added 2 commits July 22, 2024 09:08

Some fixes

111031c

Change MixxxScreen::tranform from JS function to MixxxScreen::transfo…

8931375

…rmFrame and call method

christophehenry commented Jul 26, 2024

View reviewed changes

API interface exposing some ControllerScriptEngineBase methods to QML…

33b7649

… components

acolombier mentioned this pull request Oct 8, 2024

feat: improve screen rendering framework #13737

Merged

christophehenry closed this Nov 21, 2024

		QString identifier = scriptFile.attribute(
		"identifier", scriptFile.attribute("functionprefix", ""));

QML-based components API for controllers #13459

QML-based components API for controllers #13459

Conversation

christophehenry commented Jul 10, 2024 • edited Loading

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophehenry commented Jul 11, 2024

JoergAtGithub commented Jul 11, 2024

Swiftb0y commented Jul 12, 2024

christophehenry commented Jul 12, 2024 • edited Loading

Swiftb0y commented Jul 12, 2024 • edited Loading

JoergAtGithub commented Jul 12, 2024

Swiftb0y commented Jul 12, 2024

JoergAtGithub commented Jul 12, 2024

Swiftb0y commented Jul 12, 2024

daschuer commented Jul 13, 2024

Swiftb0y commented Jul 13, 2024

christophehenry commented Jul 13, 2024

Swiftb0y commented Jul 13, 2024

christophehenry commented Jul 13, 2024

Swiftb0y commented Jul 14, 2024

christophehenry commented Jul 14, 2024

Swiftb0y commented Jul 14, 2024 • edited Loading

daschuer commented Jul 15, 2024

Swiftb0y commented Jul 15, 2024

christophehenry commented Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolombier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophehenry commented Jul 15, 2024

acolombier commented Jul 15, 2024

christophehenry commented Jul 16, 2024

christophehenry commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophehenry Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophehenry commented Jul 26, 2024

christophehenry commented Nov 21, 2024

christophehenry commented Jul 10, 2024 •

edited

Loading

christophehenry commented Jul 12, 2024 •

edited

Loading

Swiftb0y commented Jul 12, 2024 •

edited

Loading

Swiftb0y commented Jul 14, 2024 •

edited

Loading

christophehenry commented Jul 15, 2024 •

edited

Loading

christophehenry Jul 16, 2024 •

edited

Loading