fix: improve window icon handling and model reset logic#1578
Open
wjyrich wants to merge 1 commit intolinuxdeepin:masterfrom
Open
fix: improve window icon handling and model reset logic#1578wjyrich wants to merge 1 commit intolinuxdeepin:masterfrom
wjyrich wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
|
[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 guide (collapsed on small PRs)Reviewer's GuideRefactors dock task manager models and related window/icon handling to use a full model reset on source changes, respect window-split settings for choosing window vs. app icons, optimize QML icon loading for active windows, and immediately refresh X11 window icons upon mapping. Sequence diagram for X11 window mapping and immediate icon updatesequenceDiagram
participant XServer
participant X11WindowMonitor
participant Window
participant AbstractWindowMonitor
XServer->>X11WindowMonitor: MapNotify(xcb_window)
activate X11WindowMonitor
X11WindowMonitor->>X11WindowMonitor: onWindowMapped(xcb_window)
X11WindowMonitor->>X11WindowMonitor: xcb_change_window_attributes
X11WindowMonitor->>Window: trackWindow(window)
X11WindowMonitor->>Window: updateIcon()
X11WindowMonitor->>AbstractWindowMonitor: windowAdded(window)
deactivate X11WindowMonitor
Class diagram for updated dock task manager models and window/icon handlingclassDiagram
class DockItemModel {
+bool m_isUpdating
+void setSourceModel(QAbstractItemModel *model)
}
class QAbstractProxyModel {
+void setSourceModel(QAbstractItemModel *model)
+int rowCount()
}
DockItemModel --|> QAbstractProxyModel
class DockCombineModel {
+QVariant data(const QModelIndex &index, int role) const
-QHash<int,int> m_roleMaps
+DockCombineModel(QAbstractItemModel *major, QAbstractItemModel *minor, int majorRoles, CombineFunc func, QObject *parent)
}
class RoleCombineModel {
+QVariant data(const QModelIndex &index, int role) const
}
DockCombineModel --|> RoleCombineModel
class TaskManager {
<<enumeration>>
+int IconNameRole
+int WinIconRole
}
DockCombineModel ..> TaskManager : uses roles
class TaskManagerSettings {
+static TaskManagerSettings* instance()
+bool isWindowSplit() const
}
DockCombineModel ..> TaskManagerSettings : queries isWindowSplit
class X11WindowMonitor {
+void onWindowMapped(xcb_window_t xcb_window)
+void trackWindow(Window *window)
}
class AbstractWindowMonitor {
+void windowAdded(QPointer<AbstractWindow> window)
}
X11WindowMonitor --|> AbstractWindowMonitor
class Window {
+void updateIcon()
}
X11WindowMonitor ..> Window : trackWindow
class AppItem {
+bool titleActive
+Image iconImage
}
class Image {
+bool retainWhileLoading
}
AppItem *-- Image : contains iconImage
Flow diagram for DockCombineModel icon selection logicflowchart TD
A(Start) --> B[Request data for IconNameRole]
B --> C[Read winIcon from WinIconRole]
C --> D{isWindowSplit and winIcon not empty}
D -- Yes --> E[Return winIcon]
D -- No --> F[Read icon from IconNameRole]
F --> G{icon empty}
G -- Yes --> H[Set icon to winIcon]
G -- No --> I[Keep app icon]
H --> J[Return icon]
I --> J[Return icon]
E --> K(End)
J --> K(End)
Flow diagram for DockItemModel source model reset logicflowchart TD
A(Start setSourceModel) --> B{Existing sourceModel}
B -- Yes --> C[Disconnect old source model signals]
B -- No --> D[Skip disconnect]
C --> E
D --> E
E[beginResetModel] --> F["QAbstractProxyModel::setSourceModel new model"]
F --> G[endResetModel]
G --> H[Connect rowsInserted, rowsRemoved, rowsMoved, dataChanged signals]
H --> I[Set m_isUpdating to false]
I --> J(End)
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:
- In DockItemModel::setSourceModel, consider skipping the beginResetModel/endResetModel path when the new source model is identical to the current one to avoid unnecessary full model resets and view churn.
- DockCombineModel::data now depends on TaskManagerSettings::isWindowSplit(), but the model does not emit any dataChanged signals when this setting toggles; consider listening to the setting change and triggering a suitable dataChanged/model reset so views update their icons immediately when the split mode changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In DockItemModel::setSourceModel, consider skipping the beginResetModel/endResetModel path when the new source model is identical to the current one to avoid unnecessary full model resets and view churn.
- DockCombineModel::data now depends on TaskManagerSettings::isWindowSplit(), but the model does not emit any dataChanged signals when this setting toggles; consider listening to the setting change and triggering a suitable dataChanged/model reset so views update their icons immediately when the split mode changes.
## Individual Comments
### Comment 1
<location path="panels/dock/taskmanager/x11windowmonitor.cpp" line_range="189" />
<code_context>
xcb_change_window_attributes(X11->getXcbConnection(), xcb_window, XCB_CW_EVENT_MASK, value_list);
trackWindow(window.get());
+
+ window->updateIcon();
Q_EMIT AbstractWindowMonitor::windowAdded(static_cast<QPointer<AbstractWindow>>(window.get()));
}
</code_context>
<issue_to_address>
**question (bug_risk):** Check for any ordering assumptions between `updateIcon()` and `windowAdded`.
Calling `updateIcon()` before emitting `windowAdded` changes the lifecycle: listeners now see a window whose icon may already be updated, and any signals from `updateIcon()` fire first. Please confirm that no existing `windowAdded` consumers assume they run before icon‑related updates; if they do, consider keeping `updateIcon()` after `windowAdded` or clearly documenting this new ordering.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| xcb_change_window_attributes(X11->getXcbConnection(), xcb_window, XCB_CW_EVENT_MASK, value_list); | ||
| trackWindow(window.get()); | ||
|
|
||
| window->updateIcon(); |
There was a problem hiding this comment.
question (bug_risk): Check for any ordering assumptions between updateIcon() and windowAdded.
Calling updateIcon() before emitting windowAdded changes the lifecycle: listeners now see a window whose icon may already be updated, and any signals from updateIcon() fire first. Please confirm that no existing windowAdded consumers assume they run before icon‑related updates; if they do, consider keeping updateIcon() after windowAdded or clearly documenting this new ordering.
|
TAG Bot New tag: 2.0.39 |
1. Add a new dconfig setting "windowIconWhitelist" with default value ["wechat"] 2. When window split mode is enabled, the dock now checks the whitelist to decide whether to use window icons instead of app icons for specific applications 3. Fix model reset in DockItemModel::setSourceModel to use beginResetModel/endResetModel instead of manual row insertion/removal for better model consistency 4. Update X11 window monitor to trigger icon update when a new window is mapped The window icon whitelist is needed for apps like WeChat that provide superior window icon quality compared to their app icons, enhancing the visual experience when window split is disabled. Log: Added window icon whitelist config for dock task manager Influence: 1. Verify WeChat and other whitelisted apps display correct icons when window split is disabled 2. Test default whitelist behavior with WeChat running 3. Check that non-whitelisted apps still use app icons when window split is disabled 4. Test the new dconfig setting can be modified and persists correctly 5. Ensure DockItemModel resets work correctly without data corruption 6. Verify X11 window monitor still functions properly with icon update call feat: 为任务栏任务管理器添加窗口图标白名单功能 1. 添加新的 dconfig 设置"windowIconWhitelist"，默认值为["wechat"] 2. 当窗口拆分模式启用时，任务栏会检查白名单，决定是否对特定应用使用窗口 图标而非应用图标 3. 修复 DockItemModel::setSourceModel 中的模型重置，改用 beginResetModel/endResetModel 替代手动行插入/删除，以提高模型一致性 4. 更新 X11 窗口监视器，在新窗口映射时触发图标更新 对于微信等提供比应用图标更优质窗口图标的应用，窗口图标白名单是必要的，可 在窗口拆分禁用时提升视觉体验。 Log: 为任务栏添加窗口图标白名单配置 Influence: 1. 验证在白名单中的应用（如微信）在禁用窗口拆分时显示正确图标 2. 使用微信测试默认白名单行为 3. 检查非白名单应用在禁用窗口拆分时仍使用应用图标 4. 测试新的 dconfig 设置可以被修改并持久保存 5. 确保 DockItemModel 重置操作正常，无数据损坏 6. 验证 X11 窗口监视器在添加图标更新调用后仍正常工作 PMS: TASK-389031
deepin pr auto review这份代码diff主要实现了在Dock任务栏中新增"窗口图标白名单"功能，允许在特定条件下（窗口合并关闭时）优先使用窗口图标而非应用图标。以下是对代码的详细审查意见： 1. 语法与逻辑审查dockcombinemodel.cpp
dockitemmodel.cpp
x11windowmonitor.cpp
2. 代码质量
3. 代码性能
4. 代码安全
5. 其他建议
总结代码整体实现了预期的功能，但在 |
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
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.
The changes fix icon display inconsistencies where window-specific icons were incorrectly overridden by application icons when window split is disabled. The model reset simplification fixes edge cases where incremental row updates could cause state corruption. The icon loading optimization improves performance by not retaining loading states for active windows.
Log: Fixed window icon display logic and model synchronization
Influence:
fix: 改进窗口图标处理和模型重置逻辑
这些修改修复了当窗口分裂功能禁用时，窗口特定图标被应用程序图标错误覆盖的
问题。模型重置的简化修复了增量行更新可能导致的内部状态损坏问题。图标加载
优化通过不在活动窗口中保留加载状态来提高性能。
Log: 修复了窗口图标显示逻辑和模型同步问题
Influence:
PMS: TASK-389031
Summary by Sourcery
Improve dock task manager icon handling and model synchronization for better window-specific icon display and stability.
Bug Fixes:
Enhancements: