feat: refactor player proxy and create separated track proxy by acolombier · Pull Request #14516 · mixxxdj/mixxx

acolombier · 2025-03-24T02:14:14Z

This allows QML to interact with a track without depending of the player

JoergAtGithub

How does this scale with thousands of tracks in a table? How many track proxies would exist in this case?

acolombier · 2025-04-19T20:22:11Z

Thank you for your review!

How does this scale with thousands of tracks in a table? How many track proxies would exist in this case?

It's working great. The reason for this is that TableView will only load needed rows (see doc), so Qt will take care of loading what display on the screen (+ some extra items to smooth scrolling

ywwg

can you generate a code coverage report for your changes? As part of the new QML world we should aim for very coverage, which will prevent a lot of the UI bugs we get with the current barely-tested XML system.

If we do not have a good testing framework in place, we don't have to make one for this change, but we should start thinking about how to create one.

ywwg · 2025-05-05T13:23:07Z

+            &QmlPlayerProxy::loadTrackRequested,
+            this,
+            [this, group](TrackPointer track,
+#ifdef __STEM__


why are we ifdeffing stems? Can we assume that by the time QML exists, every build will have stems? if so, then I'm ok making stems a prerequisite for building qml. (but not if this would force everyone to have stems turned on)

QML is already expected to be enabled for controller screens rendering, since 2.6. We could expect that both QML and STEM might be stable enough when we reach 3.0 to remove these two flags tho, but for now they are needed.

@ywwg Do you agree with keeping these ifdefs for now?

acolombier · 2025-05-05T15:25:48Z

can you generate a code coverage report for your changes?

Code coverage is included already (+0.009% on this PR). The reason there is no new code here, I'm only moving existing accessor out of the PlayerProxy into a TrackProxy. I believe the increased is related to the fact that this PR managed to reduce the code for these accessor, due to the waveform clean-up, no longer required.

If we do not have a good testing framework in place, we don't have to make one for this change, but we should start thinking about how to create one.

I have a PR that that adds UI/E2E testing for QML . It is pretty much ready but it didn't pick much traction yet.

ywwg · 2025-05-05T18:27:02Z

It is pretty much ready but it didn't pick much traction yet.

the PR is marked as draft, I generally don't look at PRs that are not ready for review

ywwg · 2025-05-05T18:28:02Z

(but that doesn't really make a difference, I am not qualified to review QML code anyway)

Swiftb0y

couple thoughts

Swiftb0y

couple more comments, this is looking good

Swiftb0y · 2025-05-07T15:27:09Z


 class QmlPlayerProxy : public QObject {
    Q_OBJECT
+    Q_PROPERTY(QmlTrackProxy* currentTrack READ currentTrack NOTIFY trackChanged)


should we also add a setter that dispatches to loadTrack directly for symmetry?

I'm not sure. The rationale for not adding it is that loading a track is a non trivial operation (due to complexity and disruptive side effects it may have), and properties in QML may easily suffer a bug due to property binding. That's why I kept the loadTrack as a method, which you must explicitly call, instead having the risk of that implicit magic side effects

right, but doesn't the problem still apply when the function is called from for example a signal handler? IMO the QmlProxy needs to make sure to debounce these calls appropriately.

Yes exactly, the difference is that this requires an explicit call (e.g onSignal => loadTrack(...)) where if you do assignation, it could then auto load a track when the binded property changes. Perhaps we could interate on that? If we realise it makes sense to use the setter, it should be pretty trivial to add.

Right, but you can also easily bind to onPropertyChanged (for some Property) which would also get fired multiple times. So regardless of whether we have the setter or not, the loadTrack member function needs debouncing. wdyt?

Right, I think I understood now, so you mean we should for example discard subsequent request to load file if QmlPlayerProxy has already emitted the load signal to the player manager and is waiting for it to proceeded?
If I got it right this time, and since this is an existing limitation, are you happy with me creating a follow issue for this?

Right, I think I understood now, so you mean we should for example discard subsequent request to load file if QmlPlayerProxy has already emitted the load signal to the player manager and is waiting for it to proceeded?

Yes, exactly.

If I got it right this time, and since this is an existing limitation

Is it? I thought the CO does have some safety checks to avoid this.

Yes, I meant it is already a problem in QMLPlayerProxy, and I'm only moving the code around here

(see all the 3 load operations in main)

Ah, I see. Yeah sure. Lets file an issue and move on.

Swiftb0y · 2025-05-13T10:06:15Z

Please squash and resolve merge conflict, then we can merge.

Swiftb0y

LGTM. @ywwg seems to be unresponsive. I'm taking the liberty to merge regardless.

unresposive

github-actions Bot added build qml labels Mar 24, 2025

acolombier mentioned this pull request Mar 24, 2025

feat: add Library on QML #14514

Merged

acolombier added this to the 3.0.0-beta milestone Mar 25, 2025

acolombier moved this to In progress in Releases Mar 25, 2025

acolombier added this to Releases Mar 25, 2025

acolombier self-assigned this Mar 25, 2025

acolombier force-pushed the fix/split-player-track-qml-proxy branch from c89e096 to 227b78f Compare March 29, 2025 01:06

github-actions Bot added the code quality label Mar 29, 2025

acolombier force-pushed the fix/split-player-track-qml-proxy branch 5 times, most recently from dd27b8e to c8b0b01 Compare March 29, 2025 18:56

JoergAtGithub added the needs review label Apr 6, 2025

acolombier mentioned this pull request Apr 19, 2025

Create JavascriptPlayerProxy #14314

Merged

JoergAtGithub reviewed Apr 19, 2025

View reviewed changes

Comment thread src/qml/qmlplayerproxy.cpp Outdated

Comment thread src/qml/qmltrackproxy.h Outdated

ywwg previously requested changes May 5, 2025

View reviewed changes

Swiftb0y reviewed May 6, 2025

View reviewed changes

Comment thread src/qml/qmltrackproxy.h Outdated

Comment thread src/qml/qmltrackproxy.h Outdated

Comment thread src/qml/qmlplayerproxy.cpp Outdated

Comment thread src/qml/qmltrackproxy.cpp Outdated

Swiftb0y reviewed May 7, 2025

View reviewed changes

acolombier force-pushed the fix/split-player-track-qml-proxy branch from 093a32e to 6504636 Compare May 18, 2025 23:42

feat: refactor player proxy and create separated track proxy

2aeee0a

acolombier force-pushed the fix/split-player-track-qml-proxy branch from 6504636 to 2aeee0a Compare May 18, 2025 23:43

acolombier mentioned this pull request May 18, 2025

Prevent repeated load operation on PlayerProxy #14809

Closed

acolombier requested a review from ywwg May 18, 2025 23:44

acolombier requested a review from Swiftb0y May 18, 2025 23:45

Swiftb0y approved these changes May 19, 2025

View reviewed changes

Swiftb0y merged commit 7e9489f into mixxxdj:main May 19, 2025
3 checks passed

github-project-automation Bot moved this from In progress to Done in Releases May 19, 2025

Uh oh!

Conversation

acolombier commented Mar 24, 2025

Uh oh!

JoergAtGithub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

acolombier commented Apr 19, 2025

Uh oh!

ywwg left a comment

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!

acolombier commented May 5, 2025

Uh oh!

ywwg commented May 5, 2025

Uh oh!

ywwg commented May 5, 2025

Uh oh!

Swiftb0y left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Swiftb0y left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

Swiftb0y commented May 13, 2025

Uh oh!

Swiftb0y left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants