Skip to content

sync: from linuxdeepin/dde-session-shell#521

Merged
Ivy233 merged 1 commit into
masterfrom
sync-pr-74-nosync
Jul 14, 2026
Merged

sync: from linuxdeepin/dde-session-shell#521
Ivy233 merged 1 commit into
masterfrom
sync-pr-74-nosync

Conversation

@deepin-ci-robot

Copy link
Copy Markdown
Contributor

Synchronize source files from linuxdeepin/dde-session-shell.

Source-pull-request: linuxdeepin/dde-session-shell#74

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @deepin-ci-robot, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

Synchronize source files from linuxdeepin/dde-session-shell.

Source-pull-request: linuxdeepin/dde-session-shell#74
@Ivy233

Ivy233 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

/retest

@deepin-ci-robot

Copy link
Copy Markdown
Contributor Author

deepin pr auto review

★ 总体评分:60分

■ 【总体评价】

代码实现了通过宏控制配置文件路径的功能,但存在致命的宏名称不一致问题导致核心逻辑错误。
CMakeLists.txt与constants.h中使用的宏名称不匹配,导致配置文件安装路径与读取路径脱节,因核心逻辑错误扣40分。

■ 【详细分析】

  • 1.语法逻辑(存在致命错误)✕

在 CMakeLists.txt 中使用了 DDE_SESSION_SHELL_SNIPE 宏进行条件判断,而在 src/global_util/constants.h 中却使用了 ENABLE_DSS_SNIPE 宏。这两个宏名称不一致,如果构建时启用了该功能,配置文件会被安装到 /var/lib/dde-session-shell/ 目录,但程序运行时由于未定义 ENABLE_DSS_SNIPE,依然会去 /usr/share/dde-session-shell/ 目录读取配置,导致无法读取到正确的配置文件。
潜在问题:配置文件安装路径与读取路径不匹配,导致功能失效;宏定义未统一管理,易引发编译逻辑混乱。
建议:统一 CMakeLists.txt 与 C++ 源码中的宏定义名称;在 CMakeLists.txt 中通过 add_compile_definitionstarget_compile_definitions 将宏传递给 C++ 编译器。

  • 2.代码质量(一般)✕

CMakeLists.txt 中硬编码了路径 /var/lib/dde-session-shell/,未使用变量进行管理,降低了可维护性。logintipswindow.cpp 中更新了注释使其与代码逻辑更贴合,这是良好的改进。版权年份更新到 2026 年属于常规维护。
潜在问题:硬编码路径不利于后续维护和跨平台适配;宏名称笔误反映出代码审查流程存在疏漏。
建议:使用 CMake 变量替换硬编码路径;加强代码审查,确保关联文件的宏定义一致。

  • 3.代码性能(无性能问题)✓

修改仅涉及编译期条件分支和字符串常量路径的调整,不影响程序运行时的算法复杂度和资源消耗。findValueByQSettings 函数遍历列表的性能在可接受范围内,且本次未修改该逻辑。
建议:无需优化。

  • 4.代码安全(存在0个安全漏洞)✓

漏洞对比统计:新增漏洞 0 个,减少漏洞 0 个,持平 0 个
本次变更仅涉及配置文件路径的调整,未引入外部输入处理,不存在命令注入、路径遍历等安全风险。路径均为硬编码常量,攻击面无变化。

  • 建议:无需修复。

■ 【改进建议代码示例】

diff --git a/CMakeLists.txt b/CMakeLists.txt
index a47611df..c35a899b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -455,7 +455,12 @@ else ()
 endif ()
 
 # install conf files
-if (DDE_SESSION_SHELL_SNIPE)
-    install(FILES files/dde-session-shell.conf DESTINATION /var/lib/dde-session-shell/)
+if (ENABLE_DSS_SNIPE)
+    set(DDE_SESSION_SHELL_CONF_DIR "/var/lib/dde-session-shell/")
+    install(FILES files/dde-session-shell.conf DESTINATION ${DDE_SESSION_SHELL_CONF_DIR})
+    add_compile_definitions(ENABLE_DSS_SNIPE)
 else()
     install(FILES files/dde-session-shell.conf DESTINATION ${CMAKE_INSTALL_DATADIR}/dde-session-shell/)
 endif()

@deepin-ci-robot

Copy link
Copy Markdown
Contributor Author

@deepin-ci-robot: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
github-pr-review-ci 44536e9 link true /test github-pr-review-ci

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@Ivy233

Ivy233 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

DDE_SESSION_SHELL_SNIPEENABLE_DSS_SNIPE两个不同层面的变量,并非应该同名的宏:

  • DDE_SESSION_SHELL_SNIPE — CMake 构建变量
  • ENABLE_DSS_SNIPE — C++ 预处理器宏

二者的桥接在 CMakeLists.txt:35-36

if (DDE_SESSION_SHELL_SNIPE OR Qt6_FOUND)
    add_definitions(-DENABLE_DSS_SNIPE)

DDE_SESSION_SHELL_SNIPE=ON 时,ENABLE_DSS_SNIPE 会被定义为 C++ 宏;同时 CMakeLists.txt:458-459 将配置文件安装到 /var/lib/dde-session-shell/。安装路径与运行时读取路径完全一致,不存在脱节。

此外,同一段代码在 linuxdeepin/dde-session-shell#74 上也经过 bot 评审,得分 95分,结论为"基本正确",建议是"确认 CMake 中存在 add_compile_definitions"。该建议在现有代码中已满足(add_definitions(-DENABLE_DSS_SNIPE) at line 36)。

本次 60 分的扣分理由属于误判,请重新评估。

@Ivy233
Ivy233 requested review from fly602 and yixinshark July 14, 2026 03:05
@Ivy233

Ivy233 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

根据之前的合并记录,当前pr源头已经approve,因此给approve。
(其实是用这个项目做的自测代码)

@deepin-ci-robot

Copy link
Copy Markdown
Contributor Author

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deepin-ci-robot, Ivy233

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Ivy233
Ivy233 merged commit 0b16cca into master Jul 14, 2026
26 of 31 checks passed
@Ivy233
Ivy233 deleted the sync-pr-74-nosync branch July 14, 2026 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants