Skip to content

Conversation

@add-uos
Copy link
Contributor

@add-uos add-uos commented Dec 3, 2025

Add DISABLE_DRIVER macro definition when building with Qt6 to ensure compatibility. The macro conditionally hides driver management buttons in the main window title bar, as driver functionality is not supported in Qt6 builds while maintaining full functionality for Qt5 builds.

Summary by Sourcery

Conditionally disable driver-related UI elements in Qt6 builds while preserving existing behavior for Qt5 builds.

Enhancements:

  • Guard the title bar driver management button visibility with the DISABLE_DRIVER macro so it can be hidden only when driver features are disabled.

Build:

  • Define the DISABLE_DRIVER macro automatically for Qt6 builds via CMake configuration.

Add DISABLE_DRIVER macro definition when building with Qt6 to ensure
compatibility. The macro conditionally hides driver management buttons
in the main window title bar, as driver functionality is not supported
in Qt6 builds while maintaining full functionality for Qt5 builds.
@sourcery-ai
Copy link

sourcery-ai bot commented Dec 3, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Conditionally defines a DISABLE_DRIVER macro for Qt6 builds and updates the main window title bar logic to hide the driver-related button box only when appropriate (Qt6 or specific board vendor), instead of always hiding it.

File-Level Changes

Change Details Files
Add a Qt-version-based DISABLE_DRIVER macro to the build configuration.
  • Introduce a CMake check on QT_VERSION_MAJOR to distinguish Qt5 vs Qt6 builds.
  • Define the DISABLE_DRIVER macro only when building with Qt6.
  • Emit status messages at configure time indicating whether DISABLE_DRIVER is enabled or not.
deepin-devicemanager/CMakeLists.txt
Adjust title bar button box visibility logic to be conditional rather than always hidden.
  • Keep the existing condition that hides the button box when boardVendorType is non-empty.
  • Add a preprocessor guard to hide the button box when DISABLE_DRIVER is defined (Qt6 builds).
  • Remove the previous unconditional hide that removed hardware info and driver management buttons from the title bar in all builds.
deepin-devicemanager/src/Page/MainWindow.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot
Copy link

deepin pr auto review

我来对这段代码变更进行审查:

  1. 语法逻辑:
  • 代码语法正确,没有语法错误
  • 条件判断逻辑清晰,使用 Qt 版本判断来控制 DISABLE_DRIVER 宏的定义
  • 在 MainWindow.cpp 中对 DISABLE_DRIVER 宏的使用也是正确的
  1. 代码质量:
  • 优点:

    • 通过宏定义来控制功能模块的显示是一个好的实践
    • 添加了清晰的状态消息输出,便于调试
    • 移除了重复的 mp_ButtonBox->hide() 调用,使代码更简洁
  • 可改进的地方:

    • 可以考虑在 CMakeLists.txt 中添加注释说明为什么 Qt6 需要禁用驱动功能
    • 可以考虑将 DISABLE_DRIVER 的定义集中到一个配置文件中,便于管理
  1. 代码性能:
  • 这个变更对性能影响很小,主要是编译时的条件判断
  • 运行时的 hide() 调用是必要的,没有性能问题
  1. 代码安全:
  • 使用编译时宏定义来控制功能是安全的做法
  • 没有发现明显的安全隐患

建议:

  1. 在 CMakeLists.txt 中添加注释说明 Qt6 禁用驱动功能的原因
  2. 考虑将 DISABLE_DRIVER 相关的配置统一管理
  3. 可以在编译时输出更详细的配置信息,便于问题追踪

总体来说,这是一个合理的代码变更,通过条件编译来适配不同 Qt 版本的需求,代码结构清晰,逻辑正确。

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes - here's some feedback:

  • The CMake condition if(${QT_VERSION_MAJOR} EQUAL 6) would be more robust written without dereferencing in the if (e.g. if(QT_VERSION_MAJOR EQUAL 6) or with quotes) to avoid issues when the variable is unset or contains unexpected content.
  • Using add_definitions(-DDISABLE_DRIVER) applies the macro project‑wide; consider switching to target_compile_definitions on the specific targets that need this behavior to avoid unintended side effects on other targets.
  • The changes in initWindowTitle() now show mp_ButtonBox on Qt5 when boardVendorType() is empty, whereas it was always hidden before; double‑check that this UI behavior change (hardware/driver buttons becoming visible in that case) is intended, or separate the driver controls from other title bar elements if only some parts should be gated by DISABLE_DRIVER.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The CMake condition `if(${QT_VERSION_MAJOR} EQUAL 6)` would be more robust written without dereferencing in the `if` (e.g. `if(QT_VERSION_MAJOR EQUAL 6)` or with quotes) to avoid issues when the variable is unset or contains unexpected content.
- Using `add_definitions(-DDISABLE_DRIVER)` applies the macro project‑wide; consider switching to `target_compile_definitions` on the specific targets that need this behavior to avoid unintended side effects on other targets.
- The changes in `initWindowTitle()` now show `mp_ButtonBox` on Qt5 when `boardVendorType()` is empty, whereas it was always hidden before; double‑check that this UI behavior change (hardware/driver buttons becoming visible in that case) is intended, or separate the driver controls from other title bar elements if only some parts should be gated by `DISABLE_DRIVER`.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: add-uos, lzwind

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

@add-uos
Copy link
Contributor Author

add-uos commented Dec 3, 2025

/merge

@deepin-bot deepin-bot bot merged commit 1bd479a into linuxdeepin:master Dec 3, 2025
18 checks passed
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.

3 participants