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

Add Qt 6 support #6614

Open
19 of 26 tasks
PhysSong opened this issue Jan 17, 2023 · 62 comments · May be fixed by #7339
Open
19 of 26 tasks

Add Qt 6 support #6614

PhysSong opened this issue Jan 17, 2023 · 62 comments · May be fixed by #7339

Comments

@PhysSong
Copy link
Member

PhysSong commented Jan 17, 2023

Working branch: https://github.com/LMMS/lmms/tree/qt6

As 2023-05-26 is the EOL of Qt 5.15(without subscription licenses), it's worth starting working on Qt 6 support. Since we require C++17 and has helpers for deprecated Qt features, it might be not as hard as adding Qt 5 support.

See here for more information about how to port from Qt5 to Qt6: https://doc.qt.io/qt-6/portingguide.html

I made a checklist for Qt 6 support. Please feel free to suggest something to add.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Mar 3, 2024

Opened #7133 to replace QRegExp with QRegularExpression. Might wanna add to the list.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Mar 3, 2024

MM_OPERATORS confilcts with getDefaultCtr/getCopyCtr/getMoveCtr of QMetaTypeInterface for SampleBuffer

does this still affect post the removal of MemoryManager and Sample refactor?

@Snowiiii
Copy link
Contributor

Snowiiii commented Mar 3, 2024

I saw we could use Clazy to port much of the Code to Qt6, Here is the doc page: https://doc.qt.io/qt-6/porting-to-qt6-using-clazy.html, I already tried it out and it works good, This Page is also an great Resource for Porting over: https://doc.qt.io/qt-6/portingguide.html

@sakertooth
Copy link
Contributor

sakertooth commented Mar 17, 2024

MM_OPERATORS confilcts with getDefaultCtr/getCopyCtr/getMoveCtr of QMetaTypeInterface for SampleBuffer

MemoryManager was removed in #7128. I think we can remove this one (or just check it off)?

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Mar 28, 2024

Handle removal of QApplication::desktop
in gui_templates.h: use QWidget's DPI instead(or entirely remove, if possible)

done via #7159

Also, i didn't find any uses of QApplication::desktop in ComboBox.cpp

I tried to enable experimental qt6 support via a flag at #7182

@michaelgregorius
Copy link
Contributor

Pull request #7179 handles the removal of QApplication::desktop in ComboBox.cpp.

@michaelgregorius
Copy link
Contributor

michaelgregorius commented Mar 29, 2024

The Linux and mingw builds fail for pull request #7179 because the packaged Qt versions are too old. QWidget::screen has been introduced with 5.14 and for example Ubuntu 18.04 only provides 5.9.5.

Edit: Fixed with commit c1ee2d8.

@Rossmaxx
Copy link
Contributor

#7182 is now in a mergeable state. Would help if anyone from here would review it.

@michaelgregorius
Copy link
Contributor

Hi @Rossmaxx,

I have checked out the branch and have run CMake as follows:

cmake .. -DCMAKE_INSTALL_PREFIX=../target-qt6 -DWANT_QT6=True

This leads to the following error with regards to ZynAddSubFx:

[...]
-- Configuring done (7.0s)
CMake Error at plugins/ZynAddSubFx/CMakeLists.txt:117 (target_link_libraries):
  The link interface of target "ZynAddSubFxCore" contains:

    Qt5::Widgets

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.



CMake Error at plugins/ZynAddSubFx/CMakeLists.txt:186 (TARGET_LINK_LIBRARIES):
  Target "RemoteZynAddSubFx" links to:

    Qt5::Core

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.



-- Generating done (0.4s)
CMake Generate step failed.  Build files cannot be regenerated correctly.

@michaelgregorius
Copy link
Contributor

I'd like to propose to create a "qt6-migration" branch in the official repository using the changes already done in #7182. That way the code could be worked on collaboratively without the need for intermediate pull requests until everything compiles/works. Once that state is reached there could then be one "official" pull request using that branch where some final "polishing" might be discussed.

@Rossmaxx
Copy link
Contributor

@michaelgregorius i forgot about that one. For now, cmake configuration only works if you set -DLMMS_MINIMAL=ON. A bit clunky and lacks features but that's the only way for now.

@michaelgregorius
Copy link
Contributor

Thanks for the info, @Rossmaxx!

@michaelgregorius
Copy link
Contributor

michaelgregorius commented Apr 11, 2024

@Rossmaxx, I have made adjustments to your branch so that minimal mode now fully compiles. Is it okay if I push the changes into your branch?

@Rossmaxx
Copy link
Contributor

Is it okay if I push the changes into your branch?

Yes please.

@michaelgregorius
Copy link
Contributor

Is it okay if I push the changes into your branch?

Yes please.

Ok, great! Done with commit bb4d04b.

@michaelgregorius
Copy link
Contributor

michaelgregorius commented Apr 20, 2024

Here's a list of the build warnings that are currently given when building commit 684afdb from @Rossmaxx's repository: build_warnings.zip.

We will need to check how to deal with the different types of warnings. If we are lucky they can be fixed without some version specific ifdefs.

Edit: the file was created with the following command in the build directory:

make -j12 > build_out.txt 2> build_warnings.txt

@Rossmaxx
Copy link
Contributor

Handle signature changes of QAbstractNativeEventFilter::nativeEventFilter in Qt 6

Any ideas on what to do here?

@PhysSong
Copy link
Member Author

Handle signature changes of QAbstractNativeEventFilter::nativeEventFilter in Qt 6

Any ideas on what to do here?

The last parameter has type of long on Qt 5, but it is quintptr on Qt 6.

@michaelgregorius
Copy link
Contributor

Depending on the surrounding code this might also be solvable with a helper method that uses a type as the last parameter that's defined via a using depending on the Qt version used.

So similar to what was for example done for ControlLayout.h here: https://github.com/LMMS/lmms/pull/7182/files#diff-6e9cbb14ccab26c8ddbde9070cef9344193bb144261c3a89c42f001f327059f7

@Rossmaxx
Copy link
Contributor

Work around no official supports for Qt 6 on 32bit Windows

#7212 has gotten rid of the requirement for 32 bit qt for 64 bit windows. Check it off?

@michaelgregorius
Copy link
Contributor

michaelgregorius commented Apr 25, 2024

I'd like to propose to create a "qt6-migration" branch in the official repository using the changes already done in #7182. That way the code could be worked on collaboratively without the need for intermediate pull requests until everything compiles/works. Once that state is reached there could then be one "official" pull request using that branch where some final "polishing" might be discussed.

For anyone interested: that branch is now available at https://github.com/LMMS/lmms/tree/qt6.

@Rossmaxx
Copy link
Contributor

The last parameter has type of long on Qt 5, but it is quintptr on Qt 6.

Added to #7086

@tresf
Copy link
Member

tresf commented May 21, 2024

I do agree but for that 1% of users who are still using pentiums, let them have a better version with less bugs.

With that mentality, why not offer 32-bit Linux binaries? We can now, thanks to #7252. Egh...

Let's look at this from 1,000 feet:

  • We've NEVER offered 32-bit prebuilt images for Apple
    • Apple deprecated 32-bit support 7 years ago
  • We've NEVER offered 32-bit prebuilt images for Linux
    • Until this month, our build toolchain didn't offer support for 32-bit
  • Windows 11 does not offer a 32-bit version
    • Windows 10 32-bit is end-of-life late next year
  • 99% of users across all platforms don't use 32-bit OSs as their primary machine.

The data is stacked against any notion of keeping 32-bit around; If our aim is to move forward, dropping 32-bit is a step in the right direction. Arguments against this should come with fact and data, not grandeur. <3

Lastly, keeping "support" for 32-bit in the codebase is NOT THE SAME as removing it from our downloads. There's still people compiling LMMS against PPC, but there's absolutely no expectation of it appearing in our downloads area.

@Rossmaxx
Copy link
Contributor

Alright i agree completely now.

@Rossmaxx
Copy link
Contributor

A QWheelEvent may have both x and y deltas, potentially breaking SongEditor::wheelEvent()

I can't wrap my head around what to do over here. Pinging @PhysSong for assistance (I'm a bit unfamiliar with qt code in that area so would rather delegate it to you)

@tresf
Copy link
Member

tresf commented May 21, 2024

A QWheelEvent may have both x and y deltas, potentially breaking SongEditor::wheelEvent()

I can't wrap my head around what to do over here. Pinging @PhysSong for assistance (I'm a bit unfamiliar with qt code in that area so would rather delegate it to you)

Yeah it would likely be a refactor, but I'm really not sure how breaking this is to our codebase... These days I use a trackpad with LMMS almost exclusively. The areas that we check for mouse direction are historically up/down. Although we may also want to support left/right explicitly, in my experience it "just works".

For example (Qt5:)

  • ✅ The zoom slider on a trackpad already works with both scroll directions.
  • 🚫 The mixer sliders do not work with both scroll directions
  • ✅ The song editor/pattern editor already works with both scroll directions.
  • 🚫 The knobs do not work with both scroll directions
  • ... etc

I feel like this point may be unrelated to Qt6 as a whole and better suited for general usability improvements, to the likes of:

I too would like some more context as to this Qt6 requirement @PhysSong

@PhysSong
Copy link
Member Author

Quoting one of my comments in #5619(also linked in the post):

Qt 5 generates two events(vertical one and horizontal one) for arbitrary scrolling to be compatible with Qt 4. That's not true in Qt 6 anymore.

This introduces a behavioral change: a diagonal scroll input won't result in actual diagonal scrolling anymore in case there is some direction-dependent logic that is not properly upgraded for Qt 6.

@tresf
Copy link
Member

tresf commented May 21, 2024

Thanks, however, it is still not clear to me the implication of this change. I read most of #5619 and I still do not understand the implication of this change.

@michaelgregorius
Copy link
Contributor

Here's a list of all classes that override wheelEvent and that might need to be checked:

include/AutomatableSlider.h
include/AutomationEditor.h
include/ComboBox.h
include/DeprecationHelper.h
include/Fader.h
include/FloatModelEditorBase.h
include/LcdFloatSpinBox.h
include/LcdSpinBox.h
include/MidiClipView.h
include/PianoRoll.h
include/SongEditor.h
include/TabWidget.h
include/TrackContainerView.h
include/TrackView.h
plugins/AudioFileProcessor/AudioFileProcessorWaveView.h
plugins/Compressor/CompressorControlDialog.h
plugins/Eq/EqCurve.h
plugins/SlicerT/SlicerTWaveform.h
plugins/Vectorscope/VectorView.h

The list is a slightly filtered result of grep -rl " wheelEvent" | sort. I have for example removed everything found in plugins/CarlaBase.

@FyiurAmron
Copy link
Contributor

FyiurAmron commented May 27, 2024

I do agree but for that 1% of users who are still using pentiums, let them have a better version with less bugs.

With that mentality, why not offer 32-bit Linux binaries? We can now, thanks to #7252. Egh...

Let's look at this from 1,000 feet:

  • We've NEVER offered 32-bit prebuilt images for Apple

    • Apple deprecated 32-bit support 7 years ago
  • We've NEVER offered 32-bit prebuilt images for Linux

    • Until this month, our build toolchain didn't offer support for 32-bit
  • Windows 11 does not offer a 32-bit version

    • Windows 10 32-bit is end-of-life late next year
  • 99% of users across all platforms don't use 32-bit OSs as their primary machine.

The data is stacked against any notion of keeping 32-bit around; If our aim is to move forward, dropping 32-bit is a step in the right direction. Arguments against this should come with fact and data, not grandeur. <3

Lastly, keeping "support" for 32-bit in the codebase is NOT THE SAME as removing it from our downloads. There's still people compiling LMMS against PPC, but there's absolutely no expectation of it appearing in our downloads area.

Seconded. What @tresf wrote is gold here. The code can be still kept 32-bit-compatible if anyone wants to put that effort in, but first-class support for almost totally obsolete technology simply drains available dev resources really quickly. Can we go with e.g. a ticket created explicitly for dropping 32-bit support or something similar? The sooner the better, IMO - TBH, while working on updating parts of LMMS, trying to maintain 32-bit-compatibility in the build process got messy really quickly. E.g. https://github.com/FyiurAmron/lmms/actions/runs/9260084064/job/25473203303 is just a real-world example of that. While 64-bit compilation finally went ahead with no problem, I'm still debugging what's exactly wrong with the 32-bit one :D

Side note: I'm mostly a Windows 7 user (sic!), but if someone would say "let's drop support for Windows 7", I wouldn't bat an eye, as long as 7 wouldn't be artificially blocked from running the executable. I often use obsolete OSes and apps, but that doesn't mean I want the core devs wasting time on supporting them instead of doing more important stuff, like fixing bugs or adding long-awaited features.

@Rossmaxx
Copy link
Contributor

@michaelgregorius i got a random idea just now. Instead of disabling x11 embed, is there a way we can make it work using Core5Compat module, since we use it anyway? Even if it means modifying the x11embed sources, i believe @lukas-w will be willing to merge it in with correct ifdefs

@michaelgregorius
Copy link
Contributor

@Rossmaxx, I have no real overview of the X11 embed implementation and Core5Compat. There's also the question if Core5Compat might be deprecated with Qt 7, i.e. how long such an implementation will carry us. I'll let people that are more knowledgeable in that area answer that question.

@Rossmaxx
Copy link
Contributor

if Core5Compat might be deprecated with Qt 7

Hopefully, we will get done with 2.0 and different frontends by the time qt 6 will reach eol. Also, i believe we will find a better solution by then. I am looking for better solution now too tbh.

The best solution for the embedding problem which i got was, believe it or not, disabling embedding but there are some ui related backlash for that solution, that's why i didn't go that route.

I do hope to find a more long lasting solution by the time we land qt 6 on master.

@michaelgregorius
Copy link
Contributor

Ubuntu intends to remove Qt 5 for the 26.04 TLS release: https://discourse.ubuntu.com/t/removing-qt-5-from-ubuntu-before-the-release-of-26-04-lts/49296

This should be considered when working on this issue and when putting in effort to enable the code to support both versions. Perhaps the latter is not really worth it. I assume that all popular distributions that LMMS is built for already have Qt 6 packages.

@tresf
Copy link
Member

tresf commented Nov 13, 2024

I agree it's important to move forward, but having the option to revert is a safety net we can use while we iron out unforseen bugs. Furthermore, if the standalone Qt5 installer is available from Qt's website, that may still offer a sanctioned path for development.

@bratpeki
Copy link
Member

bratpeki commented Jan 26, 2025

I'm adding QT5 SVG support to the MSYS building (note the icons in the Spectrum Analyzer):

Image

This is done by adding this installation step to the MSYS build instructions:

pacboy -S qt5-svg:p

The Qt6 SVG package is available on the MSYS package repo, so @PhysSong, if you can, please note in the list about that we gotta swap qt5 for qt6 in that line on the Wiki!

@tresf
Copy link
Member

tresf commented Jan 26, 2025

I'm adding QT5 SVG support to the MSYS building (note the icons in the Spectrum Analyzer):

Thanks! Reference: https://github.com/LMMS/lmms/wiki/Dependencies-Windows/_compare/b561a52ab5b428e714e6070842a32667751a6e38...f375ff4d5c9ec2fbc8333b02e184528724f2bb5a

@tresf
Copy link
Member

tresf commented Feb 18, 2025

PR has been rebased against master.

Closed items:

Open items:

  • GitHub Actions: @messmerd added, but currently failing.

  • Packaging:

    • Windows: (TBD)

@tresf
Copy link
Member

tresf commented Feb 18, 2025

PR has been rebased against master.

  • QWheelEvent: No issues observed. We will monitor this issue, but it will not hold up merge.

Spoke too soon. I can no longer use the trackpad for moving around the song editor. Reopening.

@michaelgregorius

This comment has been minimized.

@tresf

This comment has been minimized.

@FyiurAmron

This comment has been minimized.

@tresf

This comment has been minimized.

@michaelgregorius

This comment has been minimized.

@michaelgregorius

This comment has been minimized.

@tresf
Copy link
Member

tresf commented Feb 24, 2025

Spoke too soon. I can no longer use the trackpad for moving around the song editor. Reopening.

Patched 10632b3, marked as complete above.

@tresf
Copy link
Member

tresf commented Feb 24, 2025

GitHub Actions: @messmerd will soon be adding to CI.

@messmerd has added the MSVC Qt6 CI jobs to the PR. They're currently failing. He's recently obtained a Windows machine for testing so that he can fix the issues locally instead of relying on the CI, but in the meantime any help over there is appreciated. Next task on the list is to test Linux packaging.

@tresf
Copy link
Member

tresf commented Feb 24, 2025

Linux packaging is working after a workaround:

Pushed patch to qt6 branch, marked as complete above.

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

Successfully merging a pull request may close this issue.

9 participants