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

Make Mixxx build with Qt6 #4621

Closed
wants to merge 16 commits into from
Closed

Make Mixxx build with Qt6 #4621

wants to merge 16 commits into from

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Jan 15, 2022

Makes it possible to build Mixxx with Qt6 without any compiler warnings or linker errors. Based on:

Next step is to fix the QML UI, because there are quite some incompatibilities apparently.

@Holzhaus Holzhaus force-pushed the qt6-fixes branch 2 times, most recently from e28f9d0 to 95284da Compare January 15, 2022 01:19
@Holzhaus Holzhaus changed the title Qt6 fixes Make Mixxx build with Qt6 Jan 15, 2022
@Be-ing
Copy link
Contributor

Be-ing commented Jan 15, 2022

Interesting... how do you plan to remove no longer used code?

@@ -551,7 +553,9 @@ void EngineBuffer::slotTrackLoaded(TrackPointer pTrack,
TrackPointer pOldTrack = m_pCurrentTrack;
m_pause.lock();

#if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)
Copy link
Contributor

@uklotzde uklotzde Jan 15, 2022

Choose a reason for hiding this comment

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

Let's switch these code blocks, i.e. the Qt 6 variant should take precedence and should be placed before the legacy workaround. Consistency, makes it easier to throw away all legacy code later.

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.

@Holzhaus
Copy link
Member Author

Interesting... how do you plan to remove no longer used code?

I don't have a specific plan. The immediate goal was to make Mixxx build with Qt6 because that unblocks Qt6 QML development.

Removing everything from the old skin system at once is a huge task, and such tasks almost always lead to people giving up or being too intimidate by the size to even start working on it.

Instead, this just removes QGLWidget, the weird vsync hacks, etc. and leaves everything else intact, which keeps the PR small. Removing the legacy skin stuff from the Qt6 build can be done in smaller PRs, every at the source file level. In the worst case, we can remove it once we go Qt6-only.

@Be-ing
Copy link
Contributor

Be-ing commented Jan 15, 2022

I suppose procrastinating the linking problems is one way forward... I'm still puzzled how you expect to ever remove dead code without rewriting the entire library system from scratch.

@Holzhaus
Copy link
Member Author

Holzhaus commented Jan 15, 2022

I suppose procrastinating the linking problems is one way forward... I'm still puzzled how you expect to ever remove dead code without rewriting the entire library system from scratch.

Well, the main goal of this PR is neither removing dead code nor rewriting the library system from scratch. The goal is making the Mixxx build with Qt6. I'm not saying the library system shouldn't be refactored or rewritten, or that we shouldn't remove the dead code at some point. But there is no need to connect these barely related topics and block progress on any of then unless the others are also solved. That kind of unnecessary scope creep is exactly what we previously identified as a problem with contributing to mixxx, and what wanted to avoid in the future. It scares people away from starting to work on it, leads to huge unreviewable PRs and generally makes stuff take way longer.

Now that Mixxx builds with Qt6, we don't have to fix all these issues sequentially anymore. We can work on stuff in parallel, for example:

  • @daschuer could investigate failing Qt6 tests,
  • @uklotzde could start replacing the library with aoide,
  • @Swiftb0y and @ronso0 could improve the QML skins and
  • I could remove the old legacy skins stuff.

And even if we don't remove the legacy skin code and don't replace the library yet, we already mostly solved the issue of Qt5 becoming deprecated. So in theory we could even publish a Qt6 Mixxx release where the dead code is still built (and hopefully removed by the linker).

@Holzhaus
Copy link
Member Author

Holzhaus commented Jan 16, 2022

OK, build succeeds but some tests fail when built with Qt6. I'll check the clazy warning, but fixing the tests is out of scope for this PR. It's a separate branch anyway.

@ywwg
Copy link
Member

ywwg commented Jan 16, 2022

Alternatively, you could disable the broken tests with a comment // Breaks under QT6. Unless you're worried people won't bother to fix the tests later :). But yes this sounds like a good plan.

@Holzhaus
Copy link
Member Author

Alternatively, you could disable the broken tests with a comment // Breaks under QT6. Unless you're worried people won't bother to fix the tests later :). But yes this sounds like a good plan.

These tests fail:

	293 - EngineSyncTest.ZeroLatencyRateChangeQuant (Failed)
	294 - EngineSyncTest.ZeroLatencyRateDiffQuant (Failed)
	295 - EngineSyncTest.ActivatingSyncDoesNotCauseDrifting (Failed)
	302 - EngineSyncTest.SyncPhaseToPlayingNonSyncDeck (Failed)
	304 - EngineSyncTest.UserTweakPreservedInSeek (Failed)
	311 - EngineSyncTest.SeekStayInPhase (Failed)
	724 - WPushButtonTest.LongPressLatchTest (Failed)

The WPushButtonTest doesn't matter and can be disabled, but the other tests probably fail because of this commit: 18236e6

I have no idea how this complicated and weird visualplayposition/vsync code is supposed to work and how to remove it properly.
As a hack, I just replaced it with the playposition CO. I'd appreciate if someone who is more familiar with it could have a look.

From the Qt docs:

> Note: This function is deprecated. For new code, use setSecsSinceEpoch().
>
> Source: https://doc.qt.io/qt-5/qdatetime-obsolete.html#setTime_t
This is more efficient and also fixes a potential issue with Qt 6:

    ‘QSqlQuery::QSqlQuery(const QSqlQuery&)’ is deprecated: QSqlQuery is not meant to be copied. Use move construction instead. [-Werror=deprecated-declarations]
      438 |                     parsePlaylistEntries(xml, current_path,
          |                     ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
In contrast to the `const char*` comparison, the comparison with
QLatin1String is forward compatible with Qt 6.
These version checks actually result in compiler errors when trying to
build this with Qt 6.
Qt6 replaces QStringRef with QStringView and updates the regular
QString::left()/QString::mid() methods to return those.
This is not ideal, the whole VisualPlayPosition/VSyncThead stuff needs
to be removed eventually.
This also fixes a test failure with Qt6. It's unknown why this happens,
but the new code works with both Qt5 and Qt6 and is more readable and
ideomatic anyway.
This makes is much easier to fix linker errors.
@Holzhaus
Copy link
Member Author

All green. Now that the separate qt6 branch is obsolete, I'll close this and open a new PR against main.

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.

4 participants