Skip to content

feat: add dynamic position updates for notification bubbles#1500

Open
qxp930712 wants to merge 1 commit intolinuxdeepin:masterfrom
qxp930712:master
Open

feat: add dynamic position updates for notification bubbles#1500
qxp930712 wants to merge 1 commit intolinuxdeepin:masterfrom
qxp930712:master

Conversation

@qxp930712
Copy link

@qxp930712 qxp930712 commented Mar 13, 2026

Added requestPositionUpdate method to DLayerShellWindow to allow QML components to trigger window position recalculations with custom dimensions. This enables the notification panel to dynamically adjust its position based on content height changes.

Key changes:

  1. Added Q_INVOKABLE requestPositionUpdate method in DLayerShellWindow
  2. Implemented onPositionUpdateRequested slot in LayerShellEmulation to handle custom width/height calculations
  3. Modified notification bubble panel to update window height based on content changes
  4. Added smooth removal animations for notification bubbles
  5. Fixed window visibility handling with delayed hiding when empty

The position calculation now properly accounts for anchor constraints and margins when using custom dimensions, ensuring correct window placement regardless of content size changes.

Log: Improved notification panel positioning and animations

Influence:

  1. Test notification display with varying numbers of bubbles
  2. Verify window position updates correctly when content height changes
  3. Check smooth animations when adding/removing notifications
  4. Validate window hides properly after last notification is dismissed
  5. Test with different screen resolutions and scaling factors
  6. Verify anchor constraints work correctly with dynamic sizing

PMS: BUG-284659

Summary by Sourcery

Add support for QML-triggered layer shell position recalculations to keep the notification bubble panel correctly positioned as its content changes.

New Features:

  • Expose a QML-invokable requestPositionUpdate API on DLayerShellWindow to recalculate window position using custom dimensions.
  • Allow the notification bubble panel to update its position dynamically based on the ListView content height and screen anchors.
  • Add animated removal for individual notification bubbles when they are dismissed.

Bug Fixes:

  • Ensure the notification bubble window hides only after a short delay once all bubbles are removed, preventing premature disappearance while animations complete.

Enhancements:

  • Update X11 layer shell emulation to handle explicit position update requests with proper anchor and margin constraints.
  • Adjust notification bubble window configuration to better suit top-layer anchored positioning for dynamic sizing.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qxp930712

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

@sourcery-ai
Copy link

sourcery-ai bot commented Mar 13, 2026

Reviewer's Guide

Implements a QML-invokable position update mechanism for DLayerShellWindow and wires it into the X11 layer shell emulation and notification bubble panel so that notification window geometry is recalculated from QML-provided dimensions, adds removal animations, and refines visibility handling with delayed hiding when empty.

Sequence diagram for dynamic notification window position update

sequenceDiagram
    actor User
    participant NotificationAppletQML as NotificationAppletQML
    participant BubbleView as BubbleView_ListView
    participant DLayerShellWindow as DLayerShellWindow
    participant LayerShellEmulation as LayerShellEmulation
    participant QWindow as QWindow
    participant Screen as Screen

    User->>NotificationAppletQML: Triggers notification content change
    NotificationAppletQML->>BubbleView: Update bubbles model
    BubbleView-->>BubbleView: height changed
    BubbleView->>NotificationAppletQML: onHeightChanged
    NotificationAppletQML->>DLayerShellWindow: requestPositionUpdate(390, contentHeight)
    DLayerShellWindow-->>LayerShellEmulation: positionUpdateRequested(width, height)
    LayerShellEmulation->>Screen: geometry()
    Screen-->>LayerShellEmulation: screenRect
    LayerShellEmulation-->>LayerShellEmulation: compute x,y using anchors and margins
    LayerShellEmulation-->>LayerShellEmulation: adjust width,height if constrained
    LayerShellEmulation->>QWindow: setGeometry(rect)
    QWindow-->>User: Notification panel repositioned with new size
Loading

Sequence diagram for notification bubble removal and delayed panel hiding

sequenceDiagram
    actor User
    participant NotificationAppletQML as NotificationAppletQML
    participant BubbleView as BubbleView_ListView
    participant BubbleDelegate as BubbleDelegate
    participant BubbleModel as BubbleModel
    participant BubblePanel as BubblePanel
    participant QTimer as QTimer_singleShot

    User->>NotificationAppletQML: Dismisses notification
    NotificationAppletQML->>BubbleModel: removeRow(index)
    BubbleModel-->>BubbleView: rowsRemoved
    BubbleView->>BubbleDelegate: ListView.onRemove
    BubbleDelegate-->>BubbleDelegate: SequentialAnimation
    BubbleDelegate-->>BubbleDelegate: PropertyAction delayRemove = true
    BubbleDelegate-->>BubbleDelegate: ParallelAnimation slide out
    BubbleDelegate-->>BubbleDelegate: PropertyAction delayRemove = false
    BubbleModel-->>BubblePanel: rowsRemoved
    BubblePanel->>BubblePanel: onBubbleCountChanged()
    BubblePanel-->>BubblePanel: isEmpty = true
    BubblePanel->>QTimer: singleShot(400, callback)
    QTimer-->>BubblePanel: timeout after 400ms
    BubblePanel->>BubblePanel: setVisible(false)
    BubblePanel-->>NotificationAppletQML: panel hidden
Loading

Updated class diagram for DLayerShellWindow, LayerShellEmulation, and BubblePanel

classDiagram
    class DLayerShellWindow {
        +DLayerShellWindow(QWindow* window)
        +setAnchors(int anchors)
        +anchors() int
        +setTopMargin(int margin)
        +setRightMargin(int margin)
        +setBottomMargin(int margin)
        +setLeftMargin(int margin)
        +requestPositionUpdate(int width, int height)
        +get(QWindow* window) DLayerShellWindow*
        +qmlAttachedProperties(QObject* object) DLayerShellWindow*
        <<signal>> anchorsChanged()
        <<signal>> marginsChanged()
        <<signal>> positionUpdateRequested(int width, int height)
    }

    class LayerShellEmulation {
        -QWindow* m_window
        -DLayerShellWindow* m_dlayerShellWindow
        +LayerShellEmulation(QWindow* window, QObject* parent)
        +onAnchorsChanged()
        +onMarginsChanged()
        +onLayerChanged()
        +onPositionChanged()
        +onPositionUpdateRequested(int width, int height)
        +onExclusionZoneChanged()
        +onScopeChanged()
    }

    class BubbleModel {
        +items() QList
        +rowsInserted()
        +rowsRemoved()
    }

    class BubblePanel {
        -BubbleModel* m_bubbles
        +init() bool
        +onNotificationStateChanged(qint64 id, int processedType)
        +onBubbleCountChanged()
        +addBubble(qint64 id)
        +setVisible(bool visible)
    }

    DLayerShellWindow <.. LayerShellEmulation : observes_signals
    LayerShellEmulation --> QWindow : controls_geometry
    BubblePanel --> BubbleModel : uses
    BubbleModel <.. BubblePanel : emits_rowsInserted_rowsRemoved
Loading

File-Level Changes

Change Details Files
Expose a QML-invokable API on DLayerShellWindow to request position recalculation with custom width/height and propagate it via a new signal.
  • Added Q_INVOKABLE requestPositionUpdate(int width, int height) method which emits a new positionUpdateRequested(width, height) signal.
  • Declared the new positionUpdateRequested(int width, int height) signal to notify when a manual position update is requested from QML.
frame/layershell/dlayershellwindow.h
frame/layershell/dlayershellwindow.cpp
Handle custom-dimension position updates in the X11 LayerShell emulation, respecting anchors and margins, and connect it to DLayerShellWindow.
  • Connected DLayerShellWindow::positionUpdateRequested to a new LayerShellEmulation::onPositionUpdateRequested slot in the constructor.
  • Implemented onPositionUpdateRequested(int width, int height) to compute a QRect based on provided dimensions, window anchors, margins, and screen geometry, including constrained cases where both sides of an axis are anchored.
  • Left existing onPositionChanged() logic intact, with an added warning log that outputs the computed position and window size for debugging.
  • Updated the header to declare the new onPositionUpdateRequested slot.
frame/layershell/x11dlayershellemulation.cpp
frame/layershell/x11dlayershellemulation.h
Make the notification bubble window dynamically drive its own size and position via QML, and adjust its layer/animation behavior.
  • Changed the bubble Window to start with height 0 instead of a content-based expression and set DLayerShellWindow.layer to LayerTop instead of LayerOverlay.
  • Added an updatePosition(width, height) helper function that calls DLayerShellWindow.requestPositionUpdate(...) from QML.
  • Hooked ListView.onHeightChanged to call updatePosition with a fixed window width and a height derived from the bubble view height plus its vertical margins, so window geometry tracks content height changes.
  • Extended the Bubble delegate with a ListView.onRemove SequentialAnimation that delays removal while playing a slide-out animation (x property animated to 360px over 400ms using Easing.InExpo).
panels/notification/bubble/package/main.qml
Adjust bubble panel visibility lifecycle so the panel is initially visible and hides with a delay after the last notification is removed.
  • Included QTimer header for delayed operations.
  • Set the BubblePanel to be visible at init() time to ensure the panel shows when starting.
  • Modified onBubbleCountChanged() so that when the model becomes empty, visibility is turned off after a 400 ms single-shot timer (allowing removal animations to complete), and otherwise visibility follows the non-empty/enabled state.
panels/notification/bubble/bubblepanel.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

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 found 4 issues, and left some high level feedback:

  • The qWarning() added in onPositionChanged() with the hardcoded "caimengci" tag looks like leftover debug logging and should either be removed or converted to use the existing logging category with a clearer, non-personal message.
  • In bubblepanel.cpp you now include both <qtimer.h> and <QTimer>; the lowercase header is non-standard and redundant, so it would be better to drop it and keep the conventional #include <QTimer> only.
  • The change to call setVisible(true) unconditionally in BubblePanel::init() and the delayed setVisible(false) via QTimer::singleShot when empty modifies the panel’s visibility semantics; consider whether this should respect enabled() consistently and guard against the timer firing after the panel is disabled or destroyed.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `qWarning()` added in `onPositionChanged()` with the hardcoded "caimengci" tag looks like leftover debug logging and should either be removed or converted to use the existing logging category with a clearer, non-personal message.
- In `bubblepanel.cpp` you now include both `<qtimer.h>` and `<QTimer>`; the lowercase header is non-standard and redundant, so it would be better to drop it and keep the conventional `#include <QTimer>` only.
- The change to call `setVisible(true)` unconditionally in `BubblePanel::init()` and the delayed `setVisible(false)` via `QTimer::singleShot` when empty modifies the panel’s visibility semantics; consider whether this should respect `enabled()` consistently and guard against the timer firing after the panel is disabled or destroyed.

## Individual Comments

### Comment 1
<location path="frame/layershell/x11dlayershellemulation.cpp" line_range="122-127" />
<code_context>
     auto x = screenRect.left() + (screenRect.width() - m_window->width()) / 2;
     auto y = screenRect.top() + (screenRect.height() - m_window->height()) / 2;
+
+    qWarning() << "caimengci position x=" << x << "y=" << y << "window width=" << m_window->width() << "window height=" << m_window->height();
+    
     if (anchors & DLayerShellWindow::AnchorRight) {
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid hard-coded debug logging left in production code

This `qWarning()` appears to be temporary debug logging and will emit on every position change. Please remove it, or switch to a `qDebug()` with a neutral message if you still need this information.

```suggestion
    auto x = screenRect.left() + (screenRect.width() - m_window->width()) / 2;
    auto y = screenRect.top() + (screenRect.height() - m_window->height()) / 2;

    if (anchors & DLayerShellWindow::AnchorRight) {
```
</issue_to_address>

### Comment 2
<location path="frame/layershell/x11dlayershellemulation.cpp" line_range="160-157" />
<code_context>
+void LayerShellEmulation::onPositionUpdateRequested(int width, int height)
</code_context>
<issue_to_address>
**suggestion:** Factor out shared positioning logic to avoid duplication

`onPositionUpdateRequested` mostly duplicates `onPositionChanged`, differing only in where the size comes from. Consider extracting the anchor/geometry calculation into a helper (e.g. `computeGeometry(QSize size)`) and calling it from both, to keep the logic in sync as anchor handling evolves.

Suggested implementation:

```cpp
QRect LayerShellEmulation::computeGeometry(const QSize &size)
{
    auto anchors = m_dlayerShellWindow->anchors();
    auto screen = m_window->screen();
    auto screenRect = screen->geometry();
    int x = screenRect.left() + (screenRect.width() - size.width()) / 2;
    int y = screenRect.top() + (screenRect.height() - size.height()) / 2;

    if (anchors & DLayerShellWindow::AnchorRight) {
        // https://doc.qt.io/qt-6/qrect.html#right
        x = (screen->geometry().right() + 1 - size.width() - m_dlayerShellWindow->rightMargin());
    }

    return QRect(x, y, size.width(), size.height());
}

void LayerShellEmulation::onPositionUpdateRequested(int width, int height)

```

```cpp
{
    const QSize size(width, height);
    const QRect rect = computeGeometry(size);

    // Keep behavior consistent with other positioning code: move/resize the window.
    m_window->setGeometry(rect);

```

` about similarly updating `onPositionChanged` once the full function body is available.

Here are the changes:

<file_operations>
<file_operation operation="edit" file_path="frame/layershell/x11dlayershellemulation.cpp">
<<<<<<< SEARCH
void LayerShellEmulation::onPositionUpdateRequested(int width, int height)
=======
QRect LayerShellEmulation::computeGeometry(const QSize &size)
{
    auto anchors = m_dlayerShellWindow->anchors();
    auto screen = m_window->screen();
    auto screenRect = screen->geometry();
    int x = screenRect.left() + (screenRect.width() - size.width()) / 2;
    int y = screenRect.top() + (screenRect.height() - size.height()) / 2;

    if (anchors & DLayerShellWindow::AnchorRight) {
        // https://doc.qt.io/qt-6/qrect.html#right
        x = (screen->geometry().right() + 1 - size.width() - m_dlayerShellWindow->rightMargin());
    }

    return QRect(x, y, size.width(), size.height());
}

void LayerShellEmulation::onPositionUpdateRequested(int width, int height)
>>>>>>> REPLACE

<<<<<<< SEARCH
{
    auto anchors = m_dlayerShellWindow->anchors();
    auto screen = m_window->screen();
    auto screenRect = screen->geometry();
    auto x = screenRect.left() + (screenRect.width() - width) / 2;
    auto y = screenRect.top() + (screenRect.height() - height) / 2;

    if (anchors & DLayerShellWindow::AnchorRight) {
        x = (screen->geometry().right() + 1 - width - m_dlayerShellWindow->rightMargin());
    }
=======
{
    const QSize size(width, height);
    const QRect rect = computeGeometry(size);

    // Keep behavior consistent with other positioning code: move/resize the window.
    m_window->setGeometry(rect);
>>>>>>> REPLACE
</file_operation>
</file_operations>

<additional_changes>
To fully realize the refactoring you suggested, the following should also be done once you can see the complete function:

1. In `LayerShellEmulation::onPositionChanged(...)` (or whichever method contains the snippet starting with:
   `auto y = screenRect.top() + (screenRect.height() - m_window->height()) / 2;`
   and using `anchors & DLayerShellWindow::AnchorRight`), replace the inlined anchor/geometry logic with:
   - Constructing a `QSize size(m_window->width(), m_window->height());`
   - Calling `const QRect rect = computeGeometry(size);`
   - Using `rect` for logging (e.g. `rect.x()`, `rect.y()`) and for `m_window->setGeometry(rect);`

2. If `onPositionChanged` also handles other anchors (top, bottom, left, etc.), consider moving that logic into `computeGeometry` as well, mirroring the pattern used for `AnchorRight`, so that all anchor handling is centralized.
</issue_to_address>

### Comment 3
<location path="panels/notification/bubble/bubblepanel.cpp" line_range="117-118" />
<code_context>
     bool isEmpty = m_bubbles->items().isEmpty();
-    setVisible(!isEmpty && enabled());
+
+    if (isEmpty) {
+        QTimer::singleShot(400, this, [this]() {
+            setVisible(false);
+        });
</code_context>
<issue_to_address>
**issue (bug_risk):** Pending hide timer can fire even after bubbles are added

Using `QTimer::singleShot` introduces a race: if the list becomes empty (scheduling the hide) and a bubble is added within 400 ms, the lambda will still call `setVisible(false)`, hiding a non-empty panel. Use a member `QTimer` you can stop when items are added again, or at least re-check that the model is still empty inside the lambda before hiding.
</issue_to_address>

### Comment 4
<location path="panels/notification/bubble/package/main.qml" line_range="72" />
<code_context>
     width: 390
-    height: Math.max(10, bubbleView.height + bubbleView.anchors.topMargin + bubbleView.anchors.bottomMargin)
-    DLayerShellWindow.layer: DLayerShellWindow.LayerOverlay
+    height: 0
+    DLayerShellWindow.layer: DLayerShellWindow.LayerTop
     DLayerShellWindow.anchors: DLayerShellWindow.AnchorBottom | DLayerShellWindow.AnchorRight
</code_context>
<issue_to_address>
**question (bug_risk):** Clarify relationship between QML height and geometry set from C++

`height` is now hard-coded to 0 while the actual geometry is driven from C++ via `requestPositionUpdate`. This may confuse QML code or break layouts that read `Window.height`. Either bind `height` to the same value used in `requestPositionUpdate`, or confirm and document that no QML logic depends on an accurate `height`.
</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 on lines 122 to 127
auto x = screenRect.left() + (screenRect.width() - m_window->width()) / 2;
auto y = screenRect.top() + (screenRect.height() - m_window->height()) / 2;

qWarning() << "caimengci position x=" << x << "y=" << y << "window width=" << m_window->width() << "window height=" << m_window->height();

if (anchors & DLayerShellWindow::AnchorRight) {
Copy link

Choose a reason for hiding this comment

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

suggestion (performance): Avoid hard-coded debug logging left in production code

This qWarning() appears to be temporary debug logging and will emit on every position change. Please remove it, or switch to a qDebug() with a neutral message if you still need this information.

Suggested change
auto x = screenRect.left() + (screenRect.width() - m_window->width()) / 2;
auto y = screenRect.top() + (screenRect.height() - m_window->height()) / 2;
qWarning() << "caimengci position x=" << x << "y=" << y << "window width=" << m_window->width() << "window height=" << m_window->height();
if (anchors & DLayerShellWindow::AnchorRight) {
auto x = screenRect.left() + (screenRect.width() - m_window->width()) / 2;
auto y = screenRect.top() + (screenRect.height() - m_window->height()) / 2;
if (anchors & DLayerShellWindow::AnchorRight) {

@@ -153,6 +157,45 @@ void LayerShellEmulation::onPositionChanged()
m_window->setGeometry(rect);
Copy link

Choose a reason for hiding this comment

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

suggestion: Factor out shared positioning logic to avoid duplication

onPositionUpdateRequested mostly duplicates onPositionChanged, differing only in where the size comes from. Consider extracting the anchor/geometry calculation into a helper (e.g. computeGeometry(QSize size)) and calling it from both, to keep the logic in sync as anchor handling evolves.

Suggested implementation:

QRect LayerShellEmulation::computeGeometry(const QSize &size)
{
    auto anchors = m_dlayerShellWindow->anchors();
    auto screen = m_window->screen();
    auto screenRect = screen->geometry();
    int x = screenRect.left() + (screenRect.width() - size.width()) / 2;
    int y = screenRect.top() + (screenRect.height() - size.height()) / 2;

    if (anchors & DLayerShellWindow::AnchorRight) {
        // https://doc.qt.io/qt-6/qrect.html#right
        x = (screen->geometry().right() + 1 - size.width() - m_dlayerShellWindow->rightMargin());
    }

    return QRect(x, y, size.width(), size.height());
}

void LayerShellEmulation::onPositionUpdateRequested(int width, int height)
{
    const QSize size(width, height);
    const QRect rect = computeGeometry(size);

    // Keep behavior consistent with other positioning code: move/resize the window.
    m_window->setGeometry(rect);

about similarly updatingonPositionChanged` once the full function body is available.

Here are the changes:

<file_operations>
<file_operation operation="edit" file_path="frame/layershell/x11dlayershellemulation.cpp">
<<<<<<< SEARCH
void LayerShellEmulation::onPositionUpdateRequested(int width, int height)

QRect LayerShellEmulation::computeGeometry(const QSize &size)
{
auto anchors = m_dlayerShellWindow->anchors();
auto screen = m_window->screen();
auto screenRect = screen->geometry();
int x = screenRect.left() + (screenRect.width() - size.width()) / 2;
int y = screenRect.top() + (screenRect.height() - size.height()) / 2;

if (anchors & DLayerShellWindow::AnchorRight) {
    // https://doc.qt.io/qt-6/qrect.html#right
    x = (screen->geometry().right() + 1 - size.width() - m_dlayerShellWindow->rightMargin());
}

return QRect(x, y, size.width(), size.height());

}

void LayerShellEmulation::onPositionUpdateRequested(int width, int height)

REPLACE

<<<<<<< SEARCH
{
auto anchors = m_dlayerShellWindow->anchors();
auto screen = m_window->screen();
auto screenRect = screen->geometry();
auto x = screenRect.left() + (screenRect.width() - width) / 2;
auto y = screenRect.top() + (screenRect.height() - height) / 2;

if (anchors & DLayerShellWindow::AnchorRight) {
    x = (screen->geometry().right() + 1 - width - m_dlayerShellWindow->rightMargin());
}

=======
{
const QSize size(width, height);
const QRect rect = computeGeometry(size);

// Keep behavior consistent with other positioning code: move/resize the window.
m_window->setGeometry(rect);

REPLACE
</file_operation>
</file_operations>

<additional_changes>
To fully realize the refactoring you suggested, the following should also be done once you can see the complete function:

  1. In LayerShellEmulation::onPositionChanged(...) (or whichever method contains the snippet starting with:
    auto y = screenRect.top() + (screenRect.height() - m_window->height()) / 2;
    and using anchors & DLayerShellWindow::AnchorRight), replace the inlined anchor/geometry logic with:

    • Constructing a QSize size(m_window->width(), m_window->height());
    • Calling const QRect rect = computeGeometry(size);
    • Using rect for logging (e.g. rect.x(), rect.y()) and for m_window->setGeometry(rect);
  2. If onPositionChanged also handles other anchors (top, bottom, left, etc.), consider moving that logic into computeGeometry as well, mirroring the pattern used for AnchorRight, so that all anchor handling is centralized.

Comment on lines +117 to +118
if (isEmpty) {
QTimer::singleShot(400, this, [this]() {
Copy link

Choose a reason for hiding this comment

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

issue (bug_risk): Pending hide timer can fire even after bubbles are added

Using QTimer::singleShot introduces a race: if the list becomes empty (scheduling the hide) and a bubble is added within 400 ms, the lambda will still call setVisible(false), hiding a non-empty panel. Use a member QTimer you can stop when items are added again, or at least re-check that the model is still empty inside the lambda before hiding.

width: 390
height: Math.max(10, bubbleView.height + bubbleView.anchors.topMargin + bubbleView.anchors.bottomMargin)
DLayerShellWindow.layer: DLayerShellWindow.LayerOverlay
height: 0
Copy link

Choose a reason for hiding this comment

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

question (bug_risk): Clarify relationship between QML height and geometry set from C++

height is now hard-coded to 0 while the actual geometry is driven from C++ via requestPositionUpdate. This may confuse QML code or break layouts that read Window.height. Either bind height to the same value used in requestPositionUpdate, or confirm and document that no QML logic depends on an accurate height.

auto x = screenRect.left() + (screenRect.width() - m_window->width()) / 2;
auto y = screenRect.top() + (screenRect.height() - m_window->height()) / 2;

qWarning() << "caimengci position x=" << x << "y=" << y << "window width=" << m_window->width() << "window height=" << m_window->height();
Copy link
Member

Choose a reason for hiding this comment

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

这个日志还有用吗?甚至是warning

#include "bubblemodel.h"
#include "dataaccessorproxy.h"
#include "pluginfactory.h"
#include <qtimer.h>
Copy link
Member

Choose a reason for hiding this comment

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

No description provided.

@deepin-ci-robot
Copy link

deepin pr auto review

代码审查报告

整体评估

这段代码主要实现了Layer Shell窗口的位置更新机制和通知气泡面板的显示/隐藏逻辑。整体功能实现正确,但存在一些代码质量、安全性和性能方面的问题需要改进。

详细审查意见

1. 代码质量问题

1.1 调试代码未移除

位置: x11dlayershellemulation.cpp 第124行

qWarning() << "caimengci position x=" << x << "y=" << y << "window width=" << m_window->width() << "window height=" << m_window->height();

问题: 包含调试代码,且使用了个人名字作为标识
建议: 移除或替换为正式的调试日志

qCDebug(LOG) << "Position updated:" << x << y << "window size:" << m_window->size();

1.2 重复代码

位置: x11dlayershellemulation.cpponPositionChangedonPositionUpdateRequested 方法
问题: 两个方法有大量重复的位置计算逻辑
建议: 提取公共逻辑到私有辅助方法

QRect LayerShellEmulation::calculateGeometry(int width, int height) const
{
    // 提取公共的位置计算逻辑
}

void LayerShellEmulation::onPositionChanged()
{
    m_window->setGeometry(calculateGeometry(m_window->width(), m_window->height()));
}

void LayerShellEmulation::onPositionUpdateRequested(int width, int height)
{
    m_window->setGeometry(calculateGeometry(width, height));
}

1.3 头文件包含顺序

位置: bubblepanel.cpp 第7-10行

#include "bubblemodel.h"
#include "dataaccessorproxy.h"
#include "pluginfactory.h"
#include <qtimer.h>

#include <QTimer>

问题: 头文件包含顺序混乱,且QTimer被包含两次
建议: 按照标准顺序组织头文件:相关头文件 -> 项目头文件 -> 系统头文件

2. 逻辑问题

2.1 窗口高度初始值

位置: main.qml 第72行

height: 0

问题: 初始高度设置为0可能导致布局问题
建议: 设置合理的最小高度或使用动态计算

height: Math.max(10, bubbleView.height + bubbleView.anchors.topMargin + bubbleView.anchors.bottomMargin)

2.2 窗口可见性逻辑

位置: bubblepanel.cpp 第116-123行

if (isEmpty) {
    QTimer::singleShot(400, this, [this]() {
        setVisible(false);
    });
} else {
    setVisible(!isEmpty && enabled());
}

问题:

  1. 延迟隐藏可能导致用户在400ms内看到空窗口
  2. 没有考虑窗口被禁用的情况
    建议: 优化可见性逻辑
if (isEmpty) {
    // 立即隐藏或使用更短的延迟
    QTimer::singleShot(200, this, [this]() {
        if (m_bubbles->items().isEmpty()) {
            setVisible(false);
        }
    });
} else {
    setVisible(enabled());
}

2.3 边距计算不一致

位置: main.qml 第107-110行

anchors {
    bottom: parent.bottom
    bottomMargin: 10
    rightMargin: 10
    topMargin: 10
    margins: 10
}

问题: margins 会覆盖所有边距设置,导致 topMarginbottomMargin 设置无效
建议: 移除 margins 或明确设置所有边距

anchors {
    bottom: parent.bottom
    right: parent.right
    topMargin: 10
    bottomMargin: 10
    rightMargin: 10
}

3. 性能问题

3.1 频繁的位置更新

位置: main.qml 第112-114行

onHeightChanged: {
    updatePosition(390, bubbleView.height + bubbleView.anchors.topMargin + bubbleView.anchors.bottomMargin)
}

问题: 每次高度变化都会触发位置更新,可能导致频繁重绘
建议: 使用防抖机制

Timer {
    id: updateTimer
    interval: 50
    onTriggered: DLayerShellWindow.requestPositionUpdate(390, bubbleView.height + bubbleView.anchors.topMargin + bubbleView.anchors.bottomMargin)
}

// 修改onHeightChanged
onHeightChanged: updateTimer.restart()

4. 安全问题

4.1 缺少边界检查

位置: x11dlayershellemulation.cpponPositionUpdateRequested 方法
问题: 没有检查传入的宽高是否合理
建议: 添加边界检查

void LayerShellEmulation::onPositionUpdateRequested(int width, int height)
{
    if (width <= 0 || height <= 0) {
        qCWarning(LOG) << "Invalid window size requested:" << width << height;
        return;
    }
    // 原有逻辑...
}

4.2 QML与C++交互安全性

位置: main.qml 第94-96行

function updatePosition(width, height) {
    DLayerShellWindow.requestPositionUpdate(width, height)
}

问题: QML可以直接调用C++方法,可能被滥用
建议: 考虑添加权限检查或限制调用频率

总结建议

  1. 重构 x11dlayershellemulation.cpp 中的位置计算逻辑,减少代码重复
  2. 修复 main.qml 中的边距设置问题
  3. onPositionUpdateRequested 添加参数验证
  4. 优化窗口可见性切换逻辑,减少闪烁
  5. 移除调试代码或使用正式的日志系统
  6. 考虑为频繁触发的位置更新添加防抖机制

这些改进将提高代码的可维护性、可靠性和性能。

Added requestPositionUpdate method to DLayerShellWindow to allow
QML components to trigger window position recalculations with custom
dimensions. This enables the notification panel to dynamically adjust
its position based on content height changes.

Key changes:
1. Added Q_INVOKABLE requestPositionUpdate method in DLayerShellWindow
2. Implemented onPositionUpdateRequested slot in LayerShellEmulation to
handle custom width/height calculations
3. Modified notification bubble panel to update window height based on
content changes
4. Added smooth removal animations for notification bubbles
5. Fixed window visibility handling with delayed hiding when empty

The position calculation now properly accounts for anchor constraints
and margins when using custom dimensions, ensuring correct window
placement regardless of content size changes.

Log: Improved notification panel positioning and animations

Influence:
1. Test notification display with varying numbers of bubbles
2. Verify window position updates correctly when content height changes
3. Check smooth animations when adding/removing notifications
4. Validate window hides properly after last notification is dismissed
5. Test with different screen resolutions and scaling factors
6. Verify anchor constraints work correctly with dynamic sizing

PMS: BUG-284659
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.

4 participants