Create JavascriptPlayerProxy by christophehenry · Pull Request #14314 · mixxxdj/mixxx

christophehenry · 2025-02-09T10:37:40Z

Continuation of #14175.

ronso0 · 2025-02-10T09:46:39Z

FYI the corresponding Zulip convo is
https://mixxx.zulipchat.com/#narrow/channel/109171-development/topic/Expose.20track.20informations.20to.20controllers

cr7pt0gr4ph7 · 2025-02-10T11:16:45Z

FYI the corresponding Zulip convo is https://mixxx.zulipchat.com/#narrow/channel/109171-development/topic/Expose.20track.20informations.20to.20controllers

Thanks, I missed that! Still getting used to how the communication is spread across the different channels.

Swiftb0y · 2025-02-10T14:57:02Z

The final API might look totally different. For example, based on the current design pattern of the Engine JS API, the following design could make sense:

Thanks for chirping in @cr7pt0gr4ph7. I appreciate your suggestion but IMO it has also been discussed previously and rejected (See the corresponding zulip thread). The engine design is plagued of bad legacy decisions (decisions that needed to be made back then because either the current mixxx devs didn't know any better or Qt didn't give them any better choice). This is supposed to signify a break and a potential alternative to overcome that. engine.getString just continues that bad design legacy.

cr7pt0gr4ph7 · 2025-02-11T11:40:33Z

Thanks for chirping in @cr7pt0gr4ph7. I appreciate your suggestion but IMO it has also been discussed previously and rejected (See the corresponding zulip thread).

I wasn't aware of the Zulip discussion when I wrote that comment. I've hidden my comment and added a note at the top to avoid future confusion, sorry for the noise. I'll continue the discussion w.r.t. API design over at Zulip.

cr7pt0gr4ph7 · 2025-02-11T11:41:01Z

Additionally, I'd also like to make the proposal of preactively linking Zulip discussions in GitHub issues and PRs for future cross-reference when these contain highly relevant discussion, such as here ;)

acolombier

This is looking pretty much good to go IMO!
Only thing is that it would be worth to combining our effort with #14516, which break the dependencies between the player and a track (quick TL;DR on why this is needed - QML needs a way to interact with track that aren't loaded on player). Note that this change also removes all the waveform left over bits, that are now irrelevant thanks to the rendergraph waveform library, and this should help simplifying your PR.

Also, it would be great if you could put typing instructions in res/controllers/engine-api.d.ts and also provide a small example on how this is intended to be used. Also, feel free to implement a basic usage in a controller mapping you feel relevant just to help with understanding and testing (just make sure to use a separate commit for it for better change control)

christophehenry · 2025-04-19T13:55:15Z

@acolombier Since you're moving most of QmlPlayerProxy's code into QmlTrackProxyin #14516, would you agree to create the read-only JavascriptTrackProxy?

JoergAtGithub · 2025-04-19T14:53:06Z

The Deck/Player might be better parent object here than the track, as it could provide additional information that depend on playposition or rate, like the formated actual key string, which is often used in jogwheel displays.

christophehenry · 2025-04-19T17:32:34Z

Ok so we're back to defining a wrapper around QmlTrackProxy+QmlPlayerProxy then?

acolombier · 2025-04-19T18:47:52Z

The Deck/Player might be better parent object here than the track, as it could provide additional information that depend on playposition or rate

The idea with the split is to allow both. Player (which can be a deck, sampler or preview) will have playing information (rate, position, ...) as well current loaded track, which itself will have its own information, unrelated to player.

JoergAtGithub · 2025-04-19T19:24:51Z

Ok so we're back to defining a wrapper around QmlTrackProxy+QmlPlayerProxy then?

This code is fine, it just needs to be adjusted as described by @acolombier above!

christophehenry · 2025-04-20T11:21:28Z

Well @acolombier's PR splits the code in two classes so I don't really understand how this code fits.

christophehenry · 2025-05-23T07:14:58Z

I rebased this PR and added the implementation.

acolombier · 2025-05-23T08:06:42Z

+    // Don't set a parent here, so that the QML engine deletes the object when
+    // the corresponding JS object is garbage collected.
+    JavascriptPlayerProxy* pPlayerProxy = new JavascriptPlayerProxy(
+            s_pPlayerManager->getPlayer(deck), nullptr);


PlayerManager::getPlayer will return a null ptr if deck doesn't exist, could you please handle that gracefully and return a QJSEngine::newErrorObject?

fyi, this a new pattern where our API would return an error. Usually it just returns undefined (which would be the case with a nullptr). Since we don't really return different errors at the moment, I would prefer to stick with the consistency of undefined. Wdyt?

Happy with the middle ground of returning null if we think this is best. I think we shouldn't return an invalid JavascriptPlayerProxy tho

Yes I changed that in the last commit.

Can I resolve this so it's clearer for me what's left?

acolombier · 2025-05-23T08:13:42Z

+    Q_PROPERTY(bool isLoaded READ isLoaded NOTIFY trackChanged)
+    Q_PROPERTY(QString artist READ getArtist NOTIFY artistChanged)
+    Q_PROPERTY(QString title READ getTitle NOTIFY titleChanged)
+    Q_PROPERTY(QString album READ getAlbum NOTIFY albumChanged)
+    Q_PROPERTY(QString albumArtist READ getAlbumArtist NOTIFY albumArtistChanged)
+    Q_PROPERTY(QString genre READ getGenre STORED false NOTIFY genreChanged)
+    Q_PROPERTY(QString composer READ getComposer NOTIFY composerChanged)
+    Q_PROPERTY(QString grouping READ getGrouping NOTIFY groupingChanged)
+    Q_PROPERTY(QString year READ getYear NOTIFY yearChanged)
+    Q_PROPERTY(QString trackNumber READ getTrackNumber NOTIFY trackNumberChanged)
+    Q_PROPERTY(QString trackTotal READ getTrackTotal NOTIFY trackTotalChanged)


I'm wondering if it is necessary to implement NOTIFY here. Since this code won't be used by QML (as it has its own implementation), there is no need to provide this to support property binding. This will allow you to remove most of the signals and reduce the code duplication with src/qml/qmltrackproxy.h

You mean for this property or all of them? I think NOTIFY is useful. In combination with #13935, it allows the following:

engine.getPlayer("deck1").titleChanged.connect(newTitle => { displayTitle( engine.convertCharset(engine.WellKnownCharsets.Latin1, newTitle) ) })

I could even add a convenience function to ComponentsJS

Yup, we should def also use signals from JS. Otherwise we'll need yet another makeConnection like facility or people will abuse (likely only tangentially related) COs instead.

Lets split any ComponentsJS changes to another PR though

Ah sorry, I didn't see that this will actively be supported in the script. This is looking good then. The duplication remains a bit of a bummer IMO, but don't want to block this further so if this isn't a problem for other member, happy to ignore!

Can I resolve this so it's clearer for me what's left?

Swiftb0y · 2025-05-23T13:21:04Z

+    // Don't set a parent here, so that the QML engine deletes the object when
+    // the corresponding JS object is garbage collected.
+    JavascriptPlayerProxy* pPlayerProxy = new JavascriptPlayerProxy(
+            s_pPlayerManager->getPlayer(deck), nullptr);


fyi, this a new pattern where our API would return an error. Usually it just returns undefined (which would be the case with a nullptr). Since we don't really return different errors at the moment, I would prefer to stick with the consistency of undefined. Wdyt?

JoergAtGithub · 2025-05-29T10:43:37Z

TypeScript declaration is LGTM now too! Thank you!
The test is the last missing piece now!

Swiftb0y · 2025-06-17T09:23:24Z

friendly ping @christophehenry

christophehenry · 2025-06-17T09:52:55Z

@Swiftb0y Did you mean to ping @JoergAtGithub? I'm waiting for their input on how to load a track to the player and test the slots.

Swiftb0y · 2025-06-17T10:51:15Z

Ah, I see. I didn't know where we were. Then this is indeed a friendly reminder @JoergAtGithub

christophehenry · 2025-06-17T12:37:22Z

Ah I see @JoergAtGithub opened christophehenry#3. I don't why I didn't get notified… I may be able to progress this week.

…pping_validation_test and controllerscriptenginelegacy_test executable without crash

christophehenry · 2025-06-20T13:00:21Z

Alright! Tests done!

Edit: ah… I should add a test for the trackUnloaded slot too. I'll do tonight. In the meantime, don't hesitate to leave your comments!

christophehenry · 2025-06-20T13:06:45Z

+    for (auto [key, value] : expectedValues.asKeyValueRange()) {
+        EXPECT_QSTRING_EQ(value, qjsvalue_cast<QString>(evaluate(QString("player.%1").arg(key))));
+        EXPECT_QSTRING_EQ(value, qjsvalue_cast<QString>(evaluate(QString("result.%1").arg(key))));
+    }


Is there a GoogleTest equivalent to Python's unittest.TestCase.subTest? Couldn't find one.

Not really. The decoding test has some code that tries to accomplish something similar, but I don't want you to reuse that. Looking at your test code however, I think you'll want to write a Value-Parameterized Test.

I think I'll just go with a custom failure message.

thats fine too, though a parametrized test wouldn't really be that much complicated IMO.

christophehenry · 2025-06-20T17:20:10Z

+        while (deck->getEngineDeck()->getEngineBuffer()->isTrackLoaded()) {
+            QTest::qSleep(100);
+        }


Sadly, this doesn't work. The test just hangs. If I execute this in the debugger with breakpoints, deck->slotEjectTrack(1.0) correctly ejects the track and the test passes. So I'm not correctly waiting here. There's a concurrency problem. I'd accept any help on how to solve this.

Did you try to eject from JavaScript using engine.setValue('[Channel1]', 'eject', 1); ?

I admit I didn't have that idea. Lemme try.
Edit: @JoergAtGithub hmm… This solution seems to not even eject the track. Even in the debugger, the event aren't fired :thinking:

I would be fine if the testcase doens't check the eject case.

Alright, deal.

maybe also processEvents() in the busy-wait loop?

Sadly no, that still hangs or not even eject, depending on the solution. I tried a number of combinations around this.

You probably need to call it twice, as in other tests:

mixxx/src/test/controlobjectscripttest.cpp

Lines 85 to 91 in 14c19a5

void processEvents() {

// Calling processEvents() twice ensures that at least all queued and

// the next round of emitted events are processed.

// Test fails occurred here for a local debug build on Linux, but not on CI (see https://github.com/mixxxdj/mixxx/pull/4588)

application()->processEvents();

application()->processEvents();

}

I called the processEvents of the test class which already does that. But it doesn't make a difference.

JoergAtGithub

LGTM! Thank you for your contribution and your patience!

daschuer · 2025-06-21T00:51:28Z

After merging this Mixxx crashed during shutdown: See #14982

Eve00000 · 2025-06-21T06:05:46Z

Congratulations for the work and perseverance.
(Too bad for the crash on exit now)

christophehenry · 2025-06-21T06:25:29Z

Awesome! It's the last piece I needed to propose an official DN-S3700 controller ❤️

github-actions Bot added build qml controller backend labels Feb 9, 2025

christophehenry force-pushed the JavascriptPlayerProxy branch 3 times, most recently from 9dc994c to ca4c97c Compare February 9, 2025 11:22

This comment was marked as outdated.

Sign in to view

christophehenry closed this Feb 12, 2025

christophehenry reopened this Mar 7, 2025

acolombier reviewed Apr 19, 2025

View reviewed changes

Comment thread src/qml/qmlplayerproxy.cpp Outdated

christophehenry force-pushed the JavascriptPlayerProxy branch from ca4c97c to b7ee0c3 Compare May 23, 2025 07:06

acolombier reviewed May 23, 2025

View reviewed changes

christophehenry force-pushed the JavascriptPlayerProxy branch 2 times, most recently from cabd6b9 to 217ecee Compare May 23, 2025 12:15

github-actions Bot added the controller mappings label May 23, 2025

Swiftb0y reviewed May 23, 2025

View reviewed changes

christophehenry commented May 24, 2025

View reviewed changes

Comment thread src/controllers/scripting/javascriptplayerproxy.h Outdated

acolombier reviewed May 24, 2025

View reviewed changes

Comment thread src/coreservices.cpp

christophehenry force-pushed the JavascriptPlayerProxy branch from e750320 to d960c0a Compare May 29, 2025 10:38

Swiftb0y mentioned this pull request Jun 17, 2025

Expose track data to javascript #6898

Closed

Made JS code containing the new engine.getPlayer API in controller_ma…

7b116a4

…pping_validation_test and controllerscriptenginelegacy_test executable without crash

github-actions Bot added the code quality label Jun 19, 2025

JoergAtGithub added this to the 2.7-beta milestone Jun 19, 2025

christophehenry commented Jun 20, 2025

View reviewed changes

christophehenry force-pushed the JavascriptPlayerProxy branch from fbd30fb to c6f0e2d Compare June 20, 2025 17:17

christophehenry commented Jun 20, 2025

View reviewed changes

christophehenry requested a review from JoergAtGithub June 20, 2025 17:44

christophehenry force-pushed the JavascriptPlayerProxy branch 2 times, most recently from 88fa23b to 9f525d5 Compare June 20, 2025 18:29

Add tests for JavascriptPlayerProxy API

80e959a

christophehenry force-pushed the JavascriptPlayerProxy branch from 9f525d5 to 80e959a Compare June 20, 2025 20:50

JoergAtGithub approved these changes Jun 20, 2025

View reviewed changes

JoergAtGithub merged commit 07bcb86 into mixxxdj:main Jun 20, 2025
3 checks passed

ronso0 mentioned this pull request Jun 20, 2025

Add controls for loaded track information #14641

Closed

daschuer mentioned this pull request Jun 21, 2025

Main: Crash during shutdown with QML=OFF #14982

Closed

christophehenry deleted the JavascriptPlayerProxy branch June 21, 2025 02:27

JoergAtGithub mentioned this pull request Sep 26, 2025

Implement comprehensive UTF-8 string control system #15416

Closed

mxmilkiib mentioned this pull request Oct 24, 2025

hotcue label get n set controls? #14138

Open

JoergAtGithub mentioned this pull request Dec 2, 2025

Controller Shared Data: Initial Proposal mixxxdj/proposals#17

Merged

	void processEvents() {
	// Calling processEvents() twice ensures that at least all queued and
	// the next round of emitted events are processed.
	// Test fails occurred here for a local debug build on Linux, but not on CI (see https://github.com/mixxxdj/mixxx/pull/4588)
	application()->processEvents();
	application()->processEvents();
	}

Uh oh!

Conversation

christophehenry commented Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

ronso0 commented Feb 10, 2025

Uh oh!

cr7pt0gr4ph7 commented Feb 10, 2025

Uh oh!

Swiftb0y commented Feb 10, 2025

Uh oh!

cr7pt0gr4ph7 commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cr7pt0gr4ph7 commented Feb 11, 2025

Uh oh!

acolombier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christophehenry commented Apr 19, 2025

Uh oh!

JoergAtGithub commented Apr 19, 2025

Uh oh!

christophehenry commented Apr 19, 2025

Uh oh!

acolombier commented Apr 19, 2025

Uh oh!

JoergAtGithub commented Apr 19, 2025

Uh oh!

christophehenry commented Apr 20, 2025

Uh oh!

christophehenry commented May 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christophehenry May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JoergAtGithub commented May 29, 2025

Uh oh!

Swiftb0y commented Jun 17, 2025

Uh oh!

christophehenry commented Jun 17, 2025

Uh oh!

Swiftb0y commented Jun 17, 2025

Uh oh!

christophehenry commented Jun 17, 2025

Uh oh!

christophehenry commented Feb 9, 2025 •

edited

Loading

cr7pt0gr4ph7 commented Feb 11, 2025 •

edited

Loading

christophehenry May 23, 2025 •

edited

Loading

christophehenry commented Jun 20, 2025 •

edited

Loading

christophehenry Jun 20, 2025 •

edited

Loading