Feature: Qt6 migration: Linux + Windows#37
Conversation
|
Amazing! I'll check it out tomorrow. It looks very promising at first glance. |
chrip
left a comment
There was a problem hiding this comment.
Thank you so much for this contribution! This is a significant Qt6 migration effort that touches build configuration and core Qt wrapper code. Really appreciate the community investment in modernizing the codebase. Below is my review with issues I've found.
Summary
This PR removes Qt5 support and hardens the codebase for Qt6 only. It updates CMake, qmake, signal/slot macros, X11 handling, and DPI detection logic. The general direction is correct, but there are several issues.
Critical Issues
1. qdpichecker.cpp - X11 DPI detection completely removed (Regression)
File: ChromiumBasedEditors/lib/qt_wrapper/src/qdpichecker.cpp
The original code had this block (inside GetMonitorDpi on Linux):
#ifdef _LINUX
if ( QX11Info::isPlatformX11() )
{
int _x11_dpix = QX11Info::appDpiX(nScreenNumber),
_x11_dpiy = QX11Info::appDpiY(nScreenNumber);
if ( nDpiX < _x11_dpix ) nDpiX = _x11_dpix;
if ( nDpiY < _x11_dpiy ) nDpiY = _x11_dpiy;
}
#endifThis is completely removed in the new version. This code was critical for accurate DPI detection on Linux/X11. On X11, QScreen::physicalDotsPerInchX/Y() frequently returns incorrect values. The QX11Info::appDpiX/Y call was the mechanism to get the correct DPI from the X server and override Qt's underreported values.
Impact: On X11 Linux with HiDPI monitors (e.g., 4K displays), DPI will likely be underreported, causing UI scaling issues. This is a high-severity regression for Linux users on X11.
Suggested fix: Keep the X11 DPI override logic, but use the Qt6 equivalent. In Qt6 on X11, QX11Info functionality moved to QtGui/private/qtx11extras_p.h (which was already included in the original Qt6 path). The code should be updated to:
#ifdef _LINUX
#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
#include <QtGui/private/qtx11extras_p.h>
#else
#include <QX11Info>
#endif
#endif
// ... in GetMonitorDpi:
#ifdef _LINUX
if (QX11Info::isPlatformX11())
{
int _x11_dpix = QX11Info::appDpiX(nScreenNumber),
_x11_dpiy = QX11Info::appDpiY(nScreenNumber);
if (nDpiX < _x11_dpix) nDpiX = _x11_dpix;
if (nDpiY < _x11_dpiy) nDpiY = _x11_dpiy;
}
#endif2. CMakeLists.txt - Hardcoded Qt root path (Build Portability)
File: ChromiumBasedEditors/lib/qt_wrapper/CMakeLists.txt:13
-set(QT_ROOT "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/qt/Qt-5.9.9/gcc_64/")
+set(QT_ROOT "/qt6/6.11.1/gcc_64")The hardcoded absolute path /qt6/6.11.1/gcc_64 will only work if Qt6 is installed at that exact location. This is a significant departure from the original vcpkg-relative path and means anyone building without Qt at /qt6/6.11.1/gcc_64 will have a broken build.
Suggested fix: Use a CMake variable or the Qt6_DIR environment variable for portability. Consider something like:
if(NOT QT_ROOT)
set(QT_ROOT "$ENV{QT6_ROOT}/6.11.1/gcc_64")
endif()Or make it configurable via CMAKE_PREFIX_PATH.
3. qdpichecker.cpp - #ifdef QT_VERSION_LESS_5_15 guard left dangling
File: ChromiumBasedEditors/lib/qt_wrapper/src/qdpichecker.cpp
The QT_VERSION_LESS_5_15 compile definition was removed from CMakeLists.txt, but qdpichecker.cpp still references it:
#ifdef QT_VERSION_LESS_5_15
#include <QDesktopWidget>
#endif
// ...
#ifndef QT_VERSION_LESS_5_15
int nScreenNumber = QApplication::screens().indexOf(w->screen());
#else
int nScreenNumber = QApplication::desktop()->screenNumber(w);
#endifSince QT_VERSION_LESS_5_15 is no longer defined (it was in target_compile_definitions and removed), the #else branch (QApplication::desktop()->screenNumber(w)) will never execute. This is fine for Qt6-only builds, but:
- The
#ifdef QT_VERSION_LESS_5_15guard around#include <QDesktopWidget>is now dead code — the include will never be pulled in. - The
#elsebranch inGetWidgetDpiis dead code.
Suggested fix: Since this is Qt6-only now, clean up the dead code paths. Remove the #ifdef QT_VERSION_LESS_5_15 / #else guards and keep only the Qt6 code. This also removes the stale #include <QDesktopWidget> include.
Minor Issues
4. qcefview_media.cpp - Unnecessary #include <qglobal.h>
File: ChromiumBasedEditors/lib/qt_wrapper/src/qcefview_media.cpp:51
#ifdef _LINUX
#include <qglobal.h> // <-- added
#if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)
#include <QX11Info>
#endifqglobal.h defines QT_VERSION and QT_VERSION_CHECK, but these are already available transitively through other Qt includes (like QWidget in qcefview.h which is included at the top of this file). The include is redundant.
Suggested fix: Remove the #include <qglobal.h> line.
5. CMakeLists.txt - Hardcoded Qt6 in target_link_libraries
File: ChromiumBasedEditors/lib/qt_wrapper/CMakeLists.txt:91
target_link_libraries(qtascdocumentscore PRIVATE Qt6::GuiPrivate)Since QT_VERSION_MAJOR is used throughout the rest of the file for Qt versioning, this should probably use Qt${QT_VERSION_MAJOR}::GuiPrivate for consistency.
Suggested fix: Use the variable: Qt${QT_VERSION_MAJOR}::GuiPrivate
6. CMakeLists.txt & videoplayerlib/CMakeLists.txt - QT_VERSION_6 as compile definition
target_compile_definitions(qtascdocumentscore PRIVATE ... QT_VERSION_6)
target_compile_definitions(videoplayerlib PRIVATE ... QT_VERSION_6)QT_VERSION_6 is typically defined by Qt itself via qglobal.h based on the actual Qt version detected at compile time. Manually defining it is redundant, and could cause issues if someone accidentally builds against Qt5 while the definition is still present. This is a minor concern but worth noting.
Nitpicks
qcefview.h:111/qcefview_media.h:51— Changingsignals:toQ_SIGNALS:andprotected slots:toprotected Q_SLOTS:is good practice for non-Qt build tool compatibility and forward compatibility. Good change.qtascdocumentscore.pro:42— The qmake removal of the Qt5/6 conditional forx11extras/gui-privateis clean and consistent with the Qt6-only direction.
Summary
| Severity | Issue |
|---|---|
| Critical | X11 DPI detection removed from qdpichecker.cpp — will break HiDPI Linux builds |
| High | Hardcoded Qt root path /qt6/6.11.1/gcc_64 — not portable |
| Medium | Dead code paths (QT_VERSION_LESS_5_15 guards) left in qdpichecker.cpp |
| Low | Redundant #include <qglobal.h> in qcefview_media.cpp |
| Low | Hardcoded Qt6::GuiPrivate instead of Qt${QT_VERSION_MAJOR}::GuiPrivate |
| Low | Manual QT_VERSION_6 compile definition is redundant |
Thanks again for the contribution. The Qt6 migration direction is correct and much needed. The main blocker is the DPI regression in qdpichecker.cpp — please address that before merging.
Assisted-by: OpenCode:Qwen3.6-27B
Signed-off-by: Peter P. Lupo <pplupo@gmail.com>
|
@rikled and @chrip , thank you so much for your kind comments. It took me a couple of days to do this, and your words made me feel the work was appreciated. @chrip , all of your findings are correct, accepted, and welcome. I believe these things are easy to do. Please bear with me for a second, I'll submit the fixes. |
|
@rikled Since this does not include any feature changes, not even bug fixes, I think I'd be ok with dropping qt5 for linux while someone esle works on it for windows/macos. I don't have windows or macos to test. Qt6 migration is long overdue, IMHO. If qt6 was a new release I'd say it would be important to keep qt5 support, but I don't think it's the case anymore. In any case, I'll support your decision. :-) I'm new to the project, I just want to add to it. |
|
Yes, I completely agree with you. Sorry for the misunderstanding. We can certainly drop Qt5 support for Linux. However, as long as we don't have a port to Qt6 for Windows/macOS yet, we should keep the option to use Qt5 for those |
* Fixed the `no space left on device` errors by migrating the Docker `buildx bake` cache mounts out of the restrictive `/tmp` RAM disk and into the host's `./.docker-cache` directory. * Disabled `docker buildx` log truncation (`BUILDKIT_STEP_LOG_MAX_SIZE=-1`) to prevent CMake compilation errors from being hidden behind the default 2MiB limit. * Updated submodules with final Qt 6 fixes. Signed-off-by: Peter P. Lupo <pplupo@gmail.com>
|
OK, I think I've addressed everything. Sorry for the wait, I could only look into this at the end of the day. I've made the build more resilient, but I was worried that others might try to build this branch and run into the same issues I had. My last commits should have addressed everything pointed out. Here's everything that I changed: 1. Main Repository (
|
|
I just tested it locally and it works perfectly. Great job! Thank you very much for your contribution! I'll try to get it working on Windows in the coming days, and maybe we then can just leave Qt5 behind for good. |
|
Now I'm eager to try a proper release. And we need to start looking into Wayland support too. I've posted on a thread about it. There are some things to discuss; it's not a straightforward port: #22 @rikled My pleasure! I've been eyeing this project since I read about it online. I wanted to offer a Canadian contribution to our allies in Europe. :-) Cheers! |
|
@pplupo I used your fork as a basis and got it working on Windows. Can I push it to your fork, or would you prefer that I create a new PR? |
|
@rikled , You could but I'm worried with the review effort. This branch hasn't been approved yet and if we add more it will get worse. |
|
Yes, Wayland support should definitely be in a separate PR. I'll add it to your fork; the changes mainly affect core, which is being updated to align with our current approach to handling third-party dependencies. Edit: I updated core, desktop-apps and desktop-sdk, but I am not allowed to update this PR. Could you grant me permission? |
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
|
@rikled I hadn't noticed your request to be included on my fork. I'm sorry. Of course, you are more than welcome to collaborate there if you think it's easier. I've just added you. I'm also thinking about adding support for MDB (MS Access) files so you can open them as a spreadsheet. I think it's important in a sovereignty project. We should be able to recover data. |
chrip
left a comment
There was a problem hiding this comment.
Thanks for pushing this forward, @pplupo — and @rikled for the Windows work. I went back through the actual code at the current submodule pointers (not just the change-log), and re-reviewed both the original Linux fixes and the newly-bundled Windows port. Short version: code looks good on both fronts, but this PR has quietly become a Linux and Windows migration and needs to be renamed and re-scoped so the Windows changes are actually visible to reviewers.
Please rename this PR: "Qt6 migration: Linux + Windows"
Right now the top-level PR diff is 6 files — three submodule SHA bumps plus two bake files and a script. But those SHA bumps pull in ~1,700+ lines of Windows-port work across all three submodules (core +1,524, desktop-apps, desktop-sdk): new MSVC/cygwin build tooling (qt/nc-build-qt.bat, icu-desktop/*, cef/nc-build.py), the Inno Setup installer scripts (package/inno/*), Win32 side-by-side manifests, and the CMake reworks. A reviewer looking at the PR would never know any of it is here.
The title/description still say "successfully builds the Linux application." Please update both to reflect Linux + Windows, and credit @rikled's commits — this shouldn't ride in as an invisible side-effect of a submodule bump. (macOS is still Qt5-only, which is fine — the version guards keep that path alive.)
✅ Original Linux review items — all resolved
Verified directly in the code at the current SHAs:
- X11 DPI regression —
qdpichecker.cppreinstatesQX11Info::appDpiX/YviaQtGui/private/qtx11extras_p.hunder a Qt6 guard. ✓ - Hardcoded Qt path — gone, and actually done well:
core/common.cmakenow resolves Qt viaQT6_ROOT→qt6_root.txt→ glob, with aFATAL_ERRORif not found. ✓ QT_VERSION_LESS_5_15dead code — removed (incl. the stale<QDesktopWidget>include). ✓- Redundant
#include <qglobal.h>— removed;<QX11Info>is now correctly guarded for Qt < 6. ✓ Qt6::GuiPrivate— nowQt${QT_VERSION_MAJOR}::GuiPrivate. ✓- Manual
QT_VERSION_6definition — removed; the code now usesQT_VERSION < QT_VERSION_CHECK(6,0,0)consistently. ✓
🪟 Windows port review (@rikled)
Read through the Windows delta — it's solid, well-guarded work:
main.cpp—Qt::AA_UseDesktopOpenGLon_WIN32to keep Qt's ANGLE from colliding with CEF'slibEGL/libGLESv2, with a clear comment. 👍- Macro migration —
!defined(QT_VERSION_6)→QT_VERSION < QT_VERSION_CHECK(6,0,0)incascapplicationmanagerwrapper.*andcaption.h; consistent with dropping the manual macro and keeps the Qt5 paths intact. desktop-sdk/.../lib/CMakeLists.txt— CEF/Boost/OpenSSL/ICU centralized intocommon.cmake,editors_helpernow pullsOfficeUtilsproperly viaadd_officeutils()(cleaner than the earlier "drop the invalid target" workaround), Linuxeditors_helpernow linkscef. Good.win-linux/CMakeLists.txt— cleanif(WIN32)/else()split for libs (libcefvscef X11 dl pthread rt z), guarded DBus/pkg-config, and theconverter/ Common-Controls v6 manifest dependencies. The inlineexecute_processICU/OpenSSL builds moving into the centralized third-party flow is a nice cleanup.
Minor / non-blocking:
win-linux/CMakeLists.txtWIN32 block hardcodesQt6::CorePrivate/GuiPrivate/PrintSupportPrivate— sameQt${QT_VERSION_MAJOR}::consistency nit as item #5 above. Low priority.corere-adds.docker/third-party.bake.Dockerfile(+67) while the top-levelbuild/linux/docker-bake.hcldrops thethird-partytarget. Looks intentional (third-party prep now happens incommon.cmake/core-base), but please confirm the standalone core Dockerfile is still wired into core's own pipeline and not orphaned.
🟡 Build / infra files
docker-bake.hcl(/tmp→./.docker-cache) andthird-partytarget removal — both good. Please add.docker-cache/andbuild_output.logto.gitignore(there's no root.gitignorecovering them, andrun_remote_build.shwrites the log to the repo root).run_remote_build.sh— this is a personal dev helper (placeholderssh://user@server, hardcodedeuro-officecache paths, exists "so the AI can read" the log). I'd rather we not commit it; move it to a documentedscripts/*.templateor drop it. (To be clear: this is not a DCO problem — all commits are signed off and the DCO check is green; it's just not something the repo needs to carry.)
CI
The pipeline isn't green — build-common is failing and build-linux/build-windows are skipped. Per @rikled's note this may be the routine not resolving the fork branch; let's get that sorted so the checks actually validate both platforms before merge.
Direction is excellent and I really want this in. Once the PR is re-titled/scoped to Linux + Windows and CI is green, I think we're basically there — the code itself is in good shape on both platforms. Thanks again, both of you. 🙏
Assisted-by: Claude Code (Opus 4.8)
Signed-off-by: Peter P. Lupo <pplupo@users.noreply.github.com>
Signed-off-by: Peter P. Lupo <pplupo@gmail.com>
|
@chrip I believe I have addressed your comments. Please, check. :-) I'm also eager to see this through so we can move on to Wayland support. |
|
@pplupo Confirmed — you've addressed everything. Re-checked against the current head:
My approval from the 29th stands. 🎉 Two minor, non-blocking nits from my last pass are still open if you want to fold them into a follow-up (no need to hold this PR):
Excellent work, both of you — looking forward to the Wayland PR next. 🙏 Assisted-by: Claude Code (Opus 4.8) |
Signed-off-by: Peter P. Lupo <pplupo@gmail.com>
|
Solved: core still ships .docker/third-party.bake.Dockerfile while the top-level bake drops the third-party stage — just confirm it's still wired into core's own pipeline and not orphaned. The wayland PR has already been created, @chrip . You can already start taking a look at it. |
Signed-off-by: Hendrik Leidinger <hendrik.leidinger@nextcloud.com>
|
@pplupo Thank you and congrats for getting your first contribution in! :) Another thing: As you are Nextcloud contributor you are invited to join our yearly conference in Berlin: https://nextcloud.com/conference-2026/ (I know this is probably a long trip for you, but I wanted to mention it anyway :)) (You are also very welcome to give a lightning talk about your contribution(s) if you like!) |
|
@rikled Thank you so much. and... mine Deustch is nicht sehr gut (that's me trying from memory). Now I just want to get the other contributions in: native wayland support (runtime - same binary works on x11), database support (so we can recover data from ms access databases + duckdb/parquet/sqlite) and the misspell wavy underline (which is much more visible and much more familiar to users of a certain other office suite. ;-) |
Overview
This PR orchestrates the project-wide migration of the Desktop Editors application suite from Qt 5 to Qt 6.11.1 for both Linux and Windows.
Changes
This commit bumps the tracking pointers for the
core,desktop-apps, anddesktop-sdksubmodules to include the following Qt 6 porting efforts:QT_NO_KEYWORDSusingQ_SIGNALSandQ_SLOTS.Testing
depends on: Euro-Office/core#110, Euro-Office/desktop-apps#28, Euro-Office/desktop-sdk#8