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

Allow switching between legacy and new QML UI with command arg #12139

Merged

Conversation

acolombier
Copy link
Contributor

@acolombier acolombier commented Oct 18, 2023

This change reintroduce the --qml argument to allow switch to QML dynamically. It also refactor slightly QML singletons to allow Qt to create a singleton per QML engines

@acolombier acolombier force-pushed the chore/allow-switch-between-legacy-qml branch from 01c6dd1 to 8b29364 Compare October 18, 2023 21:08
@acolombier acolombier marked this pull request as draft October 18, 2023 21:09
@acolombier acolombier force-pushed the chore/allow-switch-between-legacy-qml branch 6 times, most recently from 3fbe327 to 8a284e8 Compare October 18, 2023 21:51
@acolombier acolombier marked this pull request as ready for review October 19, 2023 08:14
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@JoergAtGithub
Copy link
Member

I could compile this on Windows locally. But it asserts on startup (without --qml):

warning [Main] ControlObject "[Controls]" "ShowDurationRemaining" already created
DEBUG ASSERT: "!"pCreatorCO != nullptr, ControlObject already created"" in function class QSharedPointer<class ControlDoublePrivate> __cdecl ControlDoublePrivate::getControl(const class ConfigKey &,class QFlags<enum ControlFlag>,class ControlObject *,bool,bool,bool,double) at C:\Users\Joerg.WORLDWARTWEB\source\repos\JoergAtGithub\mixxx\src\control\control.cpp:175
 	Qt6Core.dll!00007ffbd92e5438()	Unknown
 	Qt6Core.dll!00007ffbd92e9563()	Unknown
>	mixxx.exe!mixxx::`anonymous namespace'::handleMessage(QtMsgType type=QtCriticalMsg, const QMessageLogContext & context={...}, const QString & input={...}) Line 358	C++
 	[External Code]	
 	[Inline Frame] mixxx.exe!mixxx_debug_assert(const char *) Line 9	C++
 	mixxx.exe!ControlDoublePrivate::getControl(const ConfigKey & key={...}, QFlags<enum ControlFlag> flags={...}, ControlObject * pCreatorCO=0x00000157810ec8a0, bool bIgnoreNops=true, bool bTrack=false, bool bPersist=false, double defaultValue=0.0000000000000000) Line 176	C++
 	mixxx.exe!ControlObject::ControlObject(const ConfigKey & key={...}, bool bIgnoreNops, bool bTrack, bool bPersist=false, double defaultValue=0.0000000000000000) Line 24	C++
 	[External Code]	
 	mixxx.exe!DlgPrefDeck::DlgPrefDeck(QWidget * parent, QSharedPointer<ConfigObject<ConfigValue>> pConfig={...}) Line 40	C++
 	mixxx.exe!DlgPreferences::DlgPreferences(std::shared_ptr<mixxx::ScreensaverManager> pScreensaverManager={...}, std::shared_ptr<mixxx::skin::SkinLoader> pSkinLoader={...}, std::shared_ptr<SoundManager> pSoundManager={...}, std::shared_ptr<ControllerManager> pControllerManager={...}, std::shared_ptr<VinylControlManager> pVCManager={...}, std::shared_ptr<EffectsManager> pEffectsManager={...}, std::shared_ptr<SettingsManager> pSettingsManager={...}, std::shared_ptr<Library> pLibrary={...}) Line 180	C++
 	mixxx.exe!MixxxMainWindow::initialize() Line 293	C++
 	[External Code]	
 	mixxx.exe!OpenGLWindow::event(QEvent * pEv=0x0000001cb64fbdf8) Line 67	C++
 	[External Code]	
 	mixxx.exe!`anonymous namespace'::runMixxx(MixxxApplication * pApp=0x0000001cb64ffbd8, const CmdlineArgs & args={...}) Line 76	C++
 	mixxx.exe!main(int argc=1, char * * argv=0x00000157a12d7ed0) Line 206	C++
 	[External Code]	

CMakeLists.txt Outdated Show resolved Hide resolved
@acolombier acolombier force-pushed the chore/allow-switch-between-legacy-qml branch from ac3c551 to f52f0ab Compare October 26, 2023 20:48
@acolombier
Copy link
Contributor Author

Could we please look at merging this ASAP? Since this reorder the file list on the CMakeFile, this will collect conflict very quickly.

@acolombier acolombier force-pushed the chore/allow-switch-between-legacy-qml branch 3 times, most recently from ff43277 to a199d70 Compare October 29, 2023 11:08
@acolombier acolombier force-pushed the chore/allow-switch-between-legacy-qml branch from a199d70 to 93bbda6 Compare October 29, 2023 18:01
@daschuer
Copy link
Member

I can confirm this is working with Ubuntu Focal

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.

some comments:

.github/workflows/build.yml Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member

Just tested this on Arch, I can confirm that it works, with the legacy interface it crashes on shutdown:

fatal [Main] ASSERT failure in WWaveformViewer: "Called object is not of the correct type (class destructor may have already run)", file /usr/include/qt6/QtCore/qobjectdefs_impl.h, line 129
Aborted (core dumped)

Unsure if it is caused by this PR or unrelated.

@JoergAtGithub
Copy link
Member

@Holzhaus Could you please share the stacktrace.

@acolombier
Copy link
Contributor Author

I couldn't reproduce on PopOS 22.04. I tried exiting right after the init sequence (if I quit it during the init sequence, it crashes, but I can reproduce that on main too). I also tried to quite after loading a track/waveform, in vain. Perhaps something to do with your OpenGL config?

@ronso0
Copy link
Member

ronso0 commented Oct 31, 2023

@acolombier Please give the latest commit a more meaningful message, for example
cmakelist: order includes alphabetically

@acolombier
Copy link
Contributor Author

@ronso0 I was planning to squash the commit once everything is working anyway but I will rename it now.

@acolombier acolombier force-pushed the chore/allow-switch-between-legacy-qml branch from 6c9eb61 to b53a61d Compare October 31, 2023 00:17
@acolombier acolombier force-pushed the chore/allow-switch-between-legacy-qml branch 2 times, most recently from 36add5f to c3bedd8 Compare November 1, 2023 09:47
@ywwg
Copy link
Member

ywwg commented Nov 1, 2023

works for me on Ubuntu 22.04

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.

The code looks good to me now.
Can you remove the dedicated Ubuntu qml build? Than it is ready to merge for me.
Thank you

@acolombier acolombier force-pushed the chore/allow-switch-between-legacy-qml branch from c3bedd8 to c45bb4b Compare November 2, 2023 10:38
@acolombier
Copy link
Contributor Author

I've rebased the branch and remove the non-QML build for Ubuntu

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. LGTM

@daschuer daschuer merged commit c781980 into mixxxdj:main Nov 2, 2023
10 of 12 checks passed
@acolombier acolombier deleted the chore/allow-switch-between-legacy-qml branch November 2, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants