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

Qt6 port and a bunch of compiler warning fixes #5

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ximion
Copy link
Contributor

@ximion ximion commented May 6, 2024

Hi Adrian!

Qt5 is out of support unless you are paying for it, and I would like to use the IntanRHX code in a Qt6 application as well.
So this patch ports the tool to Qt6.

The required changes are not very invasive, the biggest alteration is porting to the latest QtMultimedia and the subsequent changes in the audio thread. I tested the changed code, and did not notice any differences to previous behavior.

The rest of the changes are primarily minor adjustments and replacement of deprecated code with the modern equivalent.

In addition to the Qt6 port, I also resolved a bunch of compiler warnings, some of which were indicating real issues, like functions not returning anything, or integer type mismatches.
I did not resolve all warnings though as I did not want to mess with your workflow and also did not want to introduce any accidental regressions (so I only made "safe" changes). So the following issues are still existing:

  • unused-but-set-variables
  • unused-variables
  • type-limits (mostly comparing a uint to < 0)

I also adjusted the CI and the Debian/Ubuntu package building pipeline.
What I did not do was to make the application explicitly build with both Qt6 and the older Qt5 - that would have complicated some areas of the code, and I did not see the need for it. I could try to add this if needed though.

Thank you for working on IntanRHX and I hope you find this patch series useful!
Best,
Matthias

@adrian-foy adrian-foy self-requested a review July 9, 2024 17:42
@ximion
Copy link
Contributor Author

ximion commented Jul 9, 2024

From the failing CI, it looks like there are some new string comparisons introduced that need a QStringLiteral so QString and QString are compared instead of QString and char* :-)

@adrian-foy
Copy link
Collaborator

adrian-foy commented Jul 9, 2024 via email

@ximion
Copy link
Contributor Author

ximion commented Jul 9, 2024

Give me a second, this should actually be fairly trivial to fix for me :-)

@ximion
Copy link
Contributor Author

ximion commented Jul 9, 2024

This change should resolve the issues and make CI green again.
Comparing uint < 0 makes no sense, so the compiler complained about this. I assume you want the sanity check, so instead of making the commands signed, which would be a behavior change with probably unforseen consequences, I make the compiler interpret the uint as int and then compared < 0. That means essentially that if a negative int is somehow passed as uint, we will catch it. It also means that MAXUINT values will be treated the same way and will be rejected. This depends on the platform's integer handling though, but on pretty much all platforms that this will run on, this should yield the originally intended result.

That's the only "extra" change added in the patch, the rest was needed to fix the build :-)
I hope this helps 😀

@ximion
Copy link
Contributor Author

ximion commented Jul 9, 2024

Once you have built the current Qt5 release, you should be able to merge this and then perform a Qt6 build of the software (for upcoming versions).
The Qt6 builds should actually be easier on all platforms, there are some nice QoL changes in Qt6 - also, the jump from 5 to 6 isn't actually that big in terms of changes needed.

This codebase could probably be made buildable with both versions of Qt (most changes made will work with both, except for the QtMultimedia changes), but I do not think doing so is worth the additional effort at all, both in maintenance and testing (I maintained dual Qt4/Qt5 projects briefly in the past, it was an absolute pain and not worth it at all).

If you wanted to, you could add Windows support to the CI and make it also spit out Windows binaries in addition to the Linux packages that it already generates for easy installation. Then, releasing binaries would be even easier (but Windows CI is much less fun to work with compared to the Linux one, from my experience with creating one for a driver app for the UCLA Miniscope).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants