fix: incorrect window state for JLCPCB #1331
Merged
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
简单而言,对于程序A,打开a1,打开a2,关闭a1后会观察到任务栏指示此图标无 窗口.原因是 DockGroupModel 的 rowsRemoved 发了 dataChanged，会触发 all(), 然后因为其是继承的 RoleGroupModel，它的rowsRemoved 会被它本 体和 DockGroupModel 都接到，但 DockGroupModel 比 RoleGroupModel先收 到了信号,导致了时序问题,使后续 all() 调用时其中获取的数据已经不再正 确. 此处通过使相关信号排队处理的方式,使 DockGroupModel 的事件响应晚于 RoleGroupModel 的移除事件响应. PMS: BUG-338573 Log:
Reviewer's GuideSwitches DockGroupModel's rowsInserted, rowsRemoved, and dataChanged signal-slot connections to use queued connections, ensuring DockGroupModel processes removals after RoleGroupModel and thus fixing incorrect window state updates. Sequence diagram for queued signal processing in DockGroupModelsequenceDiagram
participant RoleGroupModel
participant DockGroupModel
participant QtEventLoop
Note over RoleGroupModel, DockGroupModel: rowsRemoved signal emitted
RoleGroupModel->RoleGroupModel: Handle rowsRemoved synchronously
RoleGroupModel->QtEventLoop: Emit rowsRemoved signal
QtEventLoop->DockGroupModel: Deliver rowsRemoved signal (queued)
DockGroupModel->DockGroupModel: Handle rowsRemoved after RoleGroupModel
DockGroupModel->DockGroupModel: Update m_currentActiveWindow
DockGroupModel->DockGroupModel: Emit dataChanged
Class diagram for updated DockGroupModel signal connectionsclassDiagram
class DockGroupModel {
+DockGroupModel(QAbstractItemModel *sourceModel, int role, QObject *parent)
+data(const QModelIndex &index, int role) const
-m_roleForDeduplication
-m_currentActiveWindow
+resetActiveWindow(parentRow)
<<signal>> rowsInserted (queued)
<<signal>> rowsRemoved (queued)
<<signal>> dataChanged (queued)
}
DockGroupModel --|> RoleGroupModel
DockGroupModel --|> AbstractTaskManagerInterface
class RoleGroupModel {
+rowCount(const QModelIndex &parent) const
}
class AbstractTaskManagerInterface {
}
DockGroupModel o-- RoleGroupModel
DockGroupModel o-- AbstractTaskManagerInterface
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review这个代码变更主要是在连接信号槽时添加了 Qt::QueuedConnection 连接类型。我来分析一下这个变更：
总的来说，这是一个很好的改进，提高了代码的稳定性和可维护性，同时保持了良好的性能特征。 |
![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:
- Add a brief code comment above the queued connections explaining the signal ordering issue and why Qt::QueuedConnection is needed.
- Extract the duplicated lambda logic for rowsInserted, rowsRemoved, and dataChanged into helper methods to reduce duplication and improve maintainability.
- Review potential side effects of queuing all dataChanged and row‐change signals; ensure this deferred delivery won’t introduce latency or ordering issues elsewhere.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add a brief code comment above the queued connections explaining the signal ordering issue and why Qt::QueuedConnection is needed.
- Extract the duplicated lambda logic for rowsInserted, rowsRemoved, and dataChanged into helper methods to reduce duplication and improve maintainability.
- Review potential side effects of queuing all dataChanged and row‐change signals; ensure this deferred delivery won’t introduce latency or ordering issues elsewhere.
## Individual Comments
### Comment 1
<location> `panels/dock/taskmanager/dockgroupmodel.cpp:26` </location>
<code_context>
- Q_UNUSED(last)
- if (!parent.isValid())
- return;
- Q_EMIT dataChanged(index(parent.row(), 0), index(parent.row(), 0), {TaskManager::WindowsRole});
- });
- connect(this, &QAbstractItemModel::rowsRemoved, this, [this](const QModelIndex &parent, int first, int last) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Emitting dataChanged for only the first column may miss updates for other columns.
If your model has multiple columns, limiting dataChanged to column 0 may cause the UI to miss updates for other columns. Please confirm whether the model is single-column or update the signal to cover all relevant columns.
Suggested implementation:
```cpp
int colCount = columnCount(parent);
if (colCount > 0) {
Q_EMIT dataChanged(index(parent.row(), 0), index(parent.row(), colCount - 1), {TaskManager::WindowsRole});
}
```
If `columnCount()` is not accessible in this context, you may need to use `this->columnCount(parent)` or ensure you have a valid model pointer. Also, if the model is guaranteed to be single-column, you can leave the code as-is, but this change will ensure all columns are updated if there are multiple.
</issue_to_address>
### Comment 2
<location> `panels/dock/taskmanager/dockgroupmodel.cpp:55` </location>
<code_context>
-
- connect(this, &QAbstractItemModel::dataChanged, this, [this](const QModelIndex &topLeft, const QModelIndex &bottomRight, const QList<int> &roles) {
- Q_UNUSED(bottomRight)
- if (!topLeft.parent().isValid())
- return;
- auto parentRow = topLeft.parent().row();
</code_context>
<issue_to_address>
**question (bug_risk):** Guard clause may skip legitimate updates for top-level items.
This guard prevents updates to top-level items. If updates to these items are required, consider revising this condition.
</issue_to_address>
### Comment 3
<location> `panels/dock/taskmanager/dockgroupmodel.cpp:21` </location>
<code_context>
- // Current active window is after the removed range, shift it back
- int removedCount = last - first + 1;
- m_currentActiveWindow[parentRow] = currentActive - removedCount;
+ connect(
+ this,
+ &QAbstractItemModel::rowsInserted,
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a queuedConnect helper to eliminate repetitive connect boilerplate and simplify the constructor.
Here’s one way to collapse the repeated boiler-plate while keeping all behavior intact:
1) Add a small `queuedConnect()` helper in your `.cpp` (above your ctor):
```cpp
namespace {
template<typename Sender, typename Signal, typename Receiver, typename Slot>
QMetaObject::Connection queuedConnect(Sender* s,
Signal sig,
Receiver* r,
Slot slot)
{
return QObject::connect(s, sig, r, slot, Qt::QueuedConnection);
}
}
```
2) Replace each `connect(..., Qt::QueuedConnection)` with:
```cpp
queuedConnect(this, &QAbstractItemModel::rowsInserted, this,
[this](const QModelIndex &parent, int /*first*/, int /*last*/) {
if (!parent.isValid()) return;
Q_EMIT dataChanged(
index(parent.row(), 0),
index(parent.row(), 0),
{ TaskManager::WindowsRole }
);
}
);
queuedConnect(this, &QAbstractItemModel::rowsRemoved, this,
[this](const QModelIndex &parent, int first, int last) {
if (!parent.isValid()) return;
const int row = parent.row();
if (m_currentActiveWindow.contains(row)) {
int cur = m_currentActiveWindow[row];
if (cur >= first && cur <= last)
resetActiveWindow(row);
else if (cur > last)
m_currentActiveWindow[row] = cur - (last - first + 1);
}
Q_EMIT dataChanged(
index(row, 0),
index(row, 0),
{ TaskManager::WindowsRole }
);
}
);
queuedConnect(this, &QAbstractItemModel::dataChanged, this,
[this](const QModelIndex &topLeft, const QModelIndex&, const QList<int> &roles){
auto p = topLeft.parent();
if (!p.isValid()) return;
int row = p.row();
Q_EMIT dataChanged(index(row, 0), index(row, 0), roles);
if (roles.contains(TaskManager::ActiveRole))
m_currentActiveWindow.insert(row, topLeft.row());
}
);
```
This:
- Removes the repeated `, Qt::QueuedConnection` overload on each call
- Keeps each lambda exactly as-is
- Makes your constructor much shorter and easier to scan
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| &QAbstractItemModel::rowsInserted, | ||
| this, | ||
| [this](const QModelIndex &parent, int first, int last) { | ||
| Q_UNUSED(first) |
There was a problem hiding this comment.
suggestion (bug_risk): Emitting dataChanged for only the first column may miss updates for other columns.
If your model has multiple columns, limiting dataChanged to column 0 may cause the UI to miss updates for other columns. Please confirm whether the model is single-column or update the signal to cover all relevant columns.
Suggested implementation:
int colCount = columnCount(parent);
if (colCount > 0) {
Q_EMIT dataChanged(index(parent.row(), 0), index(parent.row(), colCount - 1), {TaskManager::WindowsRole});
}
If columnCount() is not accessible in this context, you may need to use this->columnCount(parent) or ensure you have a valid model pointer. Also, if the model is guaranteed to be single-column, you can leave the code as-is, but this change will ensure all columns are updated if there are multiple.
| // Current active window is after the removed range, shift it back | ||
| int removedCount = last - first + 1; | ||
| m_currentActiveWindow[parentRow] = currentActive - removedCount; | ||
| } |
There was a problem hiding this comment.
question (bug_risk): Guard clause may skip legitimate updates for top-level items.
This guard prevents updates to top-level items. If updates to these items are required, consider revising this condition.
| // Current active window is after the removed range, shift it back | ||
| int removedCount = last - first + 1; | ||
| m_currentActiveWindow[parentRow] = currentActive - removedCount; | ||
| connect( |
There was a problem hiding this comment.
issue (complexity): Consider introducing a queuedConnect helper to eliminate repetitive connect boilerplate and simplify the constructor.
Here’s one way to collapse the repeated boiler-plate while keeping all behavior intact:
- Add a small
queuedConnect()helper in your.cpp(above your ctor):
namespace {
template<typename Sender, typename Signal, typename Receiver, typename Slot>
QMetaObject::Connection queuedConnect(Sender* s,
Signal sig,
Receiver* r,
Slot slot)
{
return QObject::connect(s, sig, r, slot, Qt::QueuedConnection);
}
}- Replace each
connect(..., Qt::QueuedConnection)with:
queuedConnect(this, &QAbstractItemModel::rowsInserted, this,
[this](const QModelIndex &parent, int /*first*/, int /*last*/) {
if (!parent.isValid()) return;
Q_EMIT dataChanged(
index(parent.row(), 0),
index(parent.row(), 0),
{ TaskManager::WindowsRole }
);
}
);
queuedConnect(this, &QAbstractItemModel::rowsRemoved, this,
[this](const QModelIndex &parent, int first, int last) {
if (!parent.isValid()) return;
const int row = parent.row();
if (m_currentActiveWindow.contains(row)) {
int cur = m_currentActiveWindow[row];
if (cur >= first && cur <= last)
resetActiveWindow(row);
else if (cur > last)
m_currentActiveWindow[row] = cur - (last - first + 1);
}
Q_EMIT dataChanged(
index(row, 0),
index(row, 0),
{ TaskManager::WindowsRole }
);
}
);
queuedConnect(this, &QAbstractItemModel::dataChanged, this,
[this](const QModelIndex &topLeft, const QModelIndex&, const QList<int> &roles){
auto p = topLeft.parent();
if (!p.isValid()) return;
int row = p.row();
Q_EMIT dataChanged(index(row, 0), index(row, 0), roles);
if (roles.contains(TaskManager::ActiveRole))
m_currentActiveWindow.insert(row, topLeft.row());
}
);This:
- Removes the repeated
, Qt::QueuedConnectionoverload on each call - Keeps each lambda exactly as-is
- Makes your constructor much shorter and easier to scan
tsic404
approved these changes
Nov 4, 2025
18202781743
approved these changes
Nov 4, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, BLumia, tsic404 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
4 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.
简单而言,对于程序A,打开a1,打开a2,关闭a1后会观察到任务栏指示此图标无窗口.原因是 DockGroupModel 的 rowsRemoved 发了 dataChanged，会触发 all(), 然后因为其是继承的 RoleGroupModel，它的rowsRemoved 会被它本 体和 DockGroupModel 都接到，但 DockGroupModel 比 RoleGroupModel先收 到了信号,导致了时序问题,使后续 all() 调用时其中获取的数据已经不再正确.
此处通过使相关信号排队处理的方式,使 DockGroupModel 的事件响应晚于 RoleGroupModel 的移除事件响应.
Summary by Sourcery
Fix incorrect window state updates in DockGroupModel by deferring its signal handling to avoid ordering conflicts with its base class.
Bug Fixes:
Enhancements: