Skip to content

fix: Resolved user confirmation dialog hierarchy anomaly#586

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
JWWTSL:master
Mar 12, 2026
Merged

fix: Resolved user confirmation dialog hierarchy anomaly#586
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
JWWTSL:master

Conversation

@JWWTSL
Copy link
Contributor

@JWWTSL JWWTSL commented Mar 12, 2026

log: Fixed integration and rollback handling

pms: bug-352609

Summary by Sourcery

Bug Fixes:

  • Fix incorrect parent-child hierarchy and activation behavior for window-modal user confirmation dialogs by relying on default transient parent handling.

log: Fixed integration and rollback handling

pms: bug-352609
@sourcery-ai
Copy link

sourcery-ai bot commented Mar 12, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Simplifies the dialog window behavior by removing custom transient parent handling and visibility-based activation logic, relying instead on default window hierarchy and activation behavior.

Sequence diagram for simplified dialog window activation behavior

sequenceDiagram
    actor User
    participant Application
    participant DialogWindow
    participant QtWindowManager

    User->>Application: triggerAction()
    Application->>DialogWindow: create and show()
    DialogWindow->>QtWindowManager: requestShowWindow()
    QtWindowManager-->>DialogWindow: applyDefaultTransientParent()
    QtWindowManager-->>DialogWindow: manageZOrderAndActivation()
    DialogWindow-->>User: dialogVisibleAndInteractive
Loading

Updated class diagram for DialogWindow transient parent removal

classDiagram
    class DialogWindow {
        +real topPadding
        +real bottomPadding
        +real leftPadding
        +real rightPadding
        +onClosing(close)
    }

    class DialogWindowLegacy {
        +var transientParentWindow
        +updateTransientParent()
        +Component_onCompleted()
        +onVisibleChanged()
    }

    DialogWindowLegacy <|-- DialogWindow
Loading

File-Level Changes

Change Details Files
Removed custom transient parent tracking and activation logic from the dialog window implementation.
  • Deleted transientParentWindow property and its binding to the Window.transientParent property.
  • Removed the updateTransientParent() helper that tried to infer a transient parent when the dialog was window-modal.
  • Eliminated lifecycle hooks (Component.onCompleted and onVisibleChanged) that updated the transient parent and explicitly raised/activated the window.
qt6/src/qml/DialogWindow.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

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:

  • By removing the onVisibleChanged handler (including raise() and requestActivate()), dialogs may now open without keyboard focus or appear behind other windows; if those behaviors are still required in some scenarios, consider moving the focus/stacking logic to the caller or reintroducing it in a more targeted way.
  • The removal of the transientParentWindow handling means modal dialogs will no longer automatically attach to the current Qt.application.activeWindow; verify that all dialog creation sites now explicitly set transientParent where needed, or consider providing a clearer, opt‑in helper for this behavior instead of relying on implicit application state.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- By removing the `onVisibleChanged` handler (including `raise()` and `requestActivate()`), dialogs may now open without keyboard focus or appear behind other windows; if those behaviors are still required in some scenarios, consider moving the focus/stacking logic to the caller or reintroducing it in a more targeted way.
- The removal of the `transientParentWindow` handling means modal dialogs will no longer automatically attach to the current `Qt.application.activeWindow`; verify that all dialog creation sites now explicitly set `transientParent` where needed, or consider providing a clearer, opt‑in helper for this behavior instead of relying on implicit application state.

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
Contributor

deepin pr auto review

这段代码是一个对 Qt/QML 文件 DialogWindow.qml 的修改,主要移除了与 transientParent(父窗口)相关的属性和逻辑。以下是对这段 diff 的审查意见,涵盖语法逻辑、代码质量、性能和安全四个方面:

1. 语法逻辑

  • 移除逻辑的完整性
    • 问题:代码移除了 transientParentWindow 属性、transientParent 绑定以及 updateTransientParent 函数。这意味着该窗口将不再尝试自动设置其父窗口。
    • 风险:如果该组件原本用于模态对话框(modality: Qt.WindowModal),移除 transientParent 的设置会导致对话框失去与父窗口的关联。在大多数窗口管理器中,模态对话框如果没有设置 transientParent,将表现为全局模态(阻塞整个应用)或者无法正确保持在父窗口之上,且在任务栏中可能表现为独立窗口。
    • 建议:请确认移除此逻辑是有意为之,且该组件的使用场景不再需要模态对话框的层级管理。如果仍需模态行为,必须保留 transientParent 的设置逻辑。

2. 代码质量

  • 代码简洁性
    • 优点:移除了 updateTransientParent 及其调用逻辑,简化了 onCompletedonVisibleChanged 事件处理器。这使得代码更易于阅读,减少了组件内部的副作用。
    • 缺点:如果原本是为了解决某些特定窗口管理器(如 Wayland)下的窗口层级问题,移除这些代码可能会导致回归问题。
  • 硬编码与魔法值
    • 原代码中使用了 Qt.application.activeWindow 作为候选父窗口。这种逻辑虽然灵活,但有时不够精确。移除它后,意味着调用者必须显式地管理窗口关系。这实际上提高了代码的明确性,属于一种改进。

3. 代码性能

  • 运行时开销
    • 改进:原代码在 onVisibleChanged 中使用了 Qt.callLater 来调用 raise()requestActivate()。这是一个异步操作,虽然开销不大,但在高频显示/隐藏的场景下会产生不必要的调用。移除这部分逻辑减少了运行时的函数调用和属性绑定计算,对性能有微小的正面影响。
  • 属性绑定
    • 改进:移除了 transientParent: transientParentWindow 的绑定,减少了 QML 引擎对属性依赖关系的追踪。

4. 代码安全

  • 空指针引用
    • 原代码中 if (!transientParentWindow || transientParentWindow === control) 包含了对空值的检查。移除这部分逻辑后,如果外部代码错误地访问了已删除的属性(尽管属性已删除,但如果有外部 JS 或 QML 依赖它),会导致运行时错误。
  • 窗口劫持/激活风险
    • 原代码中的 control.requestActivate() 会强制夺取焦点。虽然这是模态对话框的预期行为,但在某些安全敏感场景下,自动夺取焦点的行为可能会被视为干扰用户操作。移除该逻辑消除了这种潜在的“焦点抢夺”行为,从用户体验角度看可能更安全,但从功能完整性角度看可能破坏了模态对话框的强制交互特性。

综合改进意见

  1. 确认业务逻辑:必须确认该组件是否不再需要作为模态对话框工作,或者是否由父组件统一接管了 transientParent 的设置。如果该组件仍需作为模态对话框使用,强烈建议保留 transientParent 的设置逻辑,否则会破坏用户体验(如窗口层级混乱、无法正确阻塞父窗口等)。
  2. 替代方案:如果目的是简化代码,建议将窗口层级管理的责任上移。即,由创建 DialogWindow 的父组件负责设置 transientParent,而不是在 DialogWindow 内部猜测。例如:
    // 在父组件中
    DialogWindow {
        transientParent: parentWindow // 显式指定
    }
  3. 文档更新:由于移除了自动设置父窗口的功能,务必更新该组件的 API 文档,明确告知使用者该组件不再自动管理窗口层级,需要手动设置 transientParent(如果该属性仍被 Window 继承)。
  4. 兼容性测试:重点测试在不同平台(X11, Wayland, Windows, macOS)下的窗口行为。特别是 Wayland 平台,对 transientParent 的依赖性较强,移除相关逻辑可能导致窗口无法正常弹出。

总结:这次修改在代码简洁性和性能上有微小提升,但在功能逻辑上存在重大变更风险。除非有充分的理由(如改用新的窗口管理架构),否则不应随意移除模态对话框的父窗口设置逻辑。

@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JWWTSL, 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

@JWWTSL
Copy link
Contributor Author

JWWTSL commented Mar 12, 2026

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Mar 12, 2026

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit bd3ef9b into linuxdeepin:master Mar 12, 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