Skip to content

fix(display): prevent unintended brightness reset to 10%#2993

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
ComixHe:master
Feb 4, 2026
Merged

fix(display): prevent unintended brightness reset to 10%#2993
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
ComixHe:master

Conversation

@ComixHe
Copy link
Contributor

@ComixHe ComixHe commented Feb 2, 2026

  • Replace onValueChanged with onMoved for brightness slider to ensure backend values are only updated by explicit user interaction.
  • Fix a race condition where slider auto-correction (to 'from' value 0.1) would overwrite C++ data during object destruction.

Pms: BUG-308187 BUG-307119
Log: prevent unintended brightness reset to 10%

Summary by Sourcery

Bug Fixes:

  • Prevent unintended brightness resets caused by brightness slider value changes occurring without explicit user interaction.

@ComixHe ComixHe requested a review from caixr23 February 2, 2026 10:25
@sourcery-ai
Copy link

sourcery-ai bot commented Feb 2, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adjusts the display brightness slider behavior so that backend brightness is updated only on explicit user interaction, preventing unintended resets to 10% and avoiding a race condition during object destruction.

Sequence diagram for explicit brightness update via slider movement

sequenceDiagram
    actor User
    participant BrightnessSlider
    participant DisplayScreenItem
    participant BackendCPlusPlus

    User->>BrightnessSlider: drag handle
    BrightnessSlider->>BrightnessSlider: onMoved(value)
    BrightnessSlider->>DisplayScreenItem: set brightness(value)
    DisplayScreenItem->>BackendCPlusPlus: updateBrightness(value)

    rect rgb(240,240,240)
    User-->>BrightnessSlider: no interaction
    BackendCPlusPlus->>DisplayScreenItem: set brightness(programmaticValue)
    DisplayScreenItem->>BrightnessSlider: value = programmaticValue
    BrightnessSlider--xDisplayScreenItem: onMoved not triggered
    DisplayScreenItem-->>BackendCPlusPlus: brightness not overwritten
    end

    rect rgb(230,230,255)
    BackendCPlusPlus-->>DisplayScreenItem: begin destruction
    DisplayScreenItem-->>BrightnessSlider: internal value correction to 0.1
    BrightnessSlider--xBackendCPlusPlus: onMoved not called, no write-back
    BackendCPlusPlus-->>BackendCPlusPlus: stored brightness preserved
    end
Loading

Flow diagram for brightness update logic from slider

flowchart TD
    A[User moves brightness slider handle] --> B[BrightnessSlider onMoved handler]
    B --> C[Set DisplayScreenItem brightness to slider value]
    C --> D[BackendCPlusPlus updateBrightness invoked]
    D --> E[Persisted brightness updated]

    F[Programmatic change to brightness value] --> G[DisplayScreenItem sets slider value]
    G --> H[BrightnessSlider onMoved not triggered]
    H --> I[BackendCPlusPlus brightness not overwritten]

    J[Object destruction or internal slider correction to 0.1] --> K[Slider value updated internally]
    K --> L[No onMoved callback]
    L --> M[Stored brightness preserved, no reset to 10 percent]
Loading

File-Level Changes

Change Details Files
Update brightness slider to only propagate changes on user-driven movement instead of any value change, ensuring consistent backend updates and avoiding race conditions.
  • Replaced the brightness slider handler from onValueChanged to onMoved so brightness updates occur only when the user moves the slider.
  • Simplified the brightness assignment logic by directly setting screenItem.brightness to the slider value without an equality guard, relying on user interaction semantics to avoid unintended writes.
src/plugin-display/qml/DisplayMain.qml

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

这段代码主要修改了 QML 中 Slider 组件的信号处理逻辑,将 onValueChanged 替换为 onMoved。以下是对该修改的详细审查意见:

1. 语法逻辑

  • 审查结果通过
  • 分析:语法上没有错误。onMoved 是 Qt Quick Controls 中 Slider 的有效信号处理器,当滑块被用户拖动时触发。

2. 代码质量

  • 审查结果改进明显
  • 分析
    • 旧代码 (onValueChanged)value 属性的变化不仅会由用户拖动触发,还可能由后端数据绑定(例如 value: screenItem.brightness)触发。如果不加判断直接赋值,容易导致双向绑定的循环调用。虽然旧代码加了 if (screenItem.brightness !== value) 判断来防止死循环,但逻辑稍显冗余。
    • 新代码 (onMoved)onMoved 专门响应用户的交互行为。这明确区分了"用户操作"和"数据更新"两个场景,使得代码意图更加清晰,逻辑更加直接,去掉了不必要的判断分支。

3. 代码性能

  • 审查结果略有提升
  • 分析
    • onMoved 仅在用户拖动滑块位置变化时触发。
    • onValueChanged 会在任何导致 value 变化的情况下触发(包括程序内部设置)。
    • 虽然旧代码有判断保护,避免了频繁的无效写入,但新代码从根本上减少了非用户交互触发信号处理器的可能性,略微降低了事件处理的复杂度。

4. 代码安全

  • 审查结果存在潜在风险(需注意)
  • 分析
    • 功能一致性差异
      • onValueChanged 会在任何数值变化时响应,包括通过键盘(左右键)、滚轮、触控板手势或代码修改 value 时。
      • onMoved 通常只在鼠标/触摸拖动滑块手柄时触发。
    • 风险点:如果该应用支持通过键盘快捷键或滚轮调节亮度,修改为 onMoved 后,这些操作将失效(因为 onMoved 不会被触发,导致 screenItem.brightness 无法更新)。
    • 建议
      • 如果该 Slider 仅支持鼠标拖动交互,则当前修改是安全的。
      • 如果 Slider 需要支持键盘或滚轮操作,建议保留 onValueChanged,或者同时监听 onMoved 和其他相关信号(如 onPressedChanged),或者确认是否有其他机制处理非拖动式的亮度调节。

总结与建议

这次修改在逻辑清晰度代码质量上是很好的优化,避免了双向绑定的潜在复杂性。

关键建议
请务必确认该 Slider 控件是否需要支持键盘方向键鼠标滚轮调节亮度。

  • 如果不需要支持键盘/滚轮,则当前代码是完美的。
  • 如果需要支持键盘/滚轮,建议改回 onValueChanged 并配合防抖逻辑,或者使用如下混合逻辑以确保兼容性:
// 备选方案:兼容拖动和其他输入方式
onValueChanged: {
    // 只有当值确实不同时才更新,防止循环
    if (screenItem.brightness !== value) {
        screenItem.brightness = value
    }
}

或者,如果是为了解决性能问题(拖动时频繁触发),可以使用 onPressed 结合 onReleased 来优化,但这会改变实时调节的体验。通常情况下,如果旧代码能正常工作且性能可接受,保留 onValueChanged + 判断逻辑是最安全的做法。但如果确定只需要响应拖动,当前的 onMoved 是最佳选择。

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:

  • Switching from onValueChanged to onMoved means brightness updates will no longer occur for non-pointer interactions (e.g., keyboard, accessibility tools, or programmatic slider changes); consider whether those paths still need to update screenItem.brightness and if so handle them explicitly.
  • If screenItem.brightness can change externally while the slider is visible, it may now drift out of sync with the slider’s visual state; consider whether a binding or explicit synchronization is still required in those scenarios.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Switching from `onValueChanged` to `onMoved` means brightness updates will no longer occur for non-pointer interactions (e.g., keyboard, accessibility tools, or programmatic slider changes); consider whether those paths still need to update `screenItem.brightness` and if so handle them explicitly.
- If `screenItem.brightness` can change externally while the slider is visible, it may now drift out of sync with the slider’s visual state; consider whether a binding or explicit synchronization is still required in those scenarios.

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.

- Replace onValueChanged with onMoved for brightness slider to ensure
  backend values are only updated by explicit user interaction.
- Fix a race condition where slider auto-correction (to 'from' value 0.1)
  would overwrite C++ data during object destruction.

Pms: BUG-308187 BUG-307119
Log: prevent unintended brightness reset to 10%
Signed-off-by: ComixHe <heyuming@deepin.org>
@ComixHe
Copy link
Contributor Author

ComixHe commented Feb 4, 2026

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Feb 4, 2026

This pr force merged! (status: blocked)

@deepin-bot deepin-bot bot merged commit 942b94a into linuxdeepin:master Feb 4, 2026
15 of 17 checks passed
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caixr23, ComixHe

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

1 similar comment
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caixr23, ComixHe

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