fix(notification): fix invisible delegates after collapsing expanded …#1476
Open
re2zero wants to merge 1 commit intolinuxdeepin:masterfrom
Open
fix(notification): fix invisible delegates after collapsing expanded …#1476re2zero wants to merge 1 commit intolinuxdeepin:masterfrom
re2zero wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: re2zero 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 guide (collapsed on small PRs)Reviewer's GuideReplaces a disruptive layoutChanged() emit with a lightweight ListView refresh mechanism by simulating a tiny scroll after collapsing/expanding notification app groups, ensuring displaced delegates are re-rendered without breaking animations. Sequence diagram for refreshed delegates after notification collapse/expandsequenceDiagram
actor User
participant NotifyViewDelegate
participant NotifyModel
participant NotifyView_ListView
User->>NotifyViewDelegate: clickCollapse(appGroup)
NotifyViewDelegate->>NotifyModel: collapseApp(index)
NotifyModel-->>NotifyViewDelegate: collapseCompleted
NotifyViewDelegate->>NotifyView_ListView: refreshAfterCollapse()
activate NotifyView_ListView
NotifyView_ListView->>NotifyView_ListView: Qt.callLater(callback)
Note over NotifyView_ListView: callback executes shortly after
NotifyView_ListView->>NotifyView_ListView: contentY++ (tiny scroll)
NotifyView_ListView->>NotifyView_ListView: re-render displaced delegates
deactivate NotifyView_ListView
User->>NotifyViewDelegate: clickExpand(appGroup)
NotifyViewDelegate->>NotifyModel: expandApp(index)
NotifyModel-->>NotifyViewDelegate: expandCompleted
NotifyViewDelegate->>NotifyView_ListView: refreshAfterCollapse()
activate NotifyView_ListView
NotifyView_ListView->>NotifyView_ListView: Qt.callLater(callback)
NotifyView_ListView->>NotifyView_ListView: contentY++ (tiny scroll)
NotifyView_ListView->>NotifyView_ListView: re-render displaced delegates
deactivate NotifyView_ListView
NotifyViewDelegate->>NotifyView_ListView: requestFocusOnExpand(targetIndex)
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 - I've found 1 issue, and left some high level feedback:
- The
refreshAfterCollapseimplementation permanently incrementscontentYon every call; consider storing the current value and restoring it (or doing a small up/down adjustment in the samecallLater) to avoid cumulative drift and unexpected jumps for users who collapse/expand frequently. - Since
refreshAfterCollapseis also called after expand, you might want to rename it to something more generic (e.g.refreshAfterToggle) and document any assumptions aboutroot.view(that it directly exposescontentYand is aListView) to make the coupling clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `refreshAfterCollapse` implementation permanently increments `contentY` on every call; consider storing the current value and restoring it (or doing a small up/down adjustment in the same `callLater`) to avoid cumulative drift and unexpected jumps for users who collapse/expand frequently.
- Since `refreshAfterCollapse` is also called after expand, you might want to rename it to something more generic (e.g. `refreshAfterToggle`) and document any assumptions about `root.view` (that it directly exposes `contentY` and is a `ListView`) to make the coupling clearer.
## Individual Comments
### Comment 1
<location path="panels/notification/center/NotifyView.qml" line_range="87-89" />
<code_context>
+ // Refresh delegates after collapse/expand by simulating a tiny scroll
+ // This triggers ListView to re-render displaced delegates that may be invisible
+ // Called immediately after collapseApp/expandApp, before transition starts
+ function refreshAfterCollapse() {
+ Qt.callLater(function() {
+ contentY++
+ })
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** refreshAfterCollapse permanently shifts scroll position by 1 unit on every call
Because `contentY` is only incremented and never reset, each collapse/expand shifts the viewport down by 1 unit, gradually drifting the view from the user’s expected position. If you just need to trigger a re-render, use a net-zero scroll (e.g., `contentY += 1; contentY -= 1`, possibly via two `Qt.callLater` calls) or a more direct layout invalidation API so the visible scroll position stays stable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
re2zero
force-pushed
the
bugfix
branch
3 times, most recently
from
6e5e364 to
0aa40d0
Compare
…notifications Remove emit layoutChanged() which breaks animations and add refresh mechanism to trigger ListView re-render of displaced delegates. 删除emit layoutChanged()避免破坏动画，通过模拟微小滚动触发ListView 重新渲染被移位的delegate。 PMS: BUG-351731 BUG-351729 Log: 修复收起超一屏应用通知时下方应用不显示的问题 Influence: 修复后收起应用通知下方应用正常显示，无需手动刷新
deepin pr auto review这份代码变更主要是针对通知中心列表视图（ListView）在折叠/展开应用分组后，某些委托项可能未正确渲染的问题，提供了一种通过模拟微小滚动来触发视图刷新的变通方案。此外，还更新了版权年份。 以下是针对此变更的详细审查意见： 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
改进建议虽然当前的实现可以解决问题，但为了更健壮和可维护，建议考虑以下优化：
总结这是一个有效且高效的修复方案。它用较低成本的 QML 动画帧操作替代了高成本的 C++ 模型重置信号，解决了 ListView 的特定渲染问题。代码逻辑清晰，注释完善。唯一的小建议是优化函数名以适应更广泛的使用场景，并增加对不可滚动列表的防御性检查。 |
18202781743
reviewed
Mar 4, 2026
| console.log("expand") | ||
| let expandIndex = model.index | ||
| notifyModel.expandApp(expandIndex) | ||
| root.view.refreshAfterCollapse() |
Contributor
There was a problem hiding this comment.
函数名改一下吧,它跟collapse也没啥关系了,
There was a problem hiding this comment.
Pull request overview
This PR fixes a UI issue in the notification center where delegates below a collapsed/expanded app group can become invisible, by avoiding a model layoutChanged() emission (which breaks animations) and adding a QML-side refresh mechanism to force the ListView to re-render displaced delegates.
Changes:
- Removed
emit layoutChanged()fromNotifyModel::collapseApp()to avoid breakingListViewtransitions/animations. - Added a
ListView.refreshAfterCollapse()helper in QML that simulates a tiny scroll to trigger delegate re-rendering. - Invoked the refresh helper after group collapse/expand actions in the delegate.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| panels/notification/center/notifymodel.cpp | Stops emitting layoutChanged() during collapse to preserve animations. |
| panels/notification/center/NotifyViewDelegate.qml | Calls the new view refresh hook after collapse/expand actions. |
| panels/notification/center/NotifyView.qml | Adds refreshAfterCollapse() implementation via small contentY nudge. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+72
to
73
| root.view.refreshAfterCollapse() | ||
| root.view.requestFocusOnExpand(collapseIndex) |
There was a problem hiding this comment.
In these handlers refreshAfterCollapse() is called before requestFocusOnExpand(). Because refreshAfterCollapse schedules a Qt.callLater contentY adjustment, the later adjustment can run after positionViewAtIndex() and shift the final scroll position by 1px. Either call requestFocusOnExpand() first, or change refreshAfterCollapse() to restore to a captured contentY so it can’t interfere with subsequent scrolling.
Suggested change
| root.view.refreshAfterCollapse() | |
| root.view.requestFocusOnExpand(collapseIndex) | |
| root.view.requestFocusOnExpand(collapseIndex) | |
| root.view.refreshAfterCollapse() |
Comment on lines
+215
to
216
| root.view.refreshAfterCollapse() | ||
| root.view.requestFocusOnExpand(expandIndex + 1) |
There was a problem hiding this comment.
Same ordering issue as above: refreshAfterCollapse() runs before requestFocusOnExpand(), and its deferred contentY undo can run after positionViewAtIndex(), leaving the view shifted by 1px. Reorder the calls or make refreshAfterCollapse() restore to a captured contentY value.
Suggested change
| root.view.refreshAfterCollapse() | |
| root.view.requestFocusOnExpand(expandIndex + 1) | |
| root.view.requestFocusOnExpand(expandIndex + 1) | |
| root.view.refreshAfterCollapse() |
Comment on lines
+84
to
+95
| // Refresh delegates after collapse/expand by simulating a tiny scroll | ||
| // This triggers ListView to re-render displaced delegates that may be invisible | ||
| // Called immediately after collapseApp/expandApp, before transition starts | ||
| function refreshAfterCollapse() { | ||
| if (contentY > 0) { | ||
| contentY--; | ||
| Qt.callLater(function() { contentY++; }); | ||
| } else { | ||
| contentY++; | ||
| Qt.callLater(function() { contentY--; }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Function name refreshAfterCollapse() is now used for both collapse and expand flows, which makes the intent harder to follow. Consider renaming it to reflect both cases (e.g., refreshAfterGroupToggle / refreshAfterModelChange) so future call sites don’t mislead readers.
Comment on lines
+84
to
+94
| // Refresh delegates after collapse/expand by simulating a tiny scroll | ||
| // This triggers ListView to re-render displaced delegates that may be invisible | ||
| // Called immediately after collapseApp/expandApp, before transition starts | ||
| function refreshAfterCollapse() { | ||
| if (contentY > 0) { | ||
| contentY--; | ||
| Qt.callLater(function() { contentY++; }); | ||
| } else { | ||
| contentY++; | ||
| Qt.callLater(function() { contentY--; }); | ||
| } |
There was a problem hiding this comment.
refreshAfterCollapse() nudges contentY and then uses Qt.callLater to undo it, but the undo step is relative to whatever contentY is at that later time. If anything else changes contentY between the two steps (e.g., positionViewAtIndex in requestFocusOnExpand), this will leave the ListView offset by 1px and can break SnapToItem / focus positioning. Consider capturing the intended final contentY and restoring exactly to that value (or avoid changing contentY at all by using ListView.forceLayout() if it solves the re-render issue).
Suggested change
| // Refresh delegates after collapse/expand by simulating a tiny scroll | |
| // This triggers ListView to re-render displaced delegates that may be invisible | |
| // Called immediately after collapseApp/expandApp, before transition starts | |
| function refreshAfterCollapse() { | |
| if (contentY > 0) { | |
| contentY--; | |
| Qt.callLater(function() { contentY++; }); | |
| } else { | |
| contentY++; | |
| Qt.callLater(function() { contentY--; }); | |
| } | |
| // Refresh delegates after collapse/expand by forcing a layout pass | |
| // This triggers ListView to re-render displaced delegates that may be invisible | |
| // Called immediately after collapseApp/expandApp, before transition starts | |
| function refreshAfterCollapse() { | |
| forceLayout() |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…notifications
Remove emit layoutChanged() which breaks animations and add refresh mechanism to trigger ListView re-render of displaced delegates.
删除emit layoutChanged()避免破坏动画，通过模拟微小滚动触发ListView重新渲染被移位的delegate。
PMS: BUG-351731 BUG-351729
Log: 修复收起超一屏应用通知时下方应用不显示的问题
Influence: 修复后收起应用通知下方应用正常显示，无需手动刷新
Summary by Sourcery
Bug Fixes: