Migrate to CMake/Qt6 and modernize code (regex, encoding, GUI centering, build)#3
Migrate to CMake/Qt6 and modernize code (regex, encoding, GUI centering, build)#3
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd45591e8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| project(qnapi VERSION 0.0.0 LANGUAGES CXX) | ||
|
|
||
| set(CMAKE_CXX_STANDARD 11) |
There was a problem hiding this comment.
Raise CMake C++ standard to 17 for Qt6 targets
This migration enables Qt6 but still forces CMAKE_CXX_STANDARD to 11, which breaks CMake-based builds on Qt6 toolchains that require C++17-capable consumers. In practice this causes compile-time failure for users trying the new CMake path, so the build configuration introduced here is not usable without bumping the language standard.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,26 @@ | |||
| cmake_minimum_required(VERSION 3.21) | |||
|
|
|||
| project(qnapi VERSION 0.0.0 LANGUAGES CXX) | |||
There was a problem hiding this comment.
Use real project version instead of 0.0.0
The top-level CMake project version is hardcoded to 0.0.0 while this same change advertises a 0.4.0 release in Debian metadata. Any packaging or artifact naming that reads PROJECT_VERSION will emit incorrect versioned outputs, which can break release automation and produce misleading package filenames.
Useful? React with 👍 / 👎.
| EncodingUtils encodingUtils; | ||
| QStringList codecs = encodingUtils.availableEncodings(); | ||
| std::sort(codecs.begin(), codecs.end()); |
There was a problem hiding this comment.
Restore full encoding list for “Show all encodings”
Switching this path from QTextCodec::availableCodecs() to EncodingUtils::availableEncodings() silently narrows “show all” to the same short internal codec list used for heuristics, so users lose access to many previously selectable encodings. This also makes existing configs with values outside that shortlist fail findText(...), increasing the chance those settings are unintentionally replaced when options are saved.
Useful? React with 👍 / 👎.
Motivation
QRegExp,QDesktopWidget,qrand,QTextCodec) with modern equivalents for maintainability and correctness.Description
CMakeLists.txtand subdirectory CMake targets forlibqnapi,gui, andcli, and create an alias targetqnapi::libqnapi.debian/controlanddebian/changelogto version0.4.0and switch runtime deps to Qt6 (libqt6*).QDesktopWidgetwithQGuiApplication::primaryScreen()->availableGeometry()and remove obsolete headers accordingly.QRegExp/QRegExp::exactMatchusages withQRegularExpressionand switch other parsing to the newer API, e.g. subtitle format parsers,p7zipoutput parsing and argument splitting/joining.EncodingUtilsto centralize encoding detection and conversions usingQStringConverterand migrate file I/O and subtitle processing to useEncodingUtilsinstead ofQTextCodec/QTextStream.qrand/qsrandusage withQRandomGenerator::global()->generate()for tmp filename generation and useqEnvironmentVariable("PATH")instead ofQProcess::systemEnvironment()to discover executables.Testing
Codex Task