Ubuntu25.10 wayland#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates DSView’s build system and platform integration to improve compatibility with newer environments (Qt6/Wayland, newer distros) and adds CI packaging support for Linux/Windows/macOS.
Changes:
- Bump CMake minimum version to 3.16 and enhance CMake packaging/install behavior (Linux DEB via CPack, macOS bundle support, updated install paths).
- Improve Qt6/Win32 compatibility by removing deprecated Qt5-only APIs and tightening conditional compilation.
- Add a GitHub Actions workflow to build, smoke-test, package, and publish release artifacts.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| DSView/test/CMakeLists.txt | Raises CMake minimum version for test build scripts. |
| DSView/pv/winnativewidget.cpp | Removes unused Qt includes and updates QKeyEvent construction for clarity. |
| DSView/pv/utility/path.cpp | Simplifies Windows path conversion using toLocal8Bit() (Qt6-friendly). |
| DSView/pv/utility/encoding.cpp | Makes locale codec initialization conditional for Win32 + Qt5; keeps Qt6 path on QStringConverter. |
| DSView/pv/storesession.cpp | Removes unused Windows-only QTextCodec include. |
| DSView/pv/mainwindow.cpp | Fixes Qt6 Windows screenshot capture path to avoid deprecated desktop APIs. |
| DSView/pv/mainframe.h | Guards QWinTaskbar APIs so Qt6 builds don’t include Qt5-only headers/members. |
| DSView/pv/mainframe.cpp | Adds Qt-version-specific Win32 initialization and guards taskbar progress logic. |
| CMakeLists.txt | Adds Homebrew/MSYS2 search hints, Windows-only sources, macOS bundle handling, Linux install path changes, and CPack DEB settings. |
| CMake/Findlibusb-1.0.cmake | Expands search paths and header/library name patterns for more platforms. |
| CMake/FindFFTW.cmake | Expands search paths and library name patterns for more platforms. |
| .gitignore | Un-ignores the workflow file that would otherwise match build*. |
| .github/workflows/build-release.yml | Introduces multi-OS build+package workflow and tag-based GitHub release publishing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(WIN32 AND EXISTS "/mingw64") | ||
| list(APPEND CMAKE_PREFIX_PATH "/mingw64") | ||
| list(APPEND CMAKE_INCLUDE_PATH "/mingw64/include") | ||
| list(APPEND CMAKE_LIBRARY_PATH "/mingw64/lib") | ||
| endif() |
There was a problem hiding this comment.
The MSYS2 path injection is keyed off WIN32 AND EXISTS "/mingw64", which can also be true on Windows machines where MSYS2 is installed but the project is being built with a non-MinGW toolchain (e.g., MSVC). That can lead to accidental mixing of headers/libs from /mingw64 with the selected compiler. Consider gating this on MINGW/MSYS (or a dedicated option/toolchain file) rather than WIN32, so the extra search paths are only applied for MinGW builds.
| install(FILES DSView/DSView.desktop DESTINATION share/applications RENAME dsview.desktop) | ||
| install(FILES DSView/DreamSourceLab.rules DESTINATION lib/udev/rules.d RENAME 60-dreamsourcelab.rules) |
There was a problem hiding this comment.
The install destinations for the desktop file and udev rules are hard-coded (share/applications and lib/udev/rules.d). On multi-arch systems (lib vs lib64) and when CMAKE_INSTALL_PREFIX is customized, this may not land in the correct location. Since GNUInstallDirs is included, consider using ${CMAKE_INSTALL_DATADIR}/applications and ${CMAKE_INSTALL_LIBDIR}/udev/rules.d to respect platform conventions.
| install(FILES DSView/DSView.desktop DESTINATION share/applications RENAME dsview.desktop) | |
| install(FILES DSView/DreamSourceLab.rules DESTINATION lib/udev/rules.d RENAME 60-dreamsourcelab.rules) | |
| install(FILES DSView/DSView.desktop DESTINATION ${CMAKE_INSTALL_DATADIR}/applications RENAME dsview.desktop) | |
| install(FILES DSView/DreamSourceLab.rules DESTINATION ${CMAKE_INSTALL_LIBDIR}/udev/rules.d RENAME 60-dreamsourcelab.rules) |
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
permissions: contents: write is granted for the entire workflow, including the build job that runs on pull_request. Even though fork PRs get a read-only token, same-repo PRs would run with a write-capable token, which is broader than needed for building/testing. Consider setting default workflow permissions to read-only and granting contents: write only on the release job (or even just the specific step) that uploads the GitHub Release.
| void MainFrame::setTaskbarProgress(int progress) | ||
| { | ||
| #ifdef _WIN32 | ||
| if (progress > 0) { | ||
| _taskPrg->setVisible(true); | ||
| _taskPrg->setValue(progress); | ||
| #if defined(_WIN32) && QT_VERSION < QT_VERSION_CHECK(6, 0, 0) | ||
| if (progress > 0) { | ||
| _taskPrg->setVisible(true); | ||
| _taskPrg->setValue(progress); | ||
| } else { | ||
| _taskPrg->setVisible(false); | ||
| } |
There was a problem hiding this comment.
In the Qt5/Win32 build, _taskPrg is only assigned in showEvent(), but setTaskbarProgress() dereferences it without a null check. If prgRate() is emitted before the first showEvent() (or if showEvent() doesn't run as expected), this can crash. Initializing _taskPrg to nullptr in the constructor and guarding in setTaskbarProgress() (and/or in showEvent() when progress() is unavailable) would make this robust.
No description provided.