decouple QML from legacy skin system + add preferences button #4327

Be-ing · 2021-09-27T06:47:14Z

Somehow this is hanging in the ControllerManager destructor. It seems the requestShutdown signal does not trigger slotShutdown so m_pThread->wait() blocks indefinitely.

src/qmlapplication.h

Be-ing · 2021-09-27T11:48:00Z

Any ideas why ~ControllerManager is hanging?

uklotzde · 2021-09-27T13:48:09Z

Any ideas why ~ControllerManager is hanging?

I didn't have time to test it yet. Will do later.

Be-ing · 2021-09-27T20:23:58Z

Effects including EQs are not working with QML. I tested merging #2618 but that does not fix it. There must be some implicit dependency of the effects system on the setup of the legacy GUI. Moving controller knobs for EQs changes the QML, so it seems the QML is okay and the problem is in C++.

Be-ing · 2021-09-27T20:42:44Z

Well this is really ugly. Initialization of the effects system depends on the existence of a DlgPreferences instance.

Be-ing · 2021-09-27T20:45:02Z

Even uglier: the hang in ~ControllerManager goes away when a DlgPreferences instance is created.

uklotzde

Dropping the "skin" prefix from namespaces and file paths is a good idea. We have skins and we have qml, mutually exclusive. This avoids to distinguish between legacy skins and QML skins.

src/qml/qmlapplication.h

Be-ing · 2021-09-27T21:32:09Z

More ugliness: I cannot include include preferences/dialog/dlgpreferences.h in src/qml/qmlapplication.h because compiling main.cpp fails with:

In file included from ../src/qml/qmlapplication.h:4,
                 from ../src/main.cpp:14:
../src/preferences/dialog/dlgpreferences.h:13:10: fatal error: preferences/dialog/ui_dlgpreferencesdlg.h: No such file or directory
   13 | #include "preferences/dialog/ui_dlgpreferencesdlg.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

I tried adding

add_executable(mixxx WIN32 src/main.cpp src/qml/qmlapplication.cpp)
set_target_properties(mixxx PROPERTIES AUTOUIC ON AUTOMOC ON)

to CMakeLists.txt and removing src/qml/qmlapplication.cpp from the sources list for the mixxx-lib target. Unfortunately the build still fails. CMake is generating ui_dlgpreferencesdlg.h but it is located in build/mixxx-lib_autogen/include/preferences/dialog/ui_dlgpreferencesdlg.h which is not in the list of include paths for the mixxx executable target. Any suggestions how to resolve this?

Be-ing · 2021-09-28T04:17:49Z

I made a QML button to show the preferences but I am puzzled how closing the preferences window triggers the QmlApplication destructor. I set a breakpoint in ~DlgPreferences which shows that DlgPreferences is properly getting deleted by QmlApplication... but why is QmlApplication getting destroyed in the first place when DlgPreferences is closed?? It does not matter whether DlgPreferences is closed by the Okay, Apply, or Cancel button or the window manager close button. I commented out adding all the pages in the DlgPreferences constructor but I still reproduced the issue. I also tried creating a QQuickView instead of a QQmlApplicationEngine but the issue still occurred.

0  DlgPreferences::~DlgPreferences() (this=0x9c1d060, __in_chrg=<optimized out>) at ../src/preferences/dialog/dlgpreferences.cpp:297
#1  0x000000000059394a in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (this=0x9c1d050)
    at /usr/include/c++/11/bits/shared_ptr_base.h:168
#2  std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (this=0x9c1d050) at /usr/include/c++/11/bits/shared_ptr_base.h:161
#3  std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() (this=0x7fffffffdc48, __in_chrg=<optimized out>)
    at /usr/include/c++/11/bits/shared_ptr_base.h:705
#4  std::__shared_ptr<DlgPreferences, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (this=0x7fffffffdc40, __in_chrg=<optimized out>)
    at /usr/include/c++/11/bits/shared_ptr_base.h:1154
#5  std::shared_ptr<DlgPreferences>::~shared_ptr() (this=0x7fffffffdc40, __in_chrg=<optimized out>) at /usr/include/c++/11/bits/shared_ptr.h:122
#6  mixxx::qml::QmlApplication::~QmlApplication() (this=0x7fffffffdc00, __in_chrg=<optimized out>) at ../src/qml/qmlapplication.h:19
#7  0x00000000005740c9 in (anonymous namespace)::runMixxx (args=<optimized out>, pApp=0x7fffffffdbc0) at ../src/main.cpp:41
#8  main(int, char**) (argc=<optimized out>, argv=0x7fffffffddd8) at ../src/main.cpp:189

src/qml/qmlapplication.cpp

Be-ing · 2021-09-28T05:16:45Z

Well this is interesting. I replaced the DlgPreferences instance with a simple QDialogButtonBox and reproduced the issue of QmlApplication shutting down when the widget is destroyed.

Be-ing · 2021-09-28T06:02:54Z

I figured out the problem. QApplication's event loop shuts down the QApplication when the last QWidget window is closed. I confirmed this by showing a dummy QDialogButtonBox then opening the preferences from QML. In that case, I could close the preferences without shutting down the application but the application shut down after closing both the preferences and the dummy QDialogButtonBox.

I attempted changing MixxxApplication to subclass QGuiApplication instead of QApplication, but then it is not possible to use any QWidgets at all; at runtime console output showed:

QWidget: Cannot create a QWidget without QApplication

So I think we need to filter out the erroneous QEvent::Close event in MixxxApplication when the QQmlApplicationEngine is still showing to avoid needing to rewrite the whole preferences in QML immediately.

Holzhaus · 2021-09-28T09:40:19Z

Just setting this to false doesn't work?
https://doc.qt.io/qt-5/qguiapplication.html#quitOnLastWindowClosed-prop

Be-ing · 2021-09-28T09:41:17Z

Ha! That looks like a simpler solution, thanks.

Be-ing · 2021-09-28T16:17:44Z

Adding

    app->setQuitOnLastWindowClosed(false);
    app->setQuitLockEnabled(false);

to the QmlApplication constructor does not solve the problem.

Be-ing · 2021-09-28T16:25:20Z

I found a one line fix:

m_pDlgPreferences->setAttribute(Qt::WA_QuitOnClose, false);

Now to deal with the hang in ~ControllerManager...

src/errordialoghandler.cpp

src/preferences/dialog/dlgprefinterface.cpp

res/qml/main.qml

Swiftb0y

LGTM, but I'd like to have a second opinion before merging if possible.

src/preferences/dialog/dlgpreferences.cpp

to prevent closing QWidget error dialogs from shutting down Mixxx when using the QQmlApplication GUI

That was required for hacking QML into the legacy skin system but it created a regression with the cursor getting stuck on the splitter dragging handle: https://bugs.launchpad.net/mixxx/+bug/1943860

Be-ing · 2021-09-29T23:50:23Z

Merge?

Holzhaus · 2021-12-13T14:13:32Z

This PR leads to 2 major regressions:

Closing any popup closes Mixxx. E.g. if a track fails to load, a popup will open. If you close it, Mixxx shuts down.
It's not possible to use the developer tools anymore, which makes QML skin development extremely tedious.

Be-ing · 2021-12-13T16:50:03Z

Closing any popup closes Mixxx. E.g. if a track fails to load, a popup will open. If you close it, Mixxx shuts down.

Qt::WA_QuitOnClose needs to be set to false for every QWidget window: https://github.com/mixxxdj/mixxx/pull/4327/files#diff-2768d0d13eda8f9b18b5ce2c75faed57cb48c7f8463612e674455561533234f7R63

Be-ing marked this pull request as draft September 27, 2021 06:47

github-actions bot added build skins labels Sep 27, 2021

uklotzde suggested changes Sep 27, 2021

View reviewed changes

src/qmlapplication.h Outdated Show resolved Hide resolved

src/qmlapplication.h Outdated Show resolved Hide resolved

Be-ing force-pushed the qmlapplicationengine branch 3 times, most recently from 7372c3c to 7be95d5 Compare September 27, 2021 19:43

uklotzde reviewed Sep 27, 2021

View reviewed changes

src/qml/qmlapplication.h Outdated Show resolved Hide resolved

src/qml/qmlapplication.h Outdated Show resolved Hide resolved

src/qml/qmlapplication.h Outdated Show resolved Hide resolved

github-actions bot added the ui label Sep 27, 2021

Be-ing force-pushed the qmlapplicationengine branch 2 times, most recently from 0a8c937 to 2dfe7fc Compare September 28, 2021 02:39

Be-ing requested a review from Holzhaus September 28, 2021 04:12

Be-ing force-pushed the qmlapplicationengine branch from 2dfe7fc to 4900acf Compare September 28, 2021 04:19

Be-ing commented Sep 28, 2021

View reviewed changes

src/qml/qmlapplication.cpp Outdated Show resolved Hide resolved

Holzhaus closed this Sep 28, 2021

Holzhaus reopened this Sep 28, 2021

Be-ing force-pushed the qmlapplicationengine branch from 4900acf to 030237e Compare September 28, 2021 16:24

Be-ing force-pushed the qmlapplicationengine branch from 0f791df to 78c5913 Compare September 28, 2021 22:04

Swiftb0y reviewed Sep 29, 2021

View reviewed changes

src/errordialoghandler.cpp Show resolved Hide resolved

src/preferences/dialog/dlgprefinterface.cpp Show resolved Hide resolved

res/qml/main.qml Outdated Show resolved Hide resolved

Be-ing added 7 commits September 29, 2021 10:11

add QmlApplication class activated by --qml CLI option

b316237

remove QmlSkin class

e16b5e8

move src/skin/qml to src/qml

679d07b

move mixxx::skin::qml namespace to mixxx::qml

66be390

move src/qmlapplication.{cpp,h} to src/qml

9b7f745

move res/skins/QMLDemo to res/qml

d4bf033

QmlApplication: capture weak pointers in lambdas

9215fc6

Be-ing force-pushed the qmlapplicationengine branch 2 times, most recently from 413b745 to dced202 Compare September 29, 2021 16:10

Be-ing requested a review from uklotzde September 29, 2021 16:10

Swiftb0y reviewed Sep 29, 2021

View reviewed changes

uklotzde reviewed Sep 29, 2021

View reviewed changes

src/preferences/dialog/dlgpreferences.cpp Outdated Show resolved Hide resolved

uklotzde approved these changes Sep 29, 2021

View reviewed changes

Be-ing added 3 commits September 29, 2021 18:50

add QML button to show DlgPreferences

9218ea9

ErrorDialogHandler: set WA_QuitOnClose to false

f5f517e

to prevent closing QWidget error dialogs from shutting down Mixxx when using the QQmlApplication GUI

MixxxApplication: remove Qt::AA_DontCreateNativeWidgetSiblings

74f8693

That was required for hacking QML into the legacy skin system but it created a regression with the cursor getting stuck on the splitter dragging handle: https://bugs.launchpad.net/mixxx/+bug/1943860

Be-ing force-pushed the qmlapplicationengine branch from dced202 to 74f8693 Compare September 29, 2021 23:50

Be-ing changed the title ~~decouple QML from legacy skin system~~ decouple QML from legacy skin system + add preferences button Sep 30, 2021

Be-ing mentioned this pull request Sep 30, 2021

QML optimization #4339

Merged

Swiftb0y merged commit 431d107 into mixxxdj:main Sep 30, 2021

Be-ing deleted the qmlapplicationengine branch September 30, 2021 12:55

Holzhaus mentioned this pull request Dec 13, 2021

QML: Library Improvements #4567

Merged

mixxxbot mentioned this pull request Aug 23, 2022

Once triggered, the cursor stays in "resize" mode #10532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

decouple QML from legacy skin system + add preferences button #4327

decouple QML from legacy skin system + add preferences button #4327

Be-ing commented Sep 27, 2021

Be-ing commented Sep 27, 2021

uklotzde commented Sep 27, 2021

Be-ing commented Sep 27, 2021 •

edited

Loading

Be-ing commented Sep 27, 2021

Be-ing commented Sep 27, 2021

uklotzde left a comment

Be-ing commented Sep 27, 2021

Be-ing commented Sep 28, 2021 •

edited

Loading

Be-ing commented Sep 28, 2021

Be-ing commented Sep 28, 2021 •

edited

Loading

Holzhaus commented Sep 28, 2021 •

edited

Loading

Be-ing commented Sep 28, 2021

Be-ing commented Sep 28, 2021

Be-ing commented Sep 28, 2021 •

edited

Loading

Swiftb0y left a comment

Be-ing commented Sep 29, 2021

Holzhaus commented Dec 13, 2021

Be-ing commented Dec 13, 2021

decouple QML from legacy skin system + add preferences button #4327

decouple QML from legacy skin system + add preferences button #4327

Conversation

Be-ing commented Sep 27, 2021

Be-ing commented Sep 27, 2021

uklotzde commented Sep 27, 2021

Be-ing commented Sep 27, 2021 • edited Loading

Be-ing commented Sep 27, 2021

Be-ing commented Sep 27, 2021

uklotzde left a comment

Choose a reason for hiding this comment

Be-ing commented Sep 27, 2021

Be-ing commented Sep 28, 2021 • edited Loading

Be-ing commented Sep 28, 2021

Be-ing commented Sep 28, 2021 • edited Loading

Holzhaus commented Sep 28, 2021 • edited Loading

Be-ing commented Sep 28, 2021

Be-ing commented Sep 28, 2021

Be-ing commented Sep 28, 2021 • edited Loading

Swiftb0y left a comment

Choose a reason for hiding this comment

Be-ing commented Sep 29, 2021

Holzhaus commented Dec 13, 2021

Be-ing commented Dec 13, 2021

Be-ing commented Sep 27, 2021 •

edited

Loading

Be-ing commented Sep 28, 2021 •

edited

Loading

Be-ing commented Sep 28, 2021 •

edited

Loading

Holzhaus commented Sep 28, 2021 •

edited

Loading

Be-ing commented Sep 28, 2021 •

edited

Loading