fix: emit IconNameRole change when WinIconRole changes #1426
+14
−0
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
Reviewer's guide (collapsed on small PRs)Reviewer's GuideConnects DockCombineModel to its source model's dataChanged signal and adds a slot that re-emits dataChanged for IconNameRole whenever WinIconRole changes without an accompanying IconNameRole update, ensuring taskbar icon updates for windows using window icons as app icons. Sequence diagram for propagating WinIconRole changes to IconNameRolesequenceDiagram
participant SourceModel
participant DockCombineModel
participant TaskbarView
SourceModel->>SourceModel: dataChanged(topLeft, bottomRight, [WinIconRole])
SourceModel-->>DockCombineModel: dataChanged(topLeft, bottomRight, [WinIconRole])
activate DockCombineModel
DockCombineModel->>DockCombineModel: onSourceDataChanged(topLeft, bottomRight, roles)
alt roles contains WinIconRole and not IconNameRole
DockCombineModel-->>DockCombineModel: dataChanged(topLeft, bottomRight, [IconNameRole])
DockCombineModel-->>TaskbarView: dataChanged(topLeft, bottomRight, [IconNameRole])
TaskbarView->>TaskbarView: updateTaskbarIcon()
else roles already contain IconNameRole
DockCombineModel-->>TaskbarView: dataChanged(topLeft, bottomRight, roles)
end
deactivate DockCombineModel
Updated class diagram for DockCombineModel signal handlingclassDiagram
class QAbstractItemModel {
<<abstract>>
+dataChanged(topLeft QModelIndex, bottomRight QModelIndex, roles QList_int)
}
class RoleCombineModel {
}
class AbstractTaskManagerInterface {
}
class DockCombineModel {
+DockCombineModel(major QAbstractItemModel, minor QAbstractItemModel, parent QObject)
+data(index QModelIndex, role int) QVariant
+roleNames() QHash_int_QByteArray
+onSourceDataChanged(topLeft QModelIndex, bottomRight QModelIndex, roles QList_int)
-m_roleMaps QHash_int_int
}
QAbstractItemModel <|-- RoleCombineModel
RoleCombineModel <|-- DockCombineModel
AbstractTaskManagerInterface <|-- DockCombineModel
QAbstractItemModel "1" o-- "1" DockCombineModel : sourceModel
DockCombineModel ..> QAbstractItemModel : connects dataChanged to onSourceDataChanged
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 onSourceDataChanged you emit dataChanged with TaskManager::IconNameRole, but since DockCombineModel remaps roles via m_roleMaps, the emitted role should likely be the proxy/model role (e.g. roleNames().key(MODEL_…)) rather than the source TaskManager role, otherwise views may not pick up the change correctly.
- QAbstractItemModel::dataChanged can be emitted with an empty roles list to mean "all roles changed"; consider handling the case where roles is empty but the icon has effectively changed, so that IconNameRole notifications are also propagated in that scenario.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In onSourceDataChanged you emit dataChanged with TaskManager::IconNameRole, but since DockCombineModel remaps roles via m_roleMaps, the emitted role should likely be the proxy/model role (e.g. roleNames().key(MODEL_…)) rather than the source TaskManager role, otherwise views may not pick up the change correctly.
- QAbstractItemModel::dataChanged can be emitted with an empty roles list to mean "all roles changed"; consider handling the case where roles is empty but the icon has effectively changed, so that IconNameRole notifications are also propagated in that scenario.
## Individual Comments
### Comment 1
<location> `panels/dock/taskmanager/dockcombinemodel.cpp:78-84` </location>
<code_context>
return QVariant();
}
+
+void DockCombineModel::onSourceDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight, const QList<int> &roles)
+{
+ // 部分窗口没有desktop, 应用图标使用的是窗口图标，当窗口图标变化的时候也需要发送应用图标变化的通知，以便前端更新
+ if (roles.contains(TaskManager::WinIconRole) && !roles.contains(TaskManager::IconNameRole)) {
+ Q_EMIT dataChanged(topLeft, bottomRight, {TaskManager::IconNameRole});
+ }
</code_context>
<issue_to_address>
**suggestion:** The roles list may be empty to indicate all roles changed, which is not handled here.
Qt’s `dataChanged` may be emitted with an empty `roles` list to indicate that any role might have changed. In that case, this condition will skip emitting the synthesized `IconNameRole` change even if the window icon changed. Consider treating an empty list as “all roles” and emitting `IconNameRole` when `roles.isEmpty()` or `roles.contains(TaskManager::WinIconRole)`, for example:
```cpp
if ((roles.isEmpty() || roles.contains(TaskManager::WinIconRole)) &&
!roles.contains(TaskManager::IconNameRole)) {
Q_EMIT dataChanged(topLeft, bottomRight, { TaskManager::IconNameRole });
}
```
```suggestion
void DockCombineModel::onSourceDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight, const QList<int> &roles)
{
// 部分窗口没有desktop, 应用图标使用的是窗口图标，当窗口图标变化的时候也需要发送应用图标变化的通知，以便前端更新
if ((roles.isEmpty() || roles.contains(TaskManager::WinIconRole)) &&
!roles.contains(TaskManager::IconNameRole)) {
Q_EMIT dataChanged(topLeft, bottomRight, { TaskManager::IconNameRole });
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Added connection to source model's dataChanged signal in DockCombineModel constructor to handle icon updates properly. When window icon changes (WinIconRole) but application icon name (IconNameRole) is not included in the change notification, the model now emits dataChanged signal for IconNameRole as well. This ensures that UI components relying on IconNameRole updates are properly refreshed when window icons change, particularly for windows without desktop files that use window icons as application icons. Log: Fixed taskbar icon updates when window icons change fix: 当窗口图标变化时发出 IconNameRole 变更通知 在 DockCombineModel 构造函数中添加了对源模型 dataChanged 信号的连接， 以正确处理图标更新。当窗口图标（WinIconRole）发生变化但应用图标名称 （IconNameRole）未包含在变更通知中时，模型现在会额外为 IconNameRole 发 出 dataChanged 信号。这确保了依赖 IconNameRole 更新的 UI 组件在窗口图标 变化时能正确刷新，特别是对于那些没有桌面文件而使用窗口图标作为应用图标的 窗口。 Log: 修复窗口图标变化时任务栏图标更新问题 PMS: BUG-321407
Contributor
Author
|
也可以在window icon 变化的时候同时发出iconname changed的信号 |
deepin pr auto review这段代码的目的是在 以下是对这段代码的详细审查意见，包括语法逻辑、代码质量、代码性能和代码安全方面的改进建议： 1. 语法逻辑与代码质量优点：
改进意见：
2. 代码性能潜在问题：
3. 代码安全改进意见：
4. 修改后的代码建议结合以上分析，以下是优化后的代码片段建议： // dockcombinemodel.h
// ...
private slots:
// 添加 override 关键字（如果继承自接口）或保持现状
void onSourceDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight, const QList<int> &roles);
// ...
// dockcombinemodel.cpp
// ...
// 建议在 setSourceModel 中连接，或者在构造函数中确认 sourceModel() 不为空
// 如果必须在构造函数中连接，建议增加断言或检查
DockCombineModel::DockCombineModel(QAbstractItemModel *major, QAbstractItemModel *minor)
: RoleCombineModel(major, minor)
{
// ... 初始化代码 ...
if (sourceModel()) {
connect(sourceModel(), &QAbstractItemModel::dataChanged, this, &DockCombineModel::onSourceDataChanged);
}
}
// ...
void DockCombineModel::onSourceDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight, const QList<int> &roles)
{
// 增加索引有效性检查
if (!topLeft.isValid() || !bottomRight.isValid()) {
return;
}
// 部分窗口没有desktop, 应用图标使用的是窗口图标，当窗口图标变化的时候也需要发送应用图标变化的通知，以便前端更新
// 逻辑：如果角色列表为空（表示所有角色可能都变了） 或者 包含了 WinIconRole
// 并且 角色列表中不包含 IconNameRole（防止循环或重复通知）
if ((roles.isEmpty() || roles.contains(TaskManager::WinIconRole)) &&
!roles.contains(TaskManager::IconNameRole)) {
// 注意：如果 RoleCombineModel 是代理模型，通常需要确保发射的索引是当前模型的索引。
// 如果父类已经处理了映射，直接传递 topLeft/bottomRight 是可以的。
// 如果父类没有自动映射，且 sourceModel 的索引直接用于当前 model，则需确保逻辑一致。
// 这里假设直接传递是符合模型架构的。
Q_EMIT dataChanged(topLeft, bottomRight, { TaskManager::IconNameRole });
}
}总结这段代码逻辑上是正确的，解决了窗口图标变化同步到应用图标的需求。主要的改进点在于：
|
18202781743
approved these changes
Feb 3, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy 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 |
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.
Added connection to source model's dataChanged signal in DockCombineModel constructor to handle icon updates properly. When window icon changes (WinIconRole) but application icon name (IconNameRole) is not included in the change notification, the model now emits dataChanged signal for IconNameRole as well. This ensures that UI components relying on IconNameRole updates are properly refreshed when window icons change, particularly for windows without desktop files that use window icons as application icons.
Log: Fixed taskbar icon updates when window icons change
fix: 当窗口图标变化时发出 IconNameRole 变更通知
在 DockCombineModel 构造函数中添加了对源模型 dataChanged 信号的连接， 以正确处理图标更新。当窗口图标（WinIconRole）发生变化但应用图标名称
（IconNameRole）未包含在变更通知中时，模型现在会额外为 IconNameRole 发
出 dataChanged 信号。这确保了依赖 IconNameRole 更新的 UI 组件在窗口图标 变化时能正确刷新，特别是对于那些没有桌面文件而使用窗口图标作为应用图标的
窗口。
Log: 修复窗口图标变化时任务栏图标更新问题
PMS: BUG-321407
Summary by Sourcery
Bug Fixes: