fix: improve notification center margin calculation #1363
+41
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wjyrich 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 GuideRefactors notification center margin handling to use dynamic, screen-aware dock geometry with device-pixel–aware calculations and stored margin properties, fixing positioning issues and flicker when the dock is shown/hidden or moved. Sequence diagram for dynamic dock geometry margin updatessequenceDiagram
actor User
participant DockApplet
participant NotificationCenterWindow as RootWindow
participant DLayerShellWindow
participant Screen
User->>DockApplet: Change dock visibility/position
DockApplet->>DockApplet: Update frontendWindowRect
DockApplet-->>NotificationCenterWindow: frontendWindowRectChanged(frontendWindowRect)
NotificationCenterWindow->>NotificationCenterWindow: updateMargins(frontendRect)
NotificationCenterWindow->>DockApplet: Read screenName, position, hideState, dockSize
NotificationCenterWindow->>Screen: Read name, devicePixelRatio, virtualX, virtualY, width, height
NotificationCenterWindow->>NotificationCenterWindow: Compute dockGeometry and screenGeometry
alt Dock on same screen and visible
alt DockPositionTop
NotificationCenterWindow->>NotificationCenterWindow: Set topMarginValue
else DockPositionRight
NotificationCenterWindow->>NotificationCenterWindow: Set rightMarginValue
else DockPositionBottom
NotificationCenterWindow->>DockApplet: Read hideState and dockSize
NotificationCenterWindow->>NotificationCenterWindow: Set bottomMarginValue with anti_flicker_logic
end
else Dock on other screen or not valid
NotificationCenterWindow->>NotificationCenterWindow: Reset all margin values to 0
end
NotificationCenterWindow-->>DLayerShellWindow: topMargin, rightMargin, bottomMargin bindings updated
DLayerShellWindow->>User: Notification center repositions without flicker
Class diagram for updated notification center margin handlingclassDiagram
class RootWindow {
+int topMarginValue
+int rightMarginValue
+int bottomMarginValue
+updateMargins(frontendRect)
+onRequestClosePopup()
+onFrontendWindowRectChanged(frontendWindowRect)
}
class DockApplet {
+string screenName
+int hideState
+int position
+int dockSize
+rect frontendWindowRect
+frontendWindowRectChanged(frontendWindowRect)
}
class Screen {
+string name
+double devicePixelRatio
+int virtualX
+int virtualY
+int width
+int height
}
class DLayerShellWindow {
+int topMargin
+int rightMargin
+int bottomMargin
+int exclusionZone
+int keyboardInteractivity
}
RootWindow --> DockApplet : reads_properties_and_listens_to_signal
RootWindow --> Screen : uses_for_geometry_and_dpr
RootWindow --> DLayerShellWindow : binds_margin_properties
DLayerShellWindow ..> Screen : positioned_on
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
updateMarginsfunction repeats the pattern of resetting all margin values to 0 in several early-return branches; consider extracting this into a small helper (e.g.resetMargins()) to reduce duplication and make the intent clearer. - The
switch (dockPosition)uses raw numeric values (0/1/2) with comments forDOCK_TOP/RIGHT/BOTTOM; if the dock API exposes an enum or constants for these positions, using them instead of magic numbers would make the code more self-documenting and less error-prone. - For the bottom dock you explicitly gate the margin on
hideState === Dock.Hide, but for top and right positions the hide state is not considered (only geometry); it may be worth clarifying or aligning this behavior so that margin handling for hidden/auto-hidden docks is consistent across all positions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `updateMargins` function repeats the pattern of resetting all margin values to 0 in several early-return branches; consider extracting this into a small helper (e.g. `resetMargins()`) to reduce duplication and make the intent clearer.
- The `switch (dockPosition)` uses raw numeric values (0/1/2) with comments for `DOCK_TOP/RIGHT/BOTTOM`; if the dock API exposes an enum or constants for these positions, using them instead of magic numbers would make the code more self-documenting and less error-prone.
- For the bottom dock you explicitly gate the margin on `hideState === Dock.Hide`, but for top and right positions the hide state is not considered (only geometry); it may be worth clarifying or aligning this behavior so that margin handling for hidden/auto-hidden docks is consistent across all positions.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
TAG Bot New tag: 2.0.21 |
There was a problem hiding this comment.
Pull request overview
This PR improves the notification center's margin calculation to properly account for the dock's position and visibility. The change replaces a simple position-based margin calculation with a comprehensive geometry-based approach that handles device pixel ratios and dynamic dock state changes.
Key changes:
- Replaced
windowMargin(position)function withupdateMargins(frontendRect)that uses dock geometry instead of simple position checks - Added property storage for margin values (topMarginValue, rightMarginValue, bottomMarginValue) to avoid recalculation on every access
- Implemented device pixel ratio conversion to translate dock geometry from device pixels to logical pixels for accurate positioning
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| newRightMargin = visibleWidth | ||
| break | ||
| } | ||
| //特殊处理:因为会上述计算会导致抖动 |
There was a problem hiding this comment.
The comment has a grammatical error. It says "因为会上述计算会导致抖动" which contains "会" twice. It should be "因为上述计算会导致抖动" (remove the first "会").
Suggested change
| //特殊处理:因为会上述计算会导致抖动 | |
| //特殊处理:因为上述计算会导致抖动 |
| let dockSize = dockApplet.dockSize | ||
| let dockPosition = dockApplet.position | ||
| return dockPosition === position ? dockSize : 0 | ||
| let screen = root.screen ? root.screen.name : "" |
There was a problem hiding this comment.
The screen name check on line 31 is redundant. Line 23 already checks if root.screen is falsy and returns early, so by this point root.screen is guaranteed to be non-null. The ternary operator "root.screen ? root.screen.name : ''" can be simplified to just "root.screen.name".
Suggested change
| let screen = root.screen ? root.screen.name : "" | |
| let screen = root.screen.name |
| topMarginValue = newTopMargin | ||
| rightMarginValue = newRightMargin | ||
| bottomMarginValue = newBottomMargin | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace at the end of this line. This should be removed to maintain code cleanliness.
Suggested change
Comment on lines
153
to
115
| function onFrontendWindowRectChanged(frontendWindowRect) { | ||
| root.updateMargins(frontendWindowRect) | ||
| } |
There was a problem hiding this comment.
The updateMargins function is only called when frontendWindowRectChanged signal is emitted, but it's never called during component initialization or when the screen changes. This means the notification center may have incorrect margins when first displayed or when moved to a different screen. Consider calling updateMargins in a Component.onCompleted handler and in the onScreenChanged handler.
Comment on lines
+62
to
+44
| let screenGeometry = Qt.rect( | ||
| root.screen.virtualX, | ||
| root.screen.virtualY, | ||
| root.screen.width, | ||
| root.screen.height | ||
| ) |
There was a problem hiding this comment.
The screen geometry is recalculated every time updateMargins is called, even when only the dock's frontendRect changes. Since screen geometry changes are rare, consider caching the screen geometry and only recalculating it when the screen actually changes to improve performance.
|
TAG Bot New tag: 2.0.22 |
|
TAG Bot New tag: 2.0.23 |
|
TAG Bot New tag: 2.0.24 |
|
TAG Bot New tag: 2.0.25 |
|
TAG Bot New tag: 2.0.26 |
|
TAG Bot New tag: 2.0.27 |
|
TAG Bot New tag: 2.0.28 |
wjyrich
force-pushed
the
fix-bug-340795
branch
2 times, most recently
from
1c076fd to
4f2f176
Compare
wjyrich
force-pushed
the
fix-bug-340795
branch
from
4f2f176 to
62b019d
Compare
1. Fixed notification center window positioning logic to use frontendWindowRect for accurate dock geometry 2. Added special handling for bottom dock to avoid jitter by checking hideState 3. Improved margin calculation by considering actual visible dock area on screen edges 4. Added property binding for frontendRect updates when dock geometry changes Log: Fixed notification center positioning issues near dock edges Influence: 1. Test notification center positioning when dock is at top/right/bottom positions 2. Verify no jitter when dock is at bottom with hide/show states 3. Test positioning accuracy when dock spans multiple screens 4. Verify correct margin calculation when dock is partially off-screen 5. Test notification center behavior during dock position changes 6. Verify frontendRect updates properly when dock geometry changes fix: 修复通知中心窗口定位问题 1. 修复通知中心窗口定位逻辑，使用 frontendWindowRect 获取准确的 dock 几 何信息 2. 为底部 dock 添加特殊处理，通过检查 hideState 避免抖动问题 3. 改进边距计算，考虑屏幕边缘 dock 的实际可见区域 4. 添加 frontendRect 属性绑定，在 dock 几何信息变化时更新 Log: 修复通知中心在 dock 边缘的定位问题 Influence: 1. 测试通知中心在 dock 位于顶部/右侧/底部时的定位 2. 验证 dock 在底部隐藏/显示状态时无抖动 3. 测试 dock 跨多个屏幕时的定位准确性 4. 验证 dock 部分超出屏幕时的边距计算正确性 5. 测试 dock 位置变化时通知中心的行为 6. 验证 dock 几何信息变化时 frontendRect 正确更新 PMS: BUG-340795
wjyrich
force-pushed
the
fix-bug-340795
branch
from
62b019d to
4de5744
Compare
deepin pr auto review这段代码主要实现了通知中心面板根据任务栏(Dock)的位置和状态计算窗口边距的功能。以下是对该代码的详细审查和改进建议： 1. 语法与逻辑审查
2. 代码质量
3. 代码性能
4. 代码安全
改进后的代码建议import QtQuick 2.12
import QtQuick.Window 2.12
import org.deepin.dtk 1.0
import org.deepin.ds 1.0
import org.deepin.ds.notification
import org.deepin.ds.notificationcenter
import org.deepin.ds.dock 1.0
Window {
id: root
// 修正：初始化时不引用不存在的变量，由信号更新
property rect frontendRect: Qt.rect(0, 0, 0, 0)
function windowMargin(position) {
let dockApplet = DS.applet("org.deepin.ds.dock")
if (!dockApplet)
return 0
let dockScreen = dockApplet.screenName
let screen = root.screen.name
if (dockScreen !== screen)
return 0
let dockPosition = dockApplet.position
if (dockPosition !== position)
return 0
// 底部Dock特殊处理：使用hideState判断，避免动画或状态切换时的抖动
if (dockPosition === Dock.Position.Bottom) {
if (dockApplet.hideState === Dock.Hide)
return 0
return dockApplet.dockSize
}
let dpr = root.screen.devicePixelRatio
// 确保dpr不为0，防止除以零错误
if (dpr === 0) dpr = 1.0
let dockGeometry = Qt.rect(
frontendRect.x / dpr,
frontendRect.y / dpr,
frontendRect.width / dpr,
frontendRect.height / dpr
)
let screenGeometry = Qt.rect(
root.screen.virtualX,
root.screen.virtualY,
root.screen.width,
root.screen.height
)
switch (dockPosition) {
case Dock.Position.Top: {
// 计算Dock在屏幕内的可见高度
let visibleHeight = Math.max(0, dockGeometry.y + dockGeometry.height - screenGeometry.y)
return Math.min(visibleHeight, dockGeometry.height)
}
case Dock.Position.Right: {
// 计算Dock在屏幕内的可见宽度
let visibleWidth = Math.max(0, screenGeometry.x + screenGeometry.width - dockGeometry.x)
return Math.min(visibleWidth, dockGeometry.width)
}
case Dock.Position.Left: { // 新增：左侧Dock的处理
// 计算Dock在屏幕内的可见宽度
let visibleWidth = Math.max(0, dockGeometry.x + dockGeometry.width - screenGeometry.x)
return Math.min(visibleWidth, dockGeometry.width)
}
default:
return 0
}
}
// ... 其他代码 ...
Connections {
target: DS.applet("org.deepin.ds.dock")
function onFrontendWindowRectChanged(frontendWindowRect) {
root.frontendRect = frontendWindowRect
}
}
}总结这段代码的主要改进点在于：
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Log: Fixed notification center positioning issues when dock is visible
fix: 改进通知中心边距计算
Log: 修复任务栏可见时通知中心定位问题
PMS: BUG-340799
剪切板相关修复： #1308 linuxdeepin/dde-clipboard#215