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.

Sign up for GitHub

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

Jump to bottom

QML Pt. 1: Add initial support for loading QML skins #3894

Merged
merged 14 commits into from
May 28, 2021
Merged

QML Pt. 1: Add initial support for loading QML skins #3894

merged 14 commits into from
May 28, 2021

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented May 23, 2021 •
edited
Loading

Based on #3891.

@Holzhaus Holzhaus added the skins label May 23, 2021
@Holzhaus Holzhaus force-pushed the skin-refactor-pt2 branch from 88f81ba to a2060a6 Compare May 23, 2021 00:35
@daschuer
Copy link
Member

Thank you. For picking this up.
Is it possible to bring it in a mergeable state without any regressions to the old skins?
I think that would be a reasonable next step.

Maybe for after we have cut out 2.4 (If we go into beta right after 2.3 release)

@Holzhaus
Copy link
Member Author

What regressions did you encounter? There shouldn't be any, or I made a mistake. This is largely based on #3891. Only the last 3 commits are QML related, everything else ist just refactoring.

@daschuer
Copy link
Member

daschuer commented May 23, 2021 •
edited
Loading

I don't think that there is a regression risk until now. What else do you plan to add to this draft?

I don't think we have fixed a plan for 2.4 yet.
It feels urgent to me to decide, to make it clear how to handle new feature PRs like this one.

@Holzhaus Holzhaus force-pushed the skin-refactor-pt2 branch 4 times, most recently from 48748d1 to 9ce44b6 Compare May 23, 2021 23:46
@Holzhaus
Copy link
Member Author

Holzhaus commented May 23, 2021 •
edited
Loading

Ok, I think this is done. Please merge #3891 first, then this diff should become considerably smaller.

The QML demo skin now features a working (!) play button, 3-band EQ, filter knob, gain knob and volume slider for deck 1 and 2. Please have a look. I can remove the demo skin before merge :D

You can test by passing track path(s) on the command line when running Mixxx, because there is no library yet. Or just load tracks using a legacy skin, then switch to the QML demo skin.

@Holzhaus Holzhaus mentioned this pull request May 24, 2021
Closed
5 tasks
@ronso0
Copy link
Member

ronso0 commented May 24, 2021

The QML demo skin now features a working (!) play button, 3-band EQ, filter knob, gain knob and volume slider for deck 1 and 2. Please have a look. I can remove the demo skin before merge :D

Can we keep the QML demo but hide it by default, and list it when --developer or --qml argument is used?

@Holzhaus Holzhaus force-pushed the skin-refactor-pt2 branch 4 times, most recently from c69c72c to a21861f Compare May 25, 2021 09:34
@daschuer
Copy link
Member

Uh, this PR is rapidly growing.
Did you consider to split our the skin technology abstraction, introduced during the first commits.
For my understanding this part was already in a mergable state.

@daschuer
Copy link
Member

I am also happy to merge only the folder move.
This way we hopefully get rid of many of these 110 touched files.

@Holzhaus
Copy link
Member Author

Uh, this PR is rapidly growing.
Did you consider to split our the skin technology abstraction, introduced during the first commits.
For my understanding this part was already in a mergable state.

I already did, see #3891.

@Holzhaus Holzhaus force-pushed the skin-refactor-pt2 branch from a21861f to 4fd651e Compare May 25, 2021 13:12
@Holzhaus Holzhaus mentioned this pull request May 25, 2021
Merged
@Holzhaus Holzhaus changed the title [WIP] Add initial support for QML skins QML: Add initial support for loading QML skins May 25, 2021
@Holzhaus Holzhaus force-pushed the skin-refactor-pt2 branch from 4fd651e to 6da063f Compare May 25, 2021 15:13
@Holzhaus Holzhaus changed the title QML: Add initial support for loading QML skins QML Pt. 1: Add initial support for loading QML skins May 25, 2021
@Holzhaus Holzhaus marked this pull request as ready for review May 25, 2021 16:40
@Holzhaus Holzhaus force-pushed the skin-refactor-pt2 branch 2 times, most recently from 6c42e95 to 430e957 Compare May 26, 2021 06:29
@Holzhaus
Copy link
Member Author

I reworked the initializiation logic.Initialization is now delayed until the component is fully constructed and all initial properties have been set: https://doc.qt.io/qt-5/qtqml-cppintegration-definetypes.html#receiving-notifications-for-object-initialization

@Holzhaus Holzhaus force-pushed the skin-refactor-pt2 branch 4 times, most recently from 6145db5 to ba6861c Compare May 27, 2021 18:29
Holzhaus and others added 12 commits May 28, 2021 01:31
@Holzhaus
Skin: Add QmlSkin class
153a8d2
@Holzhaus
SkinLoader: Disable QML support by default via config option
26395a5
@Be-ing @Holzhaus
ControlProxyQml: Add support for CO access from QML
dc760ab
@Holzhaus
ControlProxyQml: Ensure that UI is updated in any case
2b98680
@Holzhaus
QML: Add more checks and warnings and expose keyValid/initialized props
fc455bd
@Holzhaus
QML: Rename ControlProxyQml to QmlControlProxy and add to namespace
41b1e3e
@Holzhaus
QML: Register QmlControlProxy as Mixxx.ControlProxy
80181b0
@Holzhaus
QmlSkin: Add private dir() helper for constructing filePaths more eff…
9cd7649 
…iciently
@Holzhaus
QML: Remove unused m_emptyString member from QmlControlProxy
87e4b82
@Holzhaus
QML: Rework QmlControlProxy initialization/reset logic
c738dc5
@Holzhaus
QML: Prevent QmlControlProxy value/parameter change while not initial…
7fc5ed0 
…ized
@Holzhaus
QML: Prevent QmlControlProxy log spam when using an invalid CO
bf2518e 
A single warning should suffice.
@Holzhaus Holzhaus force-pushed the skin-refactor-pt2 branch from ba6861c to bf2518e Compare May 27, 2021 23:34
@Holzhaus
Copy link
Member Author

OK, finally all checks pass. The ControlProxy code works much nicer now. Please have a look. For testing and see it in action, I suggest to check out the QML Demo skin from #3907.

uklotzde
uklotzde approved these changes May 28, 2021
View reviewed changes
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.

LGTM. Let's merge after the CI builds succeeded.

daschuer
daschuer requested changes May 28, 2021
View reviewed changes
src/skin/qml/qmlcontrolproxy.cpp Outdated
return m_pControlProxy->getParameter();
}

void QmlControlProxy::reinitializeOrReset() {
Copy link
Member

Choose a reason for hiding this comment

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

Is orReset still valid?
It look like the function returns early if it is already initialized

Copy link
Member

Choose a reason for hiding this comment

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

OrReset is confusing, because resetting a control proxy is different from resetting a smart pointer. It is also not the alternative to initialize.

How about:
ReinitializeFromKey()

Than it is clear that one cane assume that it becomes invalid if the key is invalid

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

daschuer
daschuer reviewed May 28, 2021
View reviewed changes
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

I have left just an optional comment where I stumbled over the function name.
LGTM

daschuer
daschuer approved these changes May 28, 2021
View reviewed changes
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you for all the QML PRs. I am really curious how this will feel ..

@Holzhaus Holzhaus requested a review from daschuer May 28, 2021 16:38
@daschuer daschuer merged commit 5b16118 into mixxxdj:main May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uklotzde uklotzde uklotzde approved these changes

@daschuer daschuer Awaiting requested review from daschuer

Assignees
No one assigned
Labels
build code quality skins ui
Projects
QML GUI
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Holzhaus @daschuer @ronso0 @uklotzde @Be-ing