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

QML Pt. 3: Add support for retrieving deck track properties #3911

Merged
merged 11 commits into from
Jun 4, 2021

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented May 25, 2021

This allows displaying track properties from loaded decks in the QML GUI. Roughly equivalent to WTrackText.

Based on #3907.

Minimal working example:

import Mixxx.CoreServices 1.0 as MixxxCoreServices

Item {
    id: root
    property var deckPlayer: MixxxCoreServices.PlayerManager.getPlayer("[Channel1]")
    
    // Display the track title
    Text {
        text: root.deckPlayer.title
    }
    
    // Display the track color
    Rectangle {
        color: root.deckPlayer.color
    }

    // Change the track title
    Button {
        text: "Change Track Title"
        onClicked: root.deckPlayer.title = "Hello World!"
    }
}

Current state of the Demo skin (track on deck 1 has green track color):

Peek 2021-05-25 18-45

@Holzhaus Holzhaus changed the title QML: Add support for retrieving deck track properties QML Pt. 3: Add support for retrieving deck track properties May 25, 2021
@uklotzde
Copy link
Contributor

Wait, wait, let's first merge the prerequisite PRs 😅

@Holzhaus Holzhaus force-pushed the qml-track-properties branch 8 times, most recently from 206b078 to c4b33fa Compare May 27, 2021 13:57
@Holzhaus Holzhaus marked this pull request as draft May 27, 2021 14:16
@Holzhaus Holzhaus force-pushed the qml-track-properties branch 4 times, most recently from 7c1b3aa to f9111b0 Compare May 27, 2021 23:49
@Holzhaus
Copy link
Member Author

Hmm, that sucks. Apparently qmlRegisterSingletonInstance does not exist in older Qt versions. I'll check if we can fall back to some alternative.

@uklotzde
Copy link
Contributor

Hmm, that sucks. Apparently qmlRegisterSingletonInstance does not exist in older Qt versions. I'll check if we can fall back to some alternative.

Instead of wasting time with workarounds for outdated Qt versions I suggest to restrict QML support to Qt 5.15 and newer. Can we organize the code in a way that allows to exclude the QML components at build time if not supported?

src/skin/qml/qmlskin.cpp Outdated Show resolved Hide resolved
@Holzhaus Holzhaus force-pushed the qml-track-properties branch from 00b21fa to 61dfc98 Compare May 28, 2021 09:00
@Holzhaus
Copy link
Member Author

Holzhaus commented May 28, 2021

The last commit should fix the issue with no #ifdef'ed code, let's hope CI agrees.

@Holzhaus Holzhaus force-pushed the qml-track-properties branch 2 times, most recently from b6c7bb9 to 07af114 Compare May 28, 2021 09:18
@Holzhaus
Copy link
Member Author

Can someone help me with the lambda? I have no idea why it doesn't work. It works on my machine and it's more or less copied from the Qt 5.12 documentation:

/home/runner/work/mixxx/mixxx/src/skin/qml/qmlskin.cpp: In member function ‘virtual QWidget* mixxx::skin::qml::QmlSkin::loadSkin(QWidget*, UserSettingsPointer, QSet<ControlObject*>*, mixxx::CoreServices*) const’: 

904Error: /home/runner/work/mixxx/mixxx/src/skin/qml/qmlskin.cpp:126:14: error: cannot convert ‘mixxx::skin::qml::QmlSkin::loadSkin(QWidget*, UserSettingsPointer, QSet<ControlObject*>*, mixxx::CoreServices*) const::<lambda(QQmlEngine*, QJSEngine*)>’ to ‘QObject* (*)(QQmlEngine*, QJSEngine*)’ 

905 126 | }); 

906 | ^ 

907 | | 

908 | mixxx::skin::qml::QmlSkin::loadSkin(QWidget*, UserSettingsPointer, QSet<ControlObject*>*, mixxx::CoreServices*) const::<lambda(QQmlEngine*, QJSEngine*)> 

909In file included from /usr/include/x86_64-linux-gnu/qt5/QtQml/qqmlengine.h:47, 

910 from /usr/include/x86_64-linux-gnu/qt5/QtQml/QQmlEngine:1, 

911 from /home/runner/work/mixxx/mixxx/src/skin/qml/qmlskin.cpp:3: 

912/usr/include/x86_64-linux-gnu/qt5/QtQml/qqml.h:635:44: note: initializing argument 5 of ‘int qmlRegisterSingletonType(const char*, int, int, const char*, QObject* (*)(QQmlEngine*, QJSEngine*)) [with T = mixxx::skin::qml::QmlPlayerManagerProxy]’ 

913 635 | QObject *(*callback)(QQmlEngine *, QJSEngine *)) 

914 | 

@Holzhaus Holzhaus force-pushed the qml-track-properties branch 2 times, most recently from 84a384b to 7d999da Compare May 28, 2021 20:19
@Holzhaus Holzhaus force-pushed the qml-track-properties branch 2 times, most recently from d208917 to 6c6f474 Compare May 28, 2021 20:47
@Holzhaus Holzhaus force-pushed the qml-track-properties branch from 6c6f474 to a37420e Compare May 29, 2021 12:40
@Holzhaus Holzhaus marked this pull request as ready for review May 29, 2021 12:40
@uklotzde
Copy link
Contributor

If rebasing on #3924 avoids introducing temporary glue code then let's do it.

@Holzhaus Holzhaus force-pushed the qml-track-properties branch from a37420e to 3196025 Compare June 1, 2021 19:44
@Holzhaus Holzhaus requested a review from uklotzde June 4, 2021 19:56
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Routing each individual signal through the proxy is error prone and smells like an anti-pattern to me. But I am not aware of a better solution yet.

These fine-grained, two-way data bindings get out of control and become unmaintainable soon, because you lose the notion of discrete states and their transitions. But that's a different topic that we cannot solve here ;)

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Since this might not be the final version let's use it for the moment to get more experience how to handle dependent properties in QML properly.

Thank you all this prototyping work! I am well aware that this is often the most challenging phase of a project 😛 LGTM

@uklotzde uklotzde merged commit c185497 into mixxxdj:main Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants