-
Notifications
You must be signed in to change notification settings - Fork 55
chore: backup plan to temporary switch back to legacy ItemModel #1330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
为 ItemModel 类添加了一些兼容目前接口行为的接口,以供临时切换任务栏 模型到 ItemModel 所需. 此 PR 中未进行实际的切换. Log:
Reviewer's GuideEnhances ItemModel with a set of request* methods for legacy taskbar model compatibility, standardizes role names using TaskManager constants, refactors the Roles enum, and updates TaskManager to delegate interactions through the new ItemModel interface—preparing for a temporary switch back to the legacy model without performing it. Sequence diagram for TaskManager delegating requests to ItemModelsequenceDiagram
participant TM as TaskManager
participant IM as ItemModel
TM->>IM: requestActivate(index)
TM->>IM: requestOpenUrls(index, urls)
TM->>IM: requestNewInstance(index, action)
TM->>IM: requestClose(index, force)
TM->>IM: requestUpdateWindowIconGeometry(index, geometry, delegate)
TM->>IM: requestWindowsView(indexes)
TM->>IM: requestOpenUrls(index, urls) (from dropFilesOnItem)
Class diagram for updated ItemModel and TaskManager interfacesclassDiagram
class ItemModel {
+QHash<int, QByteArray> roleNames() const
+int rowCount(const QModelIndex &parent) const
+QVariant data(const QModelIndex &index, int role) const
+QJsonArray dumpDockedItems() const
+void requestActivate(const QModelIndex &index) const
+void requestNewInstance(const QModelIndex &index, const QString &action) const
+void requestClose(const QModelIndex &index, bool force = false) const
+void requestOpenUrls(const QModelIndex &index, const QList<QUrl> &urls) const
+void requestWindowsView(const QModelIndexList &indexes) const
+void requestUpdateWindowIconGeometry(const QModelIndex &index, const QRect &geometry, QObject *delegate = nullptr) const
+QPointer<AbstractItem> getItemById(const QString& id) const
-void onItemDestroyed()
-void onItemChanged()
}
class TaskManager {
+void requestActivate(const QModelIndex &index) const
+void requestOpenUrls(const QModelIndex &index, const QList<QUrl> &urls) const
+void requestNewInstance(const QModelIndex &index, const QString &action) const
+void requestClose(const QModelIndex &index, bool force) const
+void requestUpdateWindowIconGeometry(const QModelIndex &index, const QRect &geometry, QObject *delegate) const
+void requestWindowsView(const QModelIndexList &indexes) const
+void dropFilesOnItem(const QString& itemId, const QStringList& urls)
}
TaskManager --> ItemModel : delegates request* methods
Class diagram for updated ItemModel::Roles enumclassDiagram
class ItemModel {
ItemIdRole : int
DockedDirRole : int
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review我来对这个代码变更进行审查:
auto baseItem = ItemModel::instance()->getItemById(itemId);
if (!baseItem) return;
auto appItem = qobject_cast<AppItem*>(baseItem.get());
if (!appItem) return;
QPointer<AbstractItem> ItemModel::getValidItem(const QModelIndex &index) const {
QString itemId = data(index).toString();
return getItemById(itemId);
}
总的来说,这次改动提高了代码的组织性和可维护性,但还有一些细节可以进一步优化。建议在合并前进行充分的测试,特别是涉及到类型转换和空指针检查的部分。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `panels/dock/taskmanager/itemmodel.cpp:101` </location>
<code_context>
+void ItemModel::requestActivate(const QModelIndex &index) const
+{
+ QString itemId = data(index).toString();
+
+ auto item = ItemModel::instance()->getItemById(itemId);
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential mismatch between index data and itemId retrieval.
Explicitly request ItemIdRole when retrieving data to ensure correct itemId lookup and prevent potential mismatches.
</issue_to_address>
### Comment 2
<location> `panels/dock/taskmanager/itemmodel.cpp:120` </location>
<code_context>
+ return;
+ }
+
+ item->handleClick(DOCK_ACTIN_LAUNCH);
+}
+
</code_context>
<issue_to_address>
**issue (typo):** Typo in constant name: DOCK_ACTIN_LAUNCH.
Please correct the constant name to DOCK_ACTION_LAUNCH to avoid potential runtime errors.
```suggestion
item->handleClick(DOCK_ACTION_LAUNCH);
```
</issue_to_address>
### Comment 3
<location> `panels/dock/taskmanager/itemmodel.cpp:162` </location>
<code_context>
+{
+ QString itemId = data(index).toString();
+
+ QPointer<AppItem> item = static_cast<AppItem *>(ItemModel::instance()->getItemById(itemId).get());
+ if (item.isNull())
+ return;
</code_context>
<issue_to_address>
**issue (bug_risk):** Unsafe static_cast from AbstractItem to AppItem.
static_cast assumes getItemById always returns an AppItem, which may not be guaranteed. Use dynamic_cast and handle failed casts to avoid undefined behavior.
</issue_to_address>
### Comment 4
<location> `panels/dock/taskmanager/itemmodel.cpp:99` </location>
<code_context>
return result;
}
+void ItemModel::requestActivate(const QModelIndex &index) const
+{
+ QString itemId = data(index).toString();
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring repeated item lookup and null-check logic into a single helper template that invokes a lambda for each request method.
You can collapse all the boiler-plate into a single helper that does “id → lookup → null-check” and then invokes a lambda. For example, add this private template in `ItemModel`:
```cpp
template<typename Func>
void withItem(const QModelIndex &idx, Func&& func) const {
// directly pull itemId from the index
const QString id = idx.data(ItemModel::ItemIdRole).toString();
if (auto item = ItemModel::instance()->getItemById(id))
func(item);
}
```
Then each request method becomes just a call into `withItem()`:
```cpp
void ItemModel::requestActivate(const QModelIndex &index) const {
withItem(index, [](auto item){
item->handleClick(QString{});
});
}
void ItemModel::requestClose(const QModelIndex &index, bool force) const {
withItem(index, [force](auto item){
item->handleClick(force ? DOCK_ACTION_FORCEQUIT : DOCK_ACTION_CLOSEALL);
});
}
```
For the URL drop you can still do conversion inside the lambda:
```cpp
void ItemModel::requestOpenUrls(const QModelIndex &index, const QList<QUrl> &urls) const {
withItem(index, [&](auto item){
QStringList list;
for (auto &u : urls) list << u.toString();
item->handleFileDrop(list);
});
}
```
This preserves every behavior but removes the six-times repeated “get id → find → if (!item) return” logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return; | ||
| } | ||
|
|
||
| item->handleClick(DOCK_ACTIN_LAUNCH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (typo): Typo in constant name: DOCK_ACTIN_LAUNCH.
Please correct the constant name to DOCK_ACTION_LAUNCH to avoid potential runtime errors.
| item->handleClick(DOCK_ACTIN_LAUNCH); | |
| item->handleClick(DOCK_ACTION_LAUNCH); |
|
TAG Bot New tag: 2.0.17 |
|
TAG Bot New tag: 2.0.18 |
|
TAG Bot New tag: 2.0.19 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, BLumia The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
为 ItemModel 类添加了一些兼容目前接口行为的接口,以供临时切换任务栏模型到 ItemModel 所需. (此 PR 中未进行实际的切换)
Summary by Sourcery
Introduce a compatibility layer in ItemModel for temporarily switching back to the legacy taskbar model by exposing request* action methods, update role definitions to use shared constants, and refactor TaskManager to forward item requests through its generic dataModel interface.
New Features:
Enhancements: