Skip to content

Conversation

@mhduiy
Copy link
Contributor

@mhduiy mhduiy commented Jan 30, 2026

Changed the scrollbar visibility initialization logic in ChameleonStyle to respect system theme's scrollbar policy. Previously, scrollbars were always set to visible by default, then hidden via animation. Now, when the system theme specifies Qt::ScrollBarAsNeeded policy, scrollbars start as invisible; otherwise, they remain visible by default. This ensures proper scrollbar behavior alignment with user's system preferences.

fix: 根据系统主题策略调整滚动条可见性

修改了ChameleonStyle中滚动条可见性的初始化逻辑,以遵循系统主题的滚动条
策略。之前滚动条始终默认设置为可见,然后通过动画隐藏。现在,当系统主题指
定Qt::ScrollBarAsNeeded策略时,滚动条初始为不可见;其他情况下保持默认可
见。这确保了滚动条行为与用户系统偏好设置的一致性。

PMS: BUG-339051

@mhduiy mhduiy force-pushed the scrollbar branch 3 times, most recently from b387b71 to 729f99b Compare January 30, 2026 09:11
Changed the scrollbar visibility initialization logic in ChameleonStyle
to respect system theme's scrollbar policy. Previously, scrollbars were
always set to visible by default, then hidden via animation. Now, when
the system theme specifies Qt::ScrollBarAsNeeded policy, scrollbars
start as invisible; otherwise, they remain visible by default.
This ensures proper scrollbar behavior alignment with user's system
preferences.

fix: 根据系统主题策略调整滚动条可见性

修改了ChameleonStyle中滚动条可见性的初始化逻辑,以遵循系统主题的滚动条
策略。之前滚动条始终默认设置为可见,然后通过动画隐藏。现在,当系统主题指
定Qt::ScrollBarAsNeeded策略时,滚动条初始为不可见;其他情况下保持默认可
见。这确保了滚动条行为与用户系统偏好设置的一致性。

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

deepin pr auto review

这段代码主要涉及 Qt 框架下滚动条的动画控制逻辑,通过 setProperty 修改动态属性来标记状态。以下是对该 git diff 的详细审查意见:

1. 语法逻辑审查

  • 变量命名与语义一致性

    • 变更点:将属性名从 _d_dtk_scrollbar_visible 修改为 _d_dtk_scrollbar_hide_ani
    • 分析:原变量名 _d_dtk_scrollbar_visible 字面意思是“滚动条可见”,但在代码逻辑中,它实际被用作一个“触发器”或“锁”,用于标记“当鼠标移开时是否需要启动隐藏动画”。原变量名具有误导性,容易让人误以为它直接控制滚动条的 Visible 状态。
    • 结论:新变量名 _d_dtk_scrollbar_hide_ani(hide animation)更准确地描述了其作为“动画触发标志”的语义。这是一个积极的改进,提高了代码的可读性。
  • 逻辑流程

    • 第一处变更
      • 逻辑:当鼠标悬停或按下且动画正在运行时,设置标志位为 true
      • 解读:这意味着“当用户正在与滚动条交互时,如果动画还在跑,请记住:等用户移开鼠标后,我们需要重新运行隐藏动画”。逻辑是自洽的。
    • 第二处变更
      • 逻辑:resetAttribute 函数中,将初始状态设为 false
      • 解读:默认情况下不需要执行隐藏动画(通常意味着默认不自动隐藏,或者等待第一次交互后的状态变化再决定)。这与 hideScrollBarByAnimation 中的逻辑配合得当。

2. 代码质量审查

  • 魔法字符串

    • 代码中直接使用了字符串字面量 "_d_dtk_scrollbar_hide_ani"。如果这个属性在多个文件中被引用,维护起来会比较困难(例如重构改名时容易遗漏)。
    • 建议:建议在类的头文件中定义一个静态常量字符串,例如:
      // 在 .h 文件中
      static const char *const kPropertyScrollbarHideAni = "_d_dtk_scrollbar_hide_ani";
      
      // 在 .cpp 文件中使用
      sbar->setProperty(kPropertyScrollbarHideAni, true);
      这样可以避免拼写错误,并方便未来统一修改。
  • 注释清晰度

    • // 默认情况下不执行隐藏动画 这条注释非常清晰,准确解释了初始化逻辑,值得肯定。

3. 代码性能审查

  • 属性查找开销
    • setPropertyproperty 操作涉及字符串哈希查找和 QVariant 的构造/转换。在 UI 重绘或鼠标移动的高频事件中频繁调用可能会有微小的性能开销。
    • 现状:考虑到这是在 hideScrollBarByAnimation(通常由定时器触发)和 resetAttribute(初始化或状态重置时调用)中使用,频率相对可控,目前的实现方式在 Qt 风格编程中是标准做法,性能方面无需过度优化

4. 代码安全审查

  • 内存与资源安全
    • 这里的操作仅涉及设置 QWidget 的动态属性,不涉及指针操作或内存分配,因此没有内存泄漏或空指针解引用的风险。
    • Qt 的 setProperty 机制是类型安全的(通过 QVariant 转换),toBool() 也能安全处理非布尔值,不会导致崩溃。

总结与改进建议

总体来说,这次修改是一次高质量的代码重构,主要是修正了变量命名的语义问题,使得代码逻辑更容易理解。

改进建议表:

维度 评分/意见 具体建议
语法逻辑 优秀 变量名修改准确反映了业务逻辑,消除了歧义。
代码质量 良好 建议消除魔法字符串,使用静态常量定义属性名。
代码性能 可接受 当前调用频率下,Qt 属性系统的性能开销可忽略。
代码安全 安全 无资源泄漏风险,类型转换安全。

示例优化代码(针对代码质量建议):

// ChameleonStyle.h (建议添加)
class ChameleonStyle : public ... {
    // ...
private:
    static const char* const kPropScrollbarHideAni; 
    // ...
};

// ChameleonStyle.cpp (建议添加)
const char* const ChameleonStyle::kPropScrollbarHideAni = "_d_dtk_scrollbar_hide_ani";

// 在函数中使用
bool ChameleonStyle::hideScrollBarByAnimation(const QStyleOptionSlider *scrollBar, ...) {
    // ...
    // 使用常量替代字符串字面量
    sbar->setProperty(kPropScrollbarHideAni, true); 
    // ...
}

@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@mhduiy mhduiy merged commit ba9171b into linuxdeepin:master Jan 30, 2026
18 of 21 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