-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
CMake: add option to build with Qt6 #4051
Merged
+163
−138
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to remove all these files? Can't we keep everything except QGLWidget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also suggest to keep everything that builds fine with Qt5 and only remove files conditionally if incompatibilities arise or if they become obsolete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there is no point trying to get the legacy skin system to build with Qt6 because essential features cannot work with Qt6. That would probably require lots more preprocessor Qt version checks scattered around the legacy code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which ones won't work? I though only QOpenGLWidget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The legacy skin system uses QGLWidget, not QOpenGLWidget, which is the reason it needs to be replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I advocated for this approach with the library but @Holzhaus was against it. Since @Holzhaus is doing the work on that and I am minimally familiar with the library code, I trust that his approach is a good way to proceed. I ask that trust be reciprocated here. From my experiences redesigning Deere, implementing the effects system, following #1974, and struggling for months with every idea I could think of to improve macOS performance with the legacy skin system, I judge that the legacy skin system is a dead end that we should abandon as soon as possible. macOS performance is still only kinda passable now and we have no idea how to fix it without starting from scratch with Qt Quick. I want to implement a new feature on top of #2618 for one big effect chain with more effects, but just the thought of adding more complexity with the legacy skin system -- and repeating that four times! -- is making me reconsider whether that is worthwhile at this time. I will not put any work into backtracking by putting QQuickWidgets into legacy skins. We know from the Qt documentation such an approach would have major performance limitations and it might not even solve our persistent macOS troubles.
Mixxx's CPU usage at baseline, which is due to the hugely inefficient GUI, is terrible. I have never gotten Mixxx to run without my computer's fans turning on. This is should not happen. Mixxx is not a fancy schmancy 3D game. Our fanciest graphics are just drawing a waveform. This should not be difficult at all for my fancy schmancy laptop, yet it is. Mixxx should be able to run easily on hardware as weak as a PinePhone, but we won't get there if we cling to very inefficient legacy GUI technology. The skin system dates from before Qt had any comparable solution. It essentially does what Qt did with the XML GUI layouts for Qt Designer, but badly. Qt's current solution with Qt Quick is much, much better both for performance and maintainability. Let's embrace it without hesitation and drop the legacy code ASAP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot drop legacy code until the new code works fine. That's my whole point. Otherwise we'll have a broken mixxx main branch and cannot do a mixxx release in years.
I know this is just for Qt6 support and I'm okay with merging initial Qt6 support without the legacy skin system, but we should not burn bridges unless we need to.
I have no objections against a new hidden feature that is not accessible from the legacy skins, just from QML. But if that feature breaks legacy skins, we cannot merge at this time. QML skin feature parity is probably still years away, and I'd like to do more releases until then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to ensure that we are all in a position to make progress. No one is forced to work on features that they do not support. Nevertheless we need to coordinate our work and be aware of the differing goals to prevent conflicts and frustration.
I guess we all agree that we want and need to replace the legacy skins with QML asap. If it turns out that keeping the legacy skins alive imposes too much work or would pollute the code base unacceptably we can decide to stop this maintenance effort at any time and shift focus. If we could agree to proceed with the intent to defer this decision until it becomes inevitable instead of giving up recklessly this would help.
How about extracting Qt6-only components/modules into separate files if needed, e.g. the new QML UI based on #2618? This will hopefully prevent to mix conflicting Qt5/Qt6 code in the same file when managing the differences becomes inconvenient. Not sure how to deal with the split at the QML-level where we also have backwards compatible components that are not even using all the features available in Qt 5.15, mainly due to outdated Ubuntu dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully agree. But wasting effort on maintaining legacy code with Qt6 does nothing to help us get there, it only puts obstacles in the way.
The point of this approach is that it does not touch the legacy code at all. It just doesn't build the old code with Qt6. No bridges were harmed in the making of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we get CI setup for Qt6 we could make QML build for only Qt6 and not worry about old Qt5 versions. Here is a PR to start that:
mixxxdj/vcpkg#19
After we get vcpkg working on macOS, it should be easy to use vcpkg for Linux CI as well.