Skip to content

fix: stabilize AlertToolTip overlay parenting - #668

Open
52cyb wants to merge 1 commit into
linuxdeepin:masterfrom
52cyb:agent/bot/cc6976f9
Open

52cyb wants to merge 1 commit into
linuxdeepin:masterfrom
52cyb:agent/bot/cc6976f9

Conversation

@52cyb

@52cyb 52cyb commented Aug 20, 2026 •

Copy link
Copy Markdown
Contributor
  1. Add shown property and derive visible state from it
  2. Resolve the overlay through target.Overlay.overlay to avoid
    parent-dependent lookup
  3. Bind parent to the overlay while shown and restore target
    when hidden
  4. Update EditPanel to control shown instead of visible

Influence:

  1. Verify alert tooltip appears above edit controls
  2. Confirm show/hide cycles emit no binding loop warnings
  3. Verify timeout expiration hides the tooltip correctly
  4. Confirm tooltip returns to target when hidden

fix: 稳定 AlertToolTip 的 Overlay 父级切换

  1. 新增 shown 属性,并由其派生 visible 状态
  2. 通过 target.Overlay.overlay 获取 Overlay,避免依赖当前父级查询
  3. 显示时将父级绑定到 Overlay,隐藏时恢复为 target
  4. 更新 EditPanel,使用 shown 代替 visible 控制提示框

Influence:

  1. 验证告警提示框在编辑控件上方正确显示
  2. 确认显示/隐藏循环不再产生 binding loop 警告
  3. 验证超时后提示框能够正确隐藏
  4. 确认隐藏时 tooltip 父级正确回到 target

PMS: TASK-392413

@deepin-ci-robot

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@sourcery-ai

sourcery-ai Bot commented Aug 20, 2026 •

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

AlertToolTip now updates its parent imperatively at lifecycle and state-change points instead of using a self-referential binding, preventing QML binding-loop warnings during alert show/hide transitions.

Sequence diagram for imperative AlertToolTip parent updates

sequenceDiagram
    participant AlertToolTip
    participant QMLLifecycle
    participant Overlay
    participant Target

    QMLLifecycle->>AlertToolTip: Component.onCompleted()
    AlertToolTip->>AlertToolTip: _updateParent()
    alt _shown
        AlertToolTip->>Overlay: parent = Overlay.overlay
    else hidden
        AlertToolTip->>Target: parent = target
    end
    QMLLifecycle->>AlertToolTip: on_ShownChanged()
    AlertToolTip->>AlertToolTip: _updateParent()
    QMLLifecycle->>AlertToolTip: onTargetChanged()
    AlertToolTip->>AlertToolTip: _updateParent()
Loading

File-Level Changes

Change Details Files
Replace the declarative parent binding with imperative updates to eliminate the AlertToolTip binding loop while preserving its overlay and target parenting behavior.
  • Add an _updateParent() helper that assigns Overlay.overlay when shown and target otherwise.
  • Invoke the helper on component completion, visibility changes, and target changes.
  • Retain existing z-order and drag-related parenting semantics.
qt6/src/qml/AlertToolTip.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

@52cyb
52cyb force-pushed the agent/bot/cc6976f9 branch from f63645c to 4928f02 Compare August 20, 2026 12:41
@52cyb
52cyb marked this pull request as ready for review August 20, 2026 12:48

@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.

Hey - I've found 1 issue, and left some high level feedback:

  • If parent should also react to changes in target (not only _shown), consider invoking _updateParent() from the onTargetChanged handler as well to keep the parent consistent.
  • Double-check that on_ShownChanged matches the actual property name (_shown vs shown) and QML signal naming conventions (e.g. onShownChanged), to ensure the handler is actually triggered.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- If `parent` should also react to changes in `target` (not only `_shown`), consider invoking `_updateParent()` from the `onTargetChanged` handler as well to keep the parent consistent.
- Double-check that `on_ShownChanged` matches the actual property name (`_shown` vs `shown`) and QML signal naming conventions (e.g. `onShownChanged`), to ensure the handler is actually triggered.

## Individual Comments

### Comment 1
<location path="qt6/src/qml/AlertToolTip.qml" line_range="33-37" />
<code_context>
         NumberAnimation { duration: 200 }
     }
-    parent: _shown ? Overlay.overlay : target
+    function _updateParent() {
+        parent = _shown ? Overlay.overlay : target
+    }
+    Component.onCompleted: _updateParent()
+    on_ShownChanged: _updateParent()
     opacity: _shown ? 1 : 0
     enabled: _shown
</code_context>
<issue_to_address>
**issue (bug_risk):** Parent is no longer reactive to `target` or `Overlay.overlay` changes, unlike the original binding.

By turning `parent: _shown ? Overlay.overlay : target` into `_updateParent()` that only runs on `Component.onCompleted` and `_shown` changes, `parent` no longer reacts to `target` or `Overlay.overlay` updates. If either can change at runtime, the item will end up parented incorrectly. Please ensure `_updateParent()` is also triggered when `target` or `Overlay.overlay` change, or reintroduce a binding-like mechanism (e.g. `Binding` or additional change handlers).
</issue_to_address>

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.

Comment thread qt6/src/qml/AlertToolTip.qml Outdated
@52cyb
52cyb force-pushed the agent/bot/cc6976f9 branch from 4928f02 to e9244f2 Compare August 20, 2026 15:38
@52cyb
52cyb marked this pull request as draft August 20, 2026 15:38
@52cyb
52cyb force-pushed the agent/bot/cc6976f9 branch from e9244f2 to 0942490 Compare August 21, 2026 01:41
@52cyb
52cyb force-pushed the agent/bot/cc6976f9 branch 2 times, most recently from d3a7812 to 840f29e Compare September 4, 2026 02:04
@deepin-bot

deepin-bot Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

TAG Bot

TAG: 6.7.50
EXISTED: no
DISTRIBUTION: unstable

@52cyb
52cyb marked this pull request as ready for review September 10, 2026 13:24
@52cyb
52cyb force-pushed the agent/bot/cc6976f9 branch from 840f29e to 935ab37 Compare September 10, 2026 13:24

@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.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

@52cyb
52cyb force-pushed the agent/bot/cc6976f9 branch from 935ab37 to e1f40dd Compare September 21, 2026 07:16
Behavior on y {
NumberAnimation { duration: 200 }
}
parent: _shown ? Overlay.overlay : target

@18202781743 18202781743 Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

还是没看懂从哪里引入的循环绑定,

qt6/src/qml/AlertToolTip.qml 第 33 行声明式绑定 parent: _shown ? Overlay.overlay : target 形成自引用依赖环:
parent → Overlay.overlay → window → parent

每次 alert 显示/隐藏时反复重父,触发 Qt binding loop 检测。

@52cyb
52cyb force-pushed the agent/bot/cc6976f9 branch 4 times, most recently from 656c946 to c0d4c5c Compare September 24, 2026 09:53
@52cyb 52cyb changed the title fix: resolve AlertToolTip parent binding loop fix: stabilize AlertToolTip overlay parenting Sep 24, 2026
Comment thread qt6/src/qml/AlertToolTip.qml Outdated
@52cyb
52cyb force-pushed the agent/bot/cc6976f9 branch from c0d4c5c to c2cbc2b Compare September 24, 2026 10:15
@deepin-ci-robot

Copy link
Copy Markdown
Contributor

deepin pr auto review

🤖 AI 代码审查报告

总体评分: 99 分 (通过阈值: 70分)

Pass


📊 总体评价

项目 结果
审查结论 代码审查通过
评分详情 总体评分 99 分,大于 70 分通过阈值,代码质量符合要求。本次变更修复了 AlertToolTip 的 Overlay 父级切换问题,通过引入 requestVisible 属性分离可见性请求与实际可见状态,有效消除了 binding loop 警告。

🔍 详细分析

1. 语法逻辑 ✅

评价: 优秀 ✅ 通过

潜在问题:
✅ 未发现明显问题

建议: 语法正确,逻辑清晰,binding loop 修复方案合理


2. 代码质量 ✅

评价: 优秀 ✅ 通过

潜在问题:

  1. qt6/src/qml/AlertToolTip.qml:15 - requestVisible 属性缺少简要内联注释,建议说明其与 visible 分离以避免 binding loop 的设计原因

建议: 建议在 requestVisible 属性上方添加简要注释说明分离原因,便于后续维护者理解设计意图


3. 代码性能 ✅

评价: 优秀 ✅ 通过

潜在问题:
✅ 未发现明显问题

建议: 性能良好,资源使用合理,_overlay 作为只读属性避免重复计算


4. 代码安全 🔒

评价: 优秀 ✅ 通过

🔐 发现 0 个安全漏洞

安全漏洞详情:
✅ 未发现安全漏洞

建议: 存在0个安全漏洞,无安全风险,null 检查处理完善


💡 改进建议代码示例

// AlertToolTip.qml - 建议添加注释说明设计意图

Control {
    id: control
    property Item target
    property string text
    property int timeout: 0

    // Use requestVisible instead of visible to break the binding loop:
    // visible depends on _shown (via opacity/parent), and _shown
    // previously depended on visible. Now _shown depends on
    // requestVisible, breaking the cycle.
    property bool requestVisible: false
    property bool _expired: false
    readonly property bool _shown: requestVisible && !_expired
    readonly property Item _overlay: target ? target.Overlay.overlay : null
    visible: requestVisible
}

本报告由 AI 代码审查工具自动生成

1. Add shown property and derive visible state from it
2. Resolve the overlay through target.Overlay.overlay to avoid
 parent-dependent lookup
3. Bind parent to the overlay while shown and restore target
 when hidden
4. Update EditPanel to control shown instead of visible

Influence:
1. Verify alert tooltip appears above edit controls
2. Confirm show/hide cycles emit no binding loop warnings
3. Verify timeout expiration hides the tooltip correctly
4. Confirm tooltip returns to target when hidden

fix: 稳定 AlertToolTip 的 Overlay 父级切换

1. 新增 shown 属性,并由其派生 visible 状态
2. 通过 target.Overlay.overlay 获取 Overlay,避免依赖当前父级查询
3. 显示时将父级绑定到 Overlay,隐藏时恢复为 target
4. 更新 EditPanel,使用 shown 代替 visible 控制提示框

Influence:
1. 验证告警提示框在编辑控件上方正确显示
2. 确认显示/隐藏循环不再产生 binding loop 警告
3. 验证超时后提示框能够正确隐藏
4. 确认隐藏时 tooltip 父级正确回到 target

PMS: TASK-392413
@52cyb
52cyb force-pushed the agent/bot/cc6976f9 branch from c2cbc2b to f96893a Compare September 28, 2026 01:39
@deepin-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, 52cyb

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

This branch has not been deployed

No deployments
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