Open PhysSong opened 1 year ago
Opened #7133 to replace QRegExp
with QRegularExpression
. Might wanna add to the list.
MM_OPERATORS confilcts with getDefaultCtr/getCopyCtr/getMoveCtr of QMetaTypeInterface for SampleBuffer
does this still affect post the removal of MemoryManager
and Sample refactor?
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
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)?
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
Pull request #7179 handles the removal of QApplication::desktop
in ComboBox.cpp
.
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 c1ee2d8346bd9c725f167e4c2535b889cff997ef.
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.
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.
@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.
Thanks for the info, @Rossmaxx!
@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?
Is it okay if I push the changes into your branch?
Yes please.
Is it okay if I push the changes into your branch?
Yes please.
Ok, great! Done with commit bb4d04b5e71.
Here's a list of the build warnings that are currently given when building commit 684afdb7c9e 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 ifdef
s.
Edit: the file was created with the following command in the build directory:
make -j12 > build_out.txt 2> build_warnings.txt
Handle signature changes of QAbstractNativeEventFilter::nativeEventFilter in Qt 6
Any ideas on what to do here?
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.
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
Work around no official supports for Qt 6 on 32bit Windows
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.
The last parameter has type of long on Qt 5, but it is quintptr on Qt 6.
Added to #7086
Current status of Qt6 builds: Linux - works Mac - works Windows MSVC - works Windows MinGW - not planned (pretty sure Qt 6 is not available there)
Work around no official supports for Qt 6 on 32bit Windows
If Qt has dropped support for 32-bit OSs, I feel we should also drop support for 32-bit OSs.
Mac - untested (@tresf can you help pls?)
Fantastic work! Mac tested, works. I tested with Unfa Spoken demo track and it sounds fine.
AFAIR the DMG creation script on master is broken, so I didn't test that. Will sort that on master in a separate issue/PR.
Will sort that on master in a separate issue/PR.
Feel free to work on that qt6 branch. As of now, the plan is to do everything there and open one final PR consisting of all changes.
Windows MinGW - not planned (pretty sure Qt 6 is not available there)
If we decide to keep Mingw support around, we would require a PPA for this. There seems to be some disagreement on Discord as to whether or not this is worthwhile to keep around. Pinging @tobydox as an FYI in the event we decide to keep it. <3
Feel free to work [DMG generation issues] on that
qt6
branch. As of now, the plan is to do everything there and open one final PR consisting of all changes.
Sorry, this probably wasn't clear but the DMG issues are unrelated to Qt6. Master's DMG is only 46 MB currently (should be ~100MB) , so something's been wrong on nightly for quite some time. I'll investigate this issue and fix it directly on master.
Sorry, this probably wasn't clear but the DMG issues are unrelated to Qt6
I spoke too soon, some are. PR opened. #7240
Also, Zyn won't load on Apple. I've added a checklist item to the OP to track this..
This bug is present on master too, removing.
@DomClark I would appreciate if you could help me debug the qt6 build.
After applying the changes from #7086, it builds successfully for me (using Qt 6.7.1 and Visual Studio 2022). I'm happy to help debug your particular configuration, but it might be better to do that on Discord so as not to clutter this issue.
After applying the changes from #7086, it builds successfully for me
Forgot that that PR had a fix. Will close that PR and open a new one against the qt6 branch.
Apple + Zyn removed from the checklist, the issue is also present on master.
The libsndfile
issues fixed in #7240 are also present on master, opened a new PR against master.
Will update #7240 to simplify it to only Qt6-related changes.
Handle signature changes of QAbstractNativeEventFilter::nativeEventFilter in Qt 6
done via #7254
Either fix qt5-x11embed to support Qt 6 or drop XEmbed support for Qt 6
I have disabled X11 embedding for now in #7273
Handle removal of Qt X11 extras
We don't need to, since I disabled it instead along with X11 embedding.
Work around no official supports for Qt 6 on 32bit Windows
It would be better to not use 32 bit for Qt6 anyway, 32 bit builds can still use Qt5 (but mark 32 bit as deprecated as 1.3 and remove for future version)
Another item worth adding to the original checklist is handle removal of Qt6::Core5Compat
module used to get Hydrogen import plugin working.
QComboBox::activated(const QString&) is renamed to textActivated in Qt 5.14, breaking signal-slot connections
fixed in #7274
It would be better to not use 32 bit for Qt6 anyway, 32 bit builds can still use Qt5 (but mark 32 bit as deprecated as 1.3 and remove for future version)
Potentially unpopular opinion: I would be fine dropping 32-bit OS support entirely (NOT 32-bit VST support)
Areas where I can see 32-bit staying strong would be DIY/hobbyist/embedded systems, but those would be very specific use-cases and may be better suited for a core/UI separation effort.
I do agree but for that 1% of users who are still using pentiums, let them have a better version with less bugs.
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:
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.
Alright i agree completely now.
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)
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:)
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
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.
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.
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
.
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.
@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
@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.
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.
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.
QRegExp
withQRegularExpression
(#7133)MM_OPERATORS
confilcts withgetDefaultCtr
/getCopyCtr
/getMoveCtr
ofQMetaTypeInterface
forSampleBuffer
~: Removed in #7128QApplication::desktop
gui_templates.h
: useQWidget
's DPI instead(or entirely remove, if possible) (#7185)ComboBox.cpp
: useQWidget
's DPI instead(or entirely remove, if possible) (#7179)QAbstractNativeEventFilter::nativeEventFilter
in Qt 6 (#7254)qt5-x11embed
to support Qt 6 or drop XEmbed support for Qt 6 (dropped in #7273)QWheelEvent
may have both x and y deltas, potentially breakingSongEditor::wheelEvent
(see https://github.com/LMMS/lmms/pull/5619#issuecomment-673919832)QComboBox::activated(const QString&)
is renamed totextActivated
in Qt 5.14, breaking signal-slot connections