feat: add dynamic position updates for notification bubbles#1500
feat: add dynamic position updates for notification bubbles#1500qxp930712 wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideImplements 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 updatesequenceDiagram
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
Sequence diagram for notification bubble removal and delayed panel hidingsequenceDiagram
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
Updated class diagram for DLayerShellWindow, LayerShellEmulation, and BubblePanelclassDiagram
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
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The
qWarning()added inonPositionChanged()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.cppyou 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 inBubblePanel::init()and the delayedsetVisible(false)viaQTimer::singleShotwhen empty modifies the panel’s visibility semantics; consider whether this should respectenabled()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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 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) { |
There was a problem hiding this comment.
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.
| 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); | |||
There was a problem hiding this comment.
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:
-
In
LayerShellEmulation::onPositionChanged(...)(or whichever method contains the snippet starting with:
auto y = screenRect.top() + (screenRect.height() - m_window->height()) / 2;
and usinganchors & 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
rectfor logging (e.g.rect.x(),rect.y()) and form_window->setGeometry(rect);
- Constructing a
-
If
onPositionChangedalso handles other anchors (top, bottom, left, etc.), consider moving that logic intocomputeGeometryas well, mirroring the pattern used forAnchorRight, so that all anchor handling is centralized.
| if (isEmpty) { | ||
| QTimer::singleShot(400, this, [this]() { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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(); |
| #include "bubblemodel.h" | ||
| #include "dataaccessorproxy.h" | ||
| #include "pluginfactory.h" | ||
| #include <qtimer.h> |
deepin pr auto review代码审查报告整体评估这段代码主要实现了Layer Shell窗口的位置更新机制和通知气泡面板的显示/隐藏逻辑。整体功能实现正确,但存在一些代码质量、安全性和性能方面的问题需要改进。 详细审查意见1. 代码质量问题1.1 调试代码未移除位置: 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 重复代码位置: 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 头文件包含顺序位置: #include "bubblemodel.h"
#include "dataaccessorproxy.h"
#include "pluginfactory.h"
#include <qtimer.h>
#include <QTimer>问题: 头文件包含顺序混乱,且QTimer被包含两次 2. 逻辑问题2.1 窗口高度初始值位置: height: 0问题: 初始高度设置为0可能导致布局问题 height: Math.max(10, bubbleView.height + bubbleView.anchors.topMargin + bubbleView.anchors.bottomMargin)2.2 窗口可见性逻辑位置: if (isEmpty) {
QTimer::singleShot(400, this, [this]() {
setVisible(false);
});
} else {
setVisible(!isEmpty && enabled());
}问题:
if (isEmpty) {
// 立即隐藏或使用更短的延迟
QTimer::singleShot(200, this, [this]() {
if (m_bubbles->items().isEmpty()) {
setVisible(false);
}
});
} else {
setVisible(enabled());
}2.3 边距计算不一致位置: anchors {
bottom: parent.bottom
bottomMargin: 10
rightMargin: 10
topMargin: 10
margins: 10
}问题: anchors {
bottom: parent.bottom
right: parent.right
topMargin: 10
bottomMargin: 10
rightMargin: 10
}3. 性能问题3.1 频繁的位置更新位置: 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 缺少边界检查位置: 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++交互安全性位置: function updatePosition(width, height) {
DLayerShellWindow.requestPositionUpdate(width, height)
}问题: QML可以直接调用C++方法,可能被滥用 总结建议
这些改进将提高代码的可维护性、可靠性和性能。 |
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
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:
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:
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:
Bug Fixes:
Enhancements: