fix(windowed): optimize sort mode switch performance#783
Open
Ivy233 wants to merge 1 commit into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ivy233 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 GuideOptimizes windowed launcher sort-mode switching by replacing QML Loader-based view swapping with dual always-instantiated views toggled via visibility, and by changing the categorized sort proxy model to use dynamic sort/filter toggling instead of full model resets, preserving delegates and avoiding redundant sorts. Sequence diagram for CategorizedSortProxyModel::setCategoryType dynamic sort togglesequenceDiagram
participant Caller
participant CategorizedSortProxyModel
participant AppListView
Caller->>CategorizedSortProxyModel: setCategoryType(categoryType)
CategorizedSortProxyModel->>CategorizedSortProxyModel: setDynamicSortFilter(false)
Note over CategorizedSortProxyModel: update isFreeSort and config
CategorizedSortProxyModel->>CategorizedSortProxyModel: setDynamicSortFilter(true)
CategorizedSortProxyModel-->>AppListView: layoutAboutToBeChanged
CategorizedSortProxyModel-->>AppListView: layoutChanged
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 2 issues, and left some high level feedback:
- In AppList.qml,
isFreeSortis both bound toCategorizedSortProxyModel.categoryTypeand then imperatively assigned inswitchToFreeSort, which breaks the binding and risks state desynchronization; consider either derivingisFreeSortpurely fromcategoryTypeor storing it only as local state without a binding, but not both. - In
CategorizedSortProxyModel::setCategoryType, callingsetDynamicSortFilter(false); ... setDynamicSortFilter(true);unconditionally may override any pre-existing dynamic sort setting; to avoid changing external behaviour, capture the previousdynamicSortFilter()value and restore it instead of hardcodingtrue.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In AppList.qml, `isFreeSort` is both bound to `CategorizedSortProxyModel.categoryType` and then imperatively assigned in `switchToFreeSort`, which breaks the binding and risks state desynchronization; consider either deriving `isFreeSort` purely from `categoryType` or storing it only as local state without a binding, but not both.
- In `CategorizedSortProxyModel::setCategoryType`, calling `setDynamicSortFilter(false); ... setDynamicSortFilter(true);` unconditionally may override any pre-existing dynamic sort setting; to avoid changing external behaviour, capture the previous `dynamicSortFilter()` value and restore it instead of hardcoding `true`.
## Individual Comments
### Comment 1
<location path="qml/windowed/AppList.qml" line_range="21-30" />
<code_context>
+ property bool isFreeSort: CategorizedSortProxyModel.categoryType === CategorizedSortProxyModel.FreeCategory
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid breaking the binding on `isFreeSort` by assigning to it directly.
In QML, assigning to this bound property in `switchToFreeSort` will break its binding; after the first assignment it will no longer follow `CategorizedSortProxyModel.categoryType`, risking model/view desync. Either keep it fully derived from `categoryType` and drive switching from that, or make `isFreeSort` an independent `property bool` and update the proxy model explicitly so ownership is clear.
</issue_to_address>
### Comment 2
<location path="src/models/categorizedsortproxymodel.cpp" line_range="22-28" />
<code_context>
-
- beginResetModel();
+
+ // Disable dynamic sort to prevent setSortRole from triggering a redundant sort.
+ // We'll do a single sort below via setDynamicSortFilter(true), which uses
+ // layoutAboutToBeChanged/layoutChanged instead of modelReset, preserving delegates.
</code_context>
<issue_to_address>
**issue (bug_risk):** Restore the previous `dynamicSortFilter` state instead of forcing it to `true`.
This change always toggles `dynamicSortFilter` off then on, which means a proxy that was intentionally configured with dynamic sorting disabled will now have it permanently enabled. Instead, cache the existing value (e.g. `const bool oldDynamic = dynamicSortFilter();`), temporarily disable it, and restore `oldDynamic` after updating the category. If `oldDynamic` was false, you may need to call `sort()` once to maintain the new behavior without altering the proxy’s long-term configuration.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1. Replace Loader with dual-view visible switching in AppList.qml to avoid costly component destruction/creation on each sort mode toggle 2. Both AppListView and FreeSortListView are now always instantiated; switching is done via the visible property, reducing switch time from ~120ms to ~2ms 3. Replace beginResetModel/endResetModel with setDynamicSortFilter toggle in CategorizedSortProxyModel::setCategoryType to avoid redundant sort triggered by setSortRole 4. The new approach uses layoutAboutToBeChanged/layoutChanged (emitted internally by d->sort() via setDynamicSortFilter(true)) instead of modelReset, so existing delegates are moved rather than destroyed and recreated 5. Extra memory cost is ~60-80KB for retaining hidden view delegates, which is negligible (< 0.03% of process RSS) Log: Optimized windowed launcher sort mode switch from ~1000ms to ~18ms by replacing Loader with visible switching and eliminating redundant model resets Influence: 1. Test switching between all three sort modes (free sort, by category, by name) and verify correct display 2. Verify section headings update correctly when switching between alphabetary and DDE category modes 3. Check that scroll position resets to top after each sort switch 4. Test drag-and-drop reordering still works in free sort mode 5. Verify context menu (right-click) works correctly in both views 6. Test rapid back-and-forth switching to check for memory leaks 7. Verify keyboard navigation (Tab, arrow keys, Enter) works in both views fix(窗口模式): 优化排序模式切换性能 1. 在 AppList.qml 中用双视图 visible 切换替代 Loader，避免每次切换 排序模式时销毁和创建组件的开销 2. AppListView 和 FreeSortListView 现在始终实例化，通过 visible 属性 切换显示，切换时间从 ~120ms 降至 ~2ms 3. 在 CategorizedSortProxyModel::setCategoryType 中用 setDynamicSortFilter 开关替代 beginResetModel/endResetModel，避免 setSortRole 触发的冗余排序 4. 新方案使用 layoutAboutToBeChanged/layoutChanged（由 setDynamicSortFilter(true) 触发的 d->sort() 内部发出）替代 modelReset，使视图移动现有 delegate 而非销毁后重建 5. 保留隐藏视图 delegate 的额外内存开销约 60-80KB，可忽略不计 （不到进程 RSS 的 0.03%） Log: 通过用 visible 切换替代 Loader 并消除冗余模型重置，将窗口模式 启动器排序模式切换时间从 ~1000ms 优化至 ~18ms Influence: 1. 测试三种排序模式（自由排序、按分类、按名称）之间的切换，验证 显示是否正确 2. 验证在首字母排序和 DDE 分类模式之间切换时分节标题是否正确更新 3. 检查每次排序切换后滚动位置是否重置到顶部 4. 测试自由排序模式下拖拽排序功能是否正常 5. 验证两个视图的右键上下文菜单是否正常工作 6. 测试快速来回切换以检查是否存在内存泄漏 7. 验证两个视图中的键盘导航（Tab、方向键、回车）是否正常 PMS: BUG-367967
Ivy233
force-pushed
the
fix/optimize-sort-switch-perf
branch
from
d58ea56 to
c78989b
Compare
Contributor
Author
|
@sourcery-ai review |
deepin pr auto review★ 总体评分：100分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // AppList.qml
// 无需修改，代码已达到较优状态
// WindowedFrame.qml
// 建议确认 sideBar.switchToFreeSort 信号是否已废弃
// 如果已废弃，当前移除操作正确；如果未废弃，需确保 categoryType 属性能被正确更新以触发 isFreeSort 变化 |
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
3 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: Optimized windowed launcher sort mode switch from ~1000ms to ~18ms by replacing Loader with visible switching and eliminating redundant model resets
Influence:
fix(窗口模式): 优化排序模式切换性能
modelReset，使视图移动现有 delegate 而非销毁后重建
Log: 通过用 visible 切换替代 Loader 并消除冗余模型重置，将窗口模式
启动器排序模式切换时间从 ~1000ms 优化至 ~18ms
Influence:
PMS: BUG-367967
Summary by Sourcery
Optimize windowed launcher sort mode switching by keeping both list views instantiated and avoiding full model resets when changing category type.
Enhancements: