Open
Conversation
Contributor
Author
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deepin-ci-robot The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideRefactors CMake configuration to derive versioning from the VERSION file, introduce a DTK5 build option that controls DTK/Qt major selection, and consistently use a DTK_NAME_SUFFIX-based naming convention across libraries, exports, examples, tests, and documentation, while adding a small runtime behavior tweak for DQuickDciIconImage. Sequence diagram for DQuickDciIconImage retainWhileLoading behaviorsequenceDiagram
participant QML as QMLCode
participant Img as DQuickDciIconImage
participant Priv as DQuickDciIconImagePrivate
participant Icon as DQuickIconImage
QML ->> Img: create DQuickDciIconImage
activate Img
Img ->> Priv: construct DQuickDciIconImagePrivate(this)
activate Priv
Priv ->> Icon: create DQuickIconImage
Priv ->> Img: connect DQuickIconImage::cacheChanged
Note over Priv,Img: For Qt >= 6.8.0
Priv ->> Img: connect DQuickIconImage::retainWhileLoadingChanged
Priv ->> Icon: setRetainWhileLoading(true)
deactivate Priv
deactivate Img
Flow diagram for DTK5 option-driven CMake configurationflowchart TD
A[Read VERSION file into FILE_VERSION] --> B[project DtkDeclarative VERSION FILE_VERSION]
B --> C{DTK5 option ON?}
C -- Yes --> D[set DTK_VERSION_MAJOR 5]
D --> E[set DTK_NAME_SUFFIX empty]
C -- No --> F[set DTK_VERSION_MAJOR 6]
F --> G[set DTK_NAME_SUFFIX 6]
D & F --> H[Derive DTK_VERSION_MINOR from PROJECT_VERSION_MINOR]
H --> I[Derive DTK_VERSION_PATCH from PROJECT_VERSION_PATCH]
I --> J[Compose DTK_VERSION = MAJOR.MINOR.PATCH]
J --> K[Set QT_VERSION_MAJOR = DTK_VERSION_MAJOR]
J --> L[Set LIB_NAME = dtk + DTK_NAME_SUFFIX + declarative]
J --> M[Set INCLUDE_INSTALL_DIR using DTK_VERSION_MAJOR]
J --> N[Set CONFIG_INSTALL_DIR using DTK_NAME_SUFFIX]
J --> O[Set DDECLARATIVE_TRANSLATIONS_DIR using DTK_VERSION_MAJOR]
C -- Yes --> P[Enable Qt5/DTK5 targets]
P --> P1[add_subdirectory src]
P --> P2[add_subdirectory qmlplugin]
C -- No --> Q[Enable Qt6/DTK6 targets]
Q --> Q1[add_subdirectory qt6]
C -- Yes --> R[USE_QQuickStylePluginPrivate via Qt::QuickControls2]
C -- No --> S[USE_QQuickStylePluginPrivate via Qt6::QuickControls2]
J --> T[Generate Dtk + DTK_NAME_SUFFIX DeclarativeConfig.cmake]
J --> U[Generate Dtk + DTK_NAME_SUFFIX DeclarativeConfigVersion.cmake]
J --> V[Generate dtk + DTK_NAME_SUFFIX declarative.pc]
C -- Yes --> W[Examples: BIN_NAME dtk-exhibition]
W --> W1[find_package DtkCore, DtkGui]
C -- No --> X[Examples: BIN_NAME dtk6-exhibition]
X --> X1[find_package Dtk6Core, Dtk6Gui]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `docs/CMakeLists.txt:20` </location>
<code_context>
+if (DTK5)
find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS Help)
else()
find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS ToolsTools)
</code_context>
<issue_to_address>
**issue (bug_risk):** Possible typo in Qt component name: "ToolsTools" looks duplicated.
`ToolsTools` is probably meant to be `Tools`. As written, this will likely cause `find_package(Qt...)` to fail and break the build.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (DTK5) | ||
| find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS Help) | ||
| else() | ||
| find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS ToolsTools) |
There was a problem hiding this comment.
issue (bug_risk): Possible typo in Qt component name: "ToolsTools" looks duplicated.
ToolsTools is probably meant to be Tools. As written, this will likely cause find_package(Qt...) to fail and break the build.
Contributor
|
Note
详情{
"CMakeLists.txt": [
{
"line": " HOMEPAGE_URL \"https://github.com/linuxdeepin/dtkdeclarative\"",
"line_number": 9,
"rule": "S35",
"reason": "Url link | e63b02d733"
}
]
} |
deepin-ci-robot
force-pushed
the
sync-pr-557-nosync
branch
from
750b7ab to
64164e4
Compare
Contributor
|
Note
详情{
"CMakeLists.txt": [
{
"line": " HOMEPAGE_URL \"https://github.com/linuxdeepin/dtkdeclarative\"",
"line_number": 9,
"rule": "S35",
"reason": "Url link | e63b02d733"
}
]
} |
Synchronize source files from linuxdeepin/dtkdeclarative. Source-pull-request: linuxdeepin/dtkdeclarative#557
deepin-ci-robot
force-pushed
the
sync-pr-557-nosync
branch
from
64164e4 to
f1a2926
Compare
Contributor
Author
deepin pr auto reviewCMakeLists 和代码变更审查总体评价这次变更主要重构了 DTK 项目中 Qt5/Qt6 版本的构建系统配置，将原来基于项目版本号判断的方式改为通过 CMake 选项显式控制。整体上提高了配置的灵活性和可维护性。 详细审查意见1. 版本控制逻辑改进优点：
建议： # 当前实现
option(DTK5 "Build DTK5." ON)
if(DTK5)
set(DTK_VERSION_MAJOR "5")
set(DTK_NAME_SUFFIX "")
else()
set(DTK_VERSION_MAJOR "6")
set(DTK_NAME_SUFFIX "6")
endif()
# 建议改进：增加对无效选项的检查
option(DTK5 "Build DTK5." ON)
if(DTK5)
set(DTK_VERSION_MAJOR "5")
set(DTK_NAME_SUFFIX "")
elseif(DTK6) # 假设未来添加DTK6选项
set(DTK_VERSION_MAJOR "6")
set(DTK_NAME_SUFFIX "6")
else()
message(FATAL_ERROR "Must specify either DTK5 or DTK6")
endif()2. 变量命名一致性问题：
建议： # 更明确的命名
file(READ "${CMAKE_CURRENT_SOURCE_DIR}/VERSION" DTK_PROJECT_VERSION)
string(STRIP "${DTK_PROJECT_VERSION}" DTK_PROJECT_VERSION)
project(DtkDeclarative
VERSION ${DTK_PROJECT_VERSION}
...
)
# 使用更一致的命名约定
set(DTK_MAJOR_VERSION "5") # 主版本号
set(DTK_VERSION_SUFFIX "") # 库名称后缀3. CMake 配置文件路径问题：
建议： # 确保配置文件路径一致
set(CONFIG_INSTALL_DIR "${CMAKE_INSTALL_LIBDIR}/cmake/Dtk${DTK_MAJOR_VERSION}Declarative" CACHE STRING "CMake config file install directory")4. C++ 代码变更问题： DQuickIconImage::DQuickIconImage(QQuickItem *parent)
: DQuickIconImage(*(new DQuickIconImagePrivate), parent) // 这里应该是QQuickImage
{
// setAsynchronous(true) 被移除，但没有说明原因
}建议： DQuickIconImage::DQuickIconImage(QQuickItem *parent)
: QQuickImage(*(new DQuickIconImagePrivate), parent) // 保持原有继承关系
{
setAsynchronous(true); // 保持原有行为，除非有特定原因移除
}5. 条件编译逻辑问题： 建议： # 显式定义两个选项
option(DTK5 "Build DTK5." ON)
option(DTK6 "Build DTK6." OFF)
# 使用更明确的条件
if(DTK5)
# Qt5 特定代码
elseif(DTK6)
# Qt6 特定代码
endif()6. 测试相关优点：
建议： if(BUILD_TESTING)
enable_testing()
add_subdirectory(tests)
# 添加更详细的测试依赖说明
add_dependencies(unit-test
${PLUGIN_NAME}
${STYLE_PLUGIN_NAME}
$<$<NOT:$<BOOL:${DTK5}>>:dtkdeclarativeprivatesplugin dtkdeclarativesettingsplugin>
)
endif()7. 文档生成问题： if (DTK5)
find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS Help)
else()
find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS ToolsTools)
endif()Qt6 的 建议： if (DTK5)
find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS Help)
else()
find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS Help) # Qt6 也使用 Help 组件
endif()8. 性能优化建议： # 在适当的地方使用生成器表达式减少重复配置
target_link_libraries(${LIB_NAME} PRIVATE
Qt${QT_VERSION_MAJOR}::Quick
Qt${QT_VERSION_MAJOR}::QuickControls2
Dtk${DTK_NAME_SUFFIX}::Core
Dtk${DTK_NAME_SUFFIX}::Gui
$<$<BOOL:${DTK5}>:Qt${QT_VERSION_MAJOR}::QuickCompiler>
)总结这次变更整体上提高了构建系统的可维护性，但还有以下几点可以改进：
建议在合并前进行全面的构建测试，确保所有平台和配置都能正常工作。 |
Contributor
|
Note
详情{
"CMakeLists.txt": [
{
"line": " HOMEPAGE_URL \"https://github.com/linuxdeepin/dtkdeclarative\"",
"line_number": 9,
"rule": "S35",
"reason": "Url link | e63b02d733"
}
]
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Synchronize source files from linuxdeepin/dtkdeclarative.
Source-pull-request: linuxdeepin/dtkdeclarative#557
Summary by Sourcery
Adjust build configuration to support selecting DTK5 or DTK6 via a DTK5 option and consistently derive versions and naming from a unified DTK version.
Enhancements:
Tests: