Skip to content

Conversation

@18202781743
Copy link
Contributor

@18202781743 18202781743 commented Feb 11, 2026

When the inactive and active colors in QPalette are identical, the
system should not apply inactive color processing to avoid unnecessary
color calculations and potential visual inconsistencies. This change
adds a check to bypass inactive color usage when the QPalette's inactive
and active color groups are equal, ensuring more efficient rendering and
consistent visual behavior.

Log: Fixed an issue where inactive color processing was incorrectly
applied when active and inactive colors were identical, improving
rendering efficiency and visual consistency.

Influence:

  1. Test UI components with identical active and inactive QPalette colors
    to ensure they render correctly
  2. Verify that components with different active/inactive colors still
    properly apply inactive colors when appropriate
  3. Check performance impact when switching between color states
  4. Test with various control types to ensure consistent behavior across
    the application
  5. Verify that the fix doesn't break existing inactive state visual
    effects

fix: 当QPalette中非活动色和活动色相同时跳过非活动色处理

当QPalette中的非活动色和活动色相同时,系统不应应用非活动色处理,以避免
不必要的颜色计算和潜在的视觉不一致。此更改添加了一个检查,当QPalette的非
活动色和活动色组相等时,跳过非活动色的使用,确保更高效的渲染和一致的视觉
行为。

Log: 修复了当活动色和非活动色相同时错误应用非活动色处理的问题,提高了渲
染效率和视觉一致性。

Influence:

  1. 测试具有相同活动和非活动QPalette颜色的UI组件,确保它们正确渲染
  2. 验证具有不同活动/非活动颜色的组件在适当时仍能正确应用非活动色
  3. 检查在不同颜色状态之间切换时的性能影响
  4. 测试各种控件类型,确保在整个应用程序中行为一致
  5. 验证此修复不会破坏现有的非活动状态视觉效果

PMS: BUG-298841

Summary by Sourcery

Bug Fixes:

  • Prevent incorrect inactive color processing when a control’s QPalette has identical active and inactive color groups, ensuring consistent rendering and avoiding unnecessary work.

@18202781743 18202781743 requested review from BLumia and mhduiy February 11, 2026 09:33
@sourcery-ai
Copy link

sourcery-ai bot commented Feb 11, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adds a runtime check before using the inactive color group so that inactive-color processing is skipped when a control’s QPalette has identical active and inactive color groups, preventing unnecessary color transformations and potential visual inconsistencies.

Sequence diagram for inactive color selection with QPalette equality check

sequenceDiagram
    participant Control as DQuickControl
    participant ColorSelector as DQuickControlColorSelector
    participant Helper as DGuiApplicationHelper
    participant QPal as QPalette

    Control->>ColorSelector: getColorOf(palette)
    ColorSelector->>Helper: testAttribute(UseInactiveColorGroup)
    Helper-->>ColorSelector: useInactiveColor

    alt UseInactiveColorGroup is true
        ColorSelector->>Control: _d_getControlPalette(control)
        Control-->>ColorSelector: qpalette
        ColorSelector->>QPal: isEqual(Inactive, Active)
        QPal-->>ColorSelector: areEqual
        alt Inactive and Active groups are equal
            ColorSelector->>ColorSelector: useInactiveColor = false
        else Groups differ
            ColorSelector->>ColorSelector: useInactiveColor = true
        end
    else UseInactiveColorGroup is false
        ColorSelector->>ColorSelector: useInactiveColor = false
    end

    alt useInactiveColor and controlState is InactiveState
        ColorSelector->>Helper: standardPalette(controlTheme)
        Helper-->>ColorSelector: standardPalette
        ColorSelector->>Helper: paletteColor(Window)
        Helper-->>ColorSelector: inactiveWindowColor
        ColorSelector-->>Control: return inactiveWindowColor
    else
        ColorSelector-->>Control: return activeColorOrOther
    end
Loading

Class diagram for updated color selection logic in DQuickControlColorSelector

classDiagram
    class DQuickControlColorSelector {
        - DQuickControl m_control
        QColor getColorOf(DQuickControlPalette palette)
    }

    class DQuickControlPalette {
        // Palette data for quick controls
    }

    class DGuiApplicationHelper {
        + static bool testAttribute(int attribute)
        + static QPalette standardPalette(int controlTheme)
        <<static>> UseInactiveColorGroup
    }

    class QPalette {
        + bool isEqual(QPalette::ColorGroup group1, QPalette::ColorGroup group2)
        + QColor color(QPalette::ColorRole role)
        <<enum>> ColorGroup
        <<enum>> ColorRole
        <<enum>> Inactive
        <<enum>> Active
        <<enum>> Window
    }

    class DQMLGlobalObject {
        <<enum>> InactiveState
    }

    DQuickControlColorSelector --> DQuickControlPalette : uses
    DQuickControlColorSelector --> DGuiApplicationHelper : calls
    DQuickControlColorSelector --> QPalette : compares groups
    DQuickControlColorSelector --> DQMLGlobalObject : reads controlState
    DGuiApplicationHelper --> QPalette : returns standardPalette
Loading

Flow diagram for deciding whether to use inactive color group

flowchart TD
    A[Start getColorOf] --> B[Call testAttribute UseInactiveColorGroup]
    B --> C{Attribute enabled?}
    C -- No --> D[useInactiveColor = false]
    D --> G[Check controlState and return color]
    C -- Yes --> E{m_control exists?}
    E -- No --> G
    E -- Yes --> F[Get qpalette from _d_getControlPalette]
    F --> H[Call qpalette.isEqual Inactive Active]
    H --> I{Inactive equals Active?}
    I -- Yes --> J[useInactiveColor = false]
    I -- No --> K[useInactiveColor = true]
    J --> G
    K --> G
    G --> L{useInactiveColor and controlState is InactiveState?}
    L -- Yes --> M[Use standardPalette Inactive colors]
    L -- No --> N[Use active or default color]
    M --> O[Return selected color]
    N --> O[Return selected color]
    O --> P[End]
Loading

File-Level Changes

Change Details Files
Gate inactive color usage on QPalette active/inactive group equality instead of a simple global attribute check.
  • Change useInactiveColor from a static cached value to a per-call value so it can depend on the current control’s palette.
  • When the global UseInactiveColorGroup attribute is enabled and a control instance is available, retrieve the control’s QPalette via _d_getControlPalette(m_control).
  • Compare the palette’s QPalette::Inactive and QPalette::Active groups, and disable inactive-color usage when they are equal.
  • Keep using the inactive color group only when the global attribute is set, the control has a palette, and the inactive and active groups differ, preserving existing behavior for distinct palettes.
src/private/dquickcontrolpalette.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
Contributor

deepin pr auto review

这段代码主要是在获取控件颜色时,增加了对 UseInactiveColorGroup 属性的额外检查逻辑。下面是对这段 diff 的详细审查意见,包括语法逻辑、代码质量、代码性能和代码安全四个方面:

1. 语法逻辑

  • 逻辑修正:原代码中 useInactiveColorstatic const,意味着它在整个程序生命周期中只计算一次。修改后去掉了 static,变成了局部变量,并且增加了动态检查逻辑。这是合理的,因为 m_control 的状态(如其 QPalette)可能在运行时发生变化,不能仅在初始化时决定。
  • 逻辑正确性:新增的逻辑是:如果全局启用了 UseInactiveColorGroup,且当前控件存在(m_control 不为空),则进一步检查该控件的 QPaletteInactiveActive 组的颜色是否一致。
    • 如果 isEqual(Inactive, Active) 返回 true(即非活动状态颜色和活动状态颜色一样),则 useInactiveColor 被设为 false
    • 这意味着如果控件自身定义的非活动颜色和活动颜色没有区别,就没有必要强制去查找非活动色,从而避免了不必要的颜色查找或覆盖。这个逻辑是自洽的。

2. 代码质量

  • 变量声明:将 static const 改为普通变量是必要的,但建议使用更明确的类型,例如 bool,而不是依赖 auto 推导,以提高可读性:
    bool useInactiveColor = DGuiApplicationHelper::testAttribute(DGuiApplicationHelper::UseInactiveColorGroup);
  • 嵌套层级:新增的 if (useInactiveColor) 嵌套是可以接受的,因为如果全局属性未开启,后续检查确实不需要执行。这属于"卫语句"的一种变体,逻辑清晰。
  • 代码注释:建议在新增的逻辑块前添加简短注释,解释为什么需要检查 QPalette 的相等性,帮助维护者理解这是为了处理控件自定义调色板的情况。

3. 代码性能

  • 性能开销
    • 去除 static:去除了 static 后,每次调用 getColorOf 都会调用 testAttribute。这比原来稍慢,但考虑到 testAttribute 通常是简单的位运算或布尔值读取,性能损耗极小,几乎可以忽略不计。
    • 新增检查:新增的 _d_getControlPaletteisEqual 调用仅在 useInactiveColor 为真且 m_control 存在时发生。这引入了额外的函数调用和对象拷贝(qpalette)开销。但在 GUI 渲染流程中,这种逻辑判断的开销通常远小于绘制开销,因此是可接受的。
  • 潜在优化:如果 _d_getControlPalette(m_control) 调用开销较大(例如涉及深拷贝或复杂计算),且 getColorOf 调用频率极高(如每帧数千次),可能需要考虑缓存 QPalette 的状态或仅在控件调色板变更时更新标志位。但从代码上下文看,作为属性读取,当前实现是合理的。

4. 代码安全

  • 空指针检查:代码中显式检查了 if (m_control),这是非常好的习惯,防止了潜在的空指针解引用风险。
  • 函数调用安全性:假设 _d_getControlPaletteQPalette::isEqual 是 Qt 框架或项目内部稳定的 API,不存在安全性问题。
  • 类型安全auto 的使用是安全的,因为右侧返回类型明确。

改进建议

综合上述分析,代码逻辑正确且安全。为了进一步提升代码质量,建议做如下微调:

  1. 显式类型声明
    auto useInactiveColor 改为 bool useInactiveColor,增强代码可读性。

  2. 添加注释
    补充注释说明为何在控件存在时检查 Palette 的相等性。

改进后的代码示例:

    // items with inactive state should use inactive color, it likes dtkgui's generatePaletteColor_helper.
    // 检查是否使用非激活颜色。如果控件存在,还需检查控件自身的非激活与激活调色板是否一致,
    // 若一致则无需特殊处理非激活状态。
    bool useInactiveColor = DGuiApplicationHelper::testAttribute(DGuiApplicationHelper::UseInactiveColorGroup);
    
    if (useInactiveColor) {
        if (m_control) {
            const auto qpalette = _d_getControlPalette(m_control);
            // 如果控件的非激活色和激活色定义相同,则不应用非激活色逻辑
            useInactiveColor = !qpalette.isEqual(QPalette::Inactive, QPalette::Active);
        }
    }
    
    if (useInactiveColor && state->controlState == DQMLGlobalObject::InactiveState) {
        const auto &palette = DGuiApplicationHelper::standardPalette(state->controlTheme);
        const auto &windowColor = palette.color(QPalette::Window);
        // ... 后续代码

总结
这段 diff 修复了在全局启用 UseInactiveColorGroup 时,未考虑控件自身 QPalette 配置(如 Inactive 和 Active 颜色相同)的潜在逻辑缺陷。改动是必要的,且实现方式安全、合理。

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 - I've left some high level feedback:

  • The previous static const auto useInactiveColor was evaluated once, but now testAttribute and _d_getControlPalette run on every call to getColorOf; consider caching these results (e.g., per-control or via a small helper) if this path is performance-sensitive.
  • The new isEqual(QPalette::Inactive, QPalette::Active) check compares full palette groups, which may be heavier than necessary; if only specific roles drive the inactive-color behavior here, comparing just those roles could simplify the logic and reduce overhead.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The previous `static const auto useInactiveColor` was evaluated once, but now `testAttribute` and `_d_getControlPalette` run on every call to `getColorOf`; consider caching these results (e.g., per-control or via a small helper) if this path is performance-sensitive.
- The new `isEqual(QPalette::Inactive, QPalette::Active)` check compares full palette groups, which may be heavier than necessary; if only specific roles drive the inactive-color behavior here, comparing just those roles could simplify the logic and reduce overhead.

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.

When the inactive and active colors in QPalette are identical, the
system should not apply inactive color processing to avoid unnecessary
color calculations and potential visual inconsistencies. This change
adds a check to bypass inactive color usage when the QPalette's inactive
and active color groups are equal, ensuring more efficient rendering and
consistent visual behavior.

Log: Fixed an issue where inactive color processing was incorrectly
applied when active and inactive colors were identical, improving
rendering efficiency and visual consistency.

Influence:
1. Test UI components with identical active and inactive QPalette colors
to ensure they render correctly
2. Verify that components with different active/inactive colors still
properly apply inactive colors when appropriate
3. Check performance impact when switching between color states
4. Test with various control types to ensure consistent behavior across
the application
5. Verify that the fix doesn't break existing inactive state visual
effects

fix: 当QPalette中非活动色和活动色相同时跳过非活动色处理

当QPalette中的非活动色和活动色相同时,系统不应应用非活动色处理,以避免
不必要的颜色计算和潜在的视觉不一致。此更改添加了一个检查,当QPalette的非
活动色和活动色组相等时,跳过非活动色的使用,确保更高效的渲染和一致的视觉
行为。

Log: 修复了当活动色和非活动色相同时错误应用非活动色处理的问题,提高了渲
染效率和视觉一致性。

Influence:
1. 测试具有相同活动和非活动QPalette颜色的UI组件,确保它们正确渲染
2. 验证具有不同活动/非活动颜色的组件在适当时仍能正确应用非活动色
3. 检查在不同颜色状态之间切换时的性能影响
4. 测试各种控件类型,确保在整个应用程序中行为一致
5. 验证此修复不会破坏现有的非活动状态视觉效果

PMS: BUG-298841
@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, BLumia

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

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