Code: Add Qt6 compatibility #2299

hoffie · 2022-01-27T21:46:42Z

Short description of changes

This PR adds the necessary changes to allow building with both Qt5 and Qt6 (Qt6 compatibility). It does not switch the autobuild targets (although it does contain changes/refactorings to make that switch simple).

The vast amount of development was done by @dcorson-ticino-com.
@softins had already helped with rebasing and his branch was of huge help to validate the changes in my branch.
I've tried to reflect those authorships in the commits, either as commit author or as Co-authored-by. If you're unhappy with any of this, please ping me and I'll adjust accordingly.

Two larger parts by @dcorson-ticino-com's tree are not part of mine, because I haven't found a reason to include that:

The Jamulus.pro update with win64 logic. I don't think it is necessary and I couldn't get it to work like that at all. The existing build logic already handles both 32bit and 64bit, so I think we should be fine.
The enum class ESvrRegStatus change. For me, the existing code compiled properly for both Qt5 and Qt6, but maybe I'm missing something?

Other than that, this PR differs in the following ways:

git history: Several parts of development iterations have been squashed together. At the same time, history has been split into independent changes. Therefore, it's very important that this PR is NOT squashed.
Mac Dock Badge Label handling has been restored
Autobuild code has been reworked to cover both the Qt6 use case and our existing requirements on master
I've done some minor cleanups such as moving Qt header includes to .h files instead of .cpp.

Context: Fixes an issue?

Qt6 support is mandatory as long as we use Qt, because there will be no more open source Qt5 releases. Relevant discussion: Moving post Qt 5.15.2 / Support for Qt6 #1795
Platform-specific bugs have come up (Mac performance issues/window focus issues, Windows 11 sound issues) without us knowingly breaking anything. So there's at least a very small hope that simply updating our framework to a version which officially supports those platforms could help there as well.
Apparently there is hope for Jamulus to become more screenreader-friendly

Does this change need documentation? What needs to be documented and how?

We should check if COMPILING.md or similar documentation should be updated.

CHANGELOG: Build: Initial support for building with Qt6 has been implemented (@dcorson-ticino-com, @softins)

Status of this Pull Request

Ready, I'm not planning further changes right now.

What is missing until this pull request can be merged?

Testing. Most of the changes are build-related and either work (compile) or don't (CI failure). I'm pretty confident that this PR is pretty safe for Qt5 and is a huge improvement for Qt6 (from "doesn't compile" to "compiles and runs"). There might be some bugs in Qt6 builds for sure.
There's one thing that should get explicit testing: The commit Mac: Use native Dock Badge handling drops Qt's macextras usage in favor of a small native ObjC wrapper. Therefore:

Create tests for the different flag handling code conversion approaches
Do Country code conversion in Settings to ensure that updates continue working
Confirm that Any  Silicon M1 native (no Rosetta 2) support? #2148 (reply in thread) is fixed
Mac: Confirm that Dock Label updates still work properly
Reviews: I suggest reviewing by commit, not (only) by full diff. I have added explanations to most of the commits.

All in all, the full diff is delightfully small and I hope to get this merged soon after 3.8.2 is done.

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

softins

Looking good so far. One small point. Maybe the conditional:

#if QT_VERSION >= 0x060000

should be changed to:

#if QT_VERSION >= QT_VERSION_CHECK( 6, 0, 0 )

for consistency with the existing version checks?

pljones · 2022-01-30T21:33:18Z

src/recorder/creaperproject.cpp

@@ -62,19 +62,19 @@ CReaperItem::CReaperItem ( const QString& name, const STrackItem& trackItem, con

    QTextStream sOut ( &out );



All of the recorder files want to flush eash line.

Hm, why would that be needed? Wouldn't it be just partial data anyway? I assumed it would be enough to ensure flushing after each "block". Such sOut.flush(); calls had been there anyway, see line 81, 107, 131 in this file.

If you really see a need to flush each individual line, what would be your preferred solution?
The ENDL define I've mentioned? After discussion with @softins I've included the simpler approach (\n) as it is more readable and does not seem to have any downsides in those specific cases.

Actually, in that case, could we drop the ; sOut from most of the lines and just have one big statement...

sOut << " <ITEM " << '\n' << " FADEIN 0 0 0 0 0 0" << '\n' ... << " >" << endl; sOut.flush();

(I'd still prefer the double flush.)

Actually, in that case, could we drop the ; sOut from most of the lines and just have one big statement...

Done (here and in line 118; the other places with 3 lines or less did not make sense, especially since clang-format would start putting those into a single line then).

(I'd still prefer the double flush.)

Do you mean endl? If so, why, is it any safer that way?
What should I do -- add a #define as initially thought? It sounded overkill and looked not that readable to me.

It was probably something I read on StackOverflow and may have been about Qt4 (as that's what we were targetting when this code was written). So it may not apply now. I think the concern was that sometimes there would be an intermediate buffer, so the first flush (endl) flushed to the file (which was buffered) and the second flush said "Oh, the first buffer is clean, I better flush the file buffer to disk". Or something.

It's fairly important to avoid missing the opportunity to save what's being written here -- the audio has been saved but the track timings relative to each other aren't easy to recover (but I've a script which helps).

src/recorder/jamcontroller.cpp

pljones · 2022-01-30T21:35:07Z

src/serverlogging.cpp

@@ -70,7 +70,7 @@ void CServerLogging::operator<< ( const QString& sNewStr )
    {
        // append new line in logging file
        QTextStream out ( &File );


The log file wants to flush on each write.

I agree, I mis-interpreted File.flush() which (probably?) only affects the underlying file, but not the QTextStream. I've updated this part to call out.flush() (but dropped the File.flush as I understand it is redundant in that case).

As noted above, I'd understood at some point that both were needed - the text stream was buffer and then the underlying file was separately buffered. The text stream flush gets to into the file but not necessarily to disk, which is why I think it was like this.

Again, as above, that might have been a Qt4 idiom that no longer applies - but it's worth checking if it's applicable in Qt5, at least, as we won't be moving to Qt6 for a long time, I expect.

pljones · 2022-01-30T21:36:37Z

This PR appears to mix up a number of different changes. Please split out those changes to the build scripts from changes to the source code.

Note that most of the changes to the source for Qt6 that are wanted have been done -- most of those included here do not appear to be wanted.

hoffie · 2022-01-30T22:32:32Z

Thanks for your comments, @softins and @pljones.

@softins

Maybe the conditional: #if QT_VERSION >= 0x06000 should be changed to #if QT_VERSION >= QT_VERSION_CHECK( 6, 0, 0 ) for consistency with the existing version checks?

That's certainly more readable and consistent. Updated, thanks!

@pljones

This PR appears to mix up a number of different changes. Please split out those changes to the build scripts from changes to the source code.

What do you mean by split out exactly? I've already invested lots of time to split those changes by their intention and did make them separate commits with explanations why they are needed. Do you mean to split those commits even further?
Or would you prefer them as individual PRs? I can do that, but the only comments regarding that question so far were that too many PRs would be more cumbersome, which is why I went the two-PR route.

So yes, this PR does contain both changes to source code and build logic because its intention is to provide Qt6 compatibility. It tries hard not to change existing behavior though. That's what #2300 is about.

It's also correct that this PR includes seemingly unrelated changes (refactorings), but I consider them a necessity. For example, the build script re-organization (especially on Windows) is necessary in order to allow building with two different Qt versions (while still keeping those two the same in this PR).
If you think that moving the build-related updates to the other PR (#2300) would be better, I can do that, of course. I think that's simply two different approaches. I had tried to distinguish between "safe to merge after review with no visible changes to release artifacts" vs. "change releases to Qt6", but distinguishing by type of change (code vs. build logic) would probably work as well, yeah. In the end, the separation into different commits is already there, so it's only a matter about how to distribute them among PRs.

Note that most of the changes to the source for Qt6 that are wanted have been done -- most of those included here do not appear to be wanted.

Not sure I follow. Do you suggest to drop specific changes from the PR, if so, which? In what way would they not be wanted?
I can only think of one previous merged PR with the goal of improving Qt6 compatibility, which was about QRegularExpression. That was certainly necessary, but not sufficient for Qt6 compatibility. This PR tries to add what's missing.

Edit: Maybe you are referring to small changes such as Document dmgbuild as reason for staying on macOS 10.15. Those are technically not needed for Qt6 compatibility, of course. I was just trying to follow a Clean Code principle. If such changes should rather be in their own PRs, I'd find that rather tedious, but if that's the consensus then I could do that as well, but they'll often be hard to work with due to a likely tendency for conflicts.

softins · 2022-01-30T22:55:07Z

If these changes are all related to the same objective (which I think they are), I don't see any point in splitting them into separate PRs for the sake of it. That just makes unnecessary work, which would have the same end result once they had all been merged. It's not as if the changes in this PR are huge and very many.

pljones · 2022-01-31T08:19:43Z

@pljones

This PR appears to mix up a number of different changes. Please split out those changes to the build scripts from changes to the source code.

What do you mean by split out exactly? I've already invested lots of time to split those changes by their intention and did make them separate commits with explanations why they are needed. Do you mean to split those commits even further? Or would you prefer them as individual PRs? I can do that, but the only comments regarding that question so far were that too many PRs would be more cumbersome, which is why I went the two-PR route.

So yes, this PR does contain both changes to source code and build logic because its intention is to provide Qt6 compatibility. It tries hard not to change existing behavior though. That's what #2300 is about.

It's also correct that this PR includes seemingly unrelated changes (refactorings), but I consider them a necessity. For example, the build script re-organization (especially on Windows) is necessary in order to allow building with two different Qt versions (while still keeping those two the same in this PR). If you think that moving the build-related updates to the other PR (#2300) would be better, I can do that, of course. I think that's simply two different approaches. I had tried to distinguish between "safe to merge after review with no visible changes to release artifacts" vs. "change releases to Qt6", but distinguishing by type of change (code vs. build logic) would probably work as well, yeah. In the end, the separation into different commits is already there, so it's only a matter about how to distribute them among PRs.

The point is to keep the opportunity for introducing problems as discrete as possible. If you're changing the build, then you know - by definition - any problems introduced into Jamulus's behaviour are because of changes to the build process, not the code.

If you change both at the same time, you have more possible places a single problem could be introduced, making it harder to diagnose.

Note that most of the changes to the source for Qt6 that are wanted have been done -- most of those included here do not appear to be wanted.

Not sure I follow. Do you suggest to drop specific changes from the PR, if so, which? In what way would they not be wanted? I can only think of one previous merged PR with the goal of improving Qt6 compatibility, which was about QRegularExpression. That was certainly necessary, but not sufficient for Qt6 compatibility. This PR tries to add what's missing.

None of the recorder changes are "missing" because they're either in another PR already or not wanted, as far as I know.

Edit: Maybe you are referring to small changes such as Document dmgbuild as reason for staying on macOS 10.15. Those are technically not needed for Qt6 compatibility, of course. I was just trying to follow a Clean Code principle. If such changes should rather be in their own PRs, I'd find that rather tedious, but if that's the consensus then I could do that as well, but they'll often be hard to work with due to a likely tendency for conflicts.

hoffie · 2022-01-31T10:29:01Z

The point is to keep the opportunity for introducing problems as discrete as possible. If you're changing the build, then you know - by definition - any problems introduced into Jamulus's behaviour are because of changes to the build process, not the code.
If you change both at the same time, you have more possible places a single problem could be introduced, making it harder to diagnose.

I fully agree there, that's why I've split everything into manageable pieces.
Do you suggest to convert those 16 commits into individual PRs? As said, I can do it, but I don't see the benefit to do that "discrete" part on a PR level instead of on a commit level. This would also be likely 16 PRs where most of those would have to be merged in a specific order as they would probably depend on each other.

None of the recorder changes are "missing" because they're either in another PR already or not wanted, as far as I know.

Could you be more specific, please? Are you referring to #1999 (which was not ready for merge and was closed)? This PR is based on @dcorson-ticino-com's work in #1999. Are you referring to other PRs? If so, which? Or had you been planning to open those?
What part exactly is "not wanted"?
I can only further improve this PR if I know what to do and currently I don't.

hoffie · 2022-01-31T11:42:36Z

Do you suggest to convert those 16 commits into individual PRs? As said, I can do it, but I don't see the benefit to do that "discrete" part on a PR level instead of on a commit level. This would also be likely 16 PRs where most of those would have to be merged in a specific order as they would probably depend on each other.

What I could do is: Move the refactoring parts in a single refactoring PR (along with other autobuild refactorings I'm currently doing). Would that be better?
It will still have the downside that this PR would depend on the refactoring PR.

pljones · 2022-01-31T17:12:16Z

Do you suggest to convert those 16 commits into individual PRs? As said, I can do it, but I don't see the benefit to do that "discrete" part on a PR level instead of on a commit level. This would also be likely 16 PRs where most of those would have to be merged in a specific order as they would probably depend on each other.

What I could do is: Move the refactoring parts in a single refactoring PR (along with other autobuild refactorings I'm currently doing). Would that be better? It will still have the downside that this PR would depend on the refactoring PR.

That's what I'm suggesting, yes. Do not try to do everything at once. That's why I have a stack of PRs waiting.

hoffie · 2022-01-31T23:28:59Z

Done. I have limited the scope of this PR to changes to the actual code. I've opened #2328 to cover the (auto)build changes. #2300 is unchanged for now and I suggest we'd ignore it for now (that's why it's a draft). I'll open yet another PR when the other (non-Qt6) autobuild refactorings are done.

src/audiomixerboard.cpp

src/util.h

src/connectdlg.cpp

Qt5 had a special qtmain library which took care of forwarding the MSVC default WinMain() entrypoint to the platform-agnostic main(). Qt6 is still supposed to have that lib under the new name QtEntryPoint. As it does not seem to be effective when building with qmake, we are rather instructing MSVC to use the platform-agnostic main() entrypoint directly. Without this change, building fails.

Jamulus uses the Qt5-internal enum values of Country codes within its protocol. Qt6 changed those assignments. This would lead to different behavior for users based on their Qt version. To avoid that, this commit adds a compatibility layer to translate Qt6 Country codes to their Qt5 equivalent and vice-versa. This affects the protocol traffic and any ini-stored country values for clients and servers. https://doc.qt.io/qt-5/qlocale.html#Country-enum https://doc.qt.io/qt-6/qlocale.html#Country-enum Co-authored-by: Tony Mountifield <tony@mountifield.org> Co-authored-by: Peter L Jones <peter@drealm.info> Co-authored-by: DonC <dcorson@ticino.com>

Required for Qt6 compatibility as they are no longer there.

Jamulus.pro

softins

I've read through the diffs and checked the CI, so happy to approve.

I haven't been able to do any tests myself though.

ann0see · 2022-04-16T21:50:20Z

Hmm. I'd be more happy if every platform is checked again before this is merged.

@hoffie what would you think?
What should be checked? Is another nightly sufficient?

hoffie · 2022-04-16T21:57:01Z

The number of platform-specific changes is low, the most invasive one (Mac badge handling) has been tested. I'd say that the general nightly/beta phase is sufficient.
This PR has technically two approvals so would be fine to merge, but as @pljones had lots of valuable input here I'd prefer if he can take a final look as well before merging.

ann0see · 2022-04-21T18:53:52Z

@pljones I'm going to do some final tests here (Linux + Windows) and then merge this PR. You can of course do another after-merge review.

ann0see · 2022-04-21T19:13:00Z

Windows ASIO didn't show anything going obviously wrong. I just noticed - again - that the meter style is now round LEDs by default, I think.

ann0see · 2022-04-21T19:16:44Z

Ok. Did the merge now since no real end user visible changes came up.

ann0see · 2022-04-21T19:49:07Z

Compiled via Qt6.3 on Windows and the images look a bit lo-res.

jujudusud · 2022-04-22T12:04:14Z

Do you want me to build it on ArchLinux with Qt 6.3?

ann0see · 2022-04-22T12:12:25Z

Not yet – unless you can verify that Qt6 doesn't introduce visible changes/bugs.

Previously, country codes in --serverinfo were interpreted natively. This worked for Qt5, but caused unintended changes on Qt6 builds as the codes differ. Not doing a conversion for --serverinfo was an oversight from the initial Qt6 compatibility work, which is now fixed with this change. Related: jamulussoftware#2299 Fixes: jamulussoftware#2809

hoffie added this to the Release 3.9.0 milestone Jan 27, 2022

hoffie requested review from softins and dcorson-ticino-com January 27, 2022 21:46

hoffie added this to Triage in Tracking (old) via automation Jan 27, 2022

hoffie moved this from Triage to Waiting on Team in Tracking (old) Jan 27, 2022

hoffie mentioned this pull request Jan 27, 2022

Autobuild: Use Qt6 for Windows 64bit #2300

Merged

14 tasks

softins reviewed Jan 30, 2022

View reviewed changes

hoffie force-pushed the Qt6-compat branch from 5929dfc to 52be1e0 Compare January 30, 2022 18:59

pljones reviewed Jan 30, 2022

View reviewed changes

src/recorder/jamcontroller.cpp Outdated Show resolved Hide resolved

pljones reviewed Jan 30, 2022

View reviewed changes

hoffie force-pushed the Qt6-compat branch from 52be1e0 to 63debc8 Compare January 30, 2022 22:34

hoffie force-pushed the Qt6-compat branch 2 times, most recently from bd64b2a to 16d8779 Compare January 31, 2022 23:17

hoffie changed the title ~~Build: Add Qt6 compatibility~~ Code: Add Qt6 compatibility Jan 31, 2022

hoffie mentioned this pull request Jan 31, 2022

Build logic: Add Qt6 compatibility #2328

Merged

6 tasks

pljones reviewed Feb 1, 2022

View reviewed changes

src/audiomixerboard.cpp Outdated Show resolved Hide resolved

pljones reviewed Feb 1, 2022

View reviewed changes

src/util.h Show resolved Hide resolved

hoffie force-pushed the Qt6-compat branch 3 times, most recently from 68ebd5f to 4292008 Compare February 2, 2022 20:13

pljones reviewed Feb 2, 2022

View reviewed changes

src/connectdlg.cpp Outdated Show resolved Hide resolved

hoffie force-pushed the Qt6-compat branch from 1d2f6b6 to 4e20b37 Compare April 16, 2022 15:56

hoffie and others added 4 commits April 16, 2022 21:47

Code: Use early returns to avoid deep nesting

8ffcb66

Build: Remove unused qt{mac,win}extras dependencies

3c335ab

Required for Qt6 compatibility as they are no longer there.

hoffie force-pushed the Qt6-compat branch from 4e20b37 to e4f2f9c Compare April 16, 2022 19:48

ann0see requested a review from softins April 16, 2022 20:36

hoffie added 2 commits April 16, 2022 22:52

iOS: Add Qt6 qmake installation workaround to fix build

deb9db1

iOS: Add additional options to disable IPA signing to fix build on Qt6

0b2ad5f

hoffie force-pushed the Qt6-compat branch from e4f2f9c to 0b2ad5f Compare April 16, 2022 20:52

hoffie commented Apr 16, 2022

View reviewed changes

Jamulus.pro Show resolved Hide resolved

softins approved these changes Apr 16, 2022

View reviewed changes

Tracking (old) automation moved this from Waiting on Team to In Progress Apr 16, 2022

hoffie requested a review from pljones April 16, 2022 21:57

ann0see moved this from In Progress to Waiting on Team in Tracking (old) Apr 21, 2022

ann0see merged commit 16c3a0d into jamulussoftware:master Apr 21, 2022

Tracking (old) automation moved this from Waiting on Team to Done Apr 21, 2022

hoffie mentioned this pull request Aug 30, 2022

Server: Fix --serverinfo country code misinterpretation on Qt6 #2829

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code: Add Qt6 compatibility #2299

Code: Add Qt6 compatibility #2299

hoffie commented Jan 27, 2022 •

edited by ann0see

Loading

softins left a comment

pljones Jan 30, 2022

hoffie Jan 30, 2022

pljones Feb 1, 2022

hoffie Feb 1, 2022

pljones Feb 2, 2022

pljones Jan 30, 2022

hoffie Jan 30, 2022

pljones Feb 2, 2022

pljones commented Jan 30, 2022

hoffie commented Jan 30, 2022 •

edited

Loading

softins commented Jan 30, 2022

pljones commented Jan 31, 2022

hoffie commented Jan 31, 2022

hoffie commented Jan 31, 2022

pljones commented Jan 31, 2022

hoffie commented Jan 31, 2022

softins left a comment

ann0see commented Apr 16, 2022 •

edited

Loading

hoffie commented Apr 16, 2022

ann0see commented Apr 21, 2022

ann0see commented Apr 21, 2022

ann0see commented Apr 21, 2022

ann0see commented Apr 21, 2022

jujudusud commented Apr 22, 2022

ann0see commented Apr 22, 2022

		@@ -62,19 +62,19 @@ CReaperItem::CReaperItem ( const QString& name, const STrackItem& trackItem, con

		QTextStream sOut ( &out );

Code: Add Qt6 compatibility #2299

Code: Add Qt6 compatibility #2299

Conversation

hoffie commented Jan 27, 2022 • edited by ann0see Loading

Checklist

softins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones commented Jan 30, 2022

hoffie commented Jan 30, 2022 • edited Loading

softins commented Jan 30, 2022

pljones commented Jan 31, 2022

hoffie commented Jan 31, 2022

hoffie commented Jan 31, 2022

pljones commented Jan 31, 2022

hoffie commented Jan 31, 2022

softins left a comment

Choose a reason for hiding this comment

ann0see commented Apr 16, 2022 • edited Loading

hoffie commented Apr 16, 2022

ann0see commented Apr 21, 2022

ann0see commented Apr 21, 2022

ann0see commented Apr 21, 2022

ann0see commented Apr 21, 2022

jujudusud commented Apr 22, 2022

ann0see commented Apr 22, 2022

hoffie commented Jan 27, 2022 •

edited by ann0see

Loading

hoffie commented Jan 30, 2022 •

edited

Loading

ann0see commented Apr 16, 2022 •

edited

Loading