fix: when drag just one app in folder to launchpad quickly will core…

[wjyrich]

…dump.

修复文件夹只有一个应用时，快速拖拽到启动器会崩溃问题。

原因为： 文件夹在被删除后，他的ItemsPage 对象仍然存活（QObject 子对象，未被删除），连接也未断开。如果后续有任何东西触发了这个 "幽灵" page 的 pageCountChanged，仍然会发出 folderPageCountChanged 信号指向一个已不存在的文件夹。

修复方案： 断开Itempage的连接后，再进行删除 folder。

Summary by Sourcery

Bug Fixes:

• Fix a crash that occurred when quickly dragging the only app out of a folder to the launcher by ensuring the folder’s items page is disconnected and cleaned up correctly.

[sourcery-ai]

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Ensures that when a folder is removed, its associated ItemsPage disconnects and is safely destroyed after all model references are cleaned up, preventing crashes triggered by lingering signal connections from a deleted folder.

Sequence diagram for updated removeFolder process

sequenceDiagram
    participant Caller
    participant ItemArrangementProxyModel
    participant ItemsPage
    participant TopLevelModel
    participant FolderModel

    Caller->>ItemArrangementProxyModel: removeFolder(idNumber)
    ItemArrangementProxyModel->>ItemArrangementProxyModel: build fullId
    ItemArrangementProxyModel->>ItemArrangementProxyModel: assert m_folders.contains(fullId)
    ItemArrangementProxyModel->>ItemsPage: page = m_folders.value(fullId)
    ItemArrangementProxyModel->>ItemsPage: disconnect(this)
    ItemArrangementProxyModel->>TopLevelModel: removeItem(fullId)
    ItemArrangementProxyModel->>FolderModel: findItems(fullId)
    FolderModel-->>ItemArrangementProxyModel: result
    ItemArrangementProxyModel->>FolderModel: removeRows(row, 1)
    ItemArrangementProxyModel->>ItemArrangementProxyModel: m_folders.remove(fullId)
    ItemArrangementProxyModel->>ItemsPage: deleteLater()
    ItemsPage-->>ItemArrangementProxyModel: scheduled for deferred deletion

Loading

Class diagram for ItemArrangementProxyModel folder and ItemsPage cleanup

classDiagram
    class ItemArrangementProxyModel {
        - QHash~QString, ItemsPage*~ m_folders
        - QStandardItemModel m_folderModel
        - TopLevelModel* m_topLevel
        + void removeFolder(QString idNumber)
    }

    class ItemsPage {
        <<QObject>>
        + void disconnect(QObject* sender)
        + void deleteLater()
    }

    class TopLevelModel {
        + void removeItem(QString fullId)
    }

    class QStandardItemModel {
        + QList~QStandardItem*~ findItems(QString text)
        + void removeRows(int row, int count)
    }

    ItemArrangementProxyModel "1" o--> "*" ItemsPage : owns_via_m_folders
    ItemArrangementProxyModel --> TopLevelModel : uses
    ItemArrangementProxyModel --> QStandardItemModel : uses

Loading

File-Level Changes

Change	Details	Files
Safely tear down a folder’s ItemsPage before removing the folder from models to avoid signals from a stale QObject.	Retrieve the ItemsPage pointer from the folder map before removal. Disconnect the ItemsPage from the ItemArrangementProxyModel to prevent further signal emissions to deleted folders. Remove the folder from the top-level item model and the folder model as before. Erase the folder entry from the internal folder map only after model cleanup. Schedule asynchronous deletion of the ItemsPage using deleteLater so it is destroyed safely after event processing.	src/models/itemarrangementproxymodel.cpp

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've left some high level feedback:

• Consider using auto *page = m_folders.take(fullId); instead of value() plus a separate remove() to both avoid a second hash lookup and make it explicit that the pointer is removed from the map at the same time it is retrieved.
• Given the crash origin, it may be safer to assert that page is non-null (e.g. Q_ASSERT(page);) after retrieving it from m_folders to catch any unexpected cases where the map contains a null entry.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Consider using `auto *page = m_folders.take(fullId);` instead of `value()` plus a separate `remove()` to both avoid a second hash lookup and make it explicit that the pointer is removed from the map at the same time it is retrieved.
- Given the crash origin, it may be safer to assert that `page` is non-null (e.g. `Q_ASSERT(page);`) after retrieving it from `m_folders` to catch any unexpected cases where the map contains a null entry.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[deepin-ci-robot]

[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.

Details

Needs approval from an approver in each of these files:

• debian/deepin/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[deepin-ci-robot]

[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.

Details

Needs approval from an approver in each of these files:

• debian/deepin/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[deepin-ci-robot]

deepin pr auto review

这段代码的 diff 展示了对 ItemArrangementProxyModel::removeFolder 函数的修改。以下是对修改后的代码进行审查，包括语法逻辑、代码质量、代码性能和代码安全方面的分析，并提出改进意见。

1. 语法逻辑分析

• 版权年份更新：将 SPDX-FileCopyrightText 从 2023 更新为 2023-2026 是符合规范的，表示代码版权的延续。
• 指针获取与断言：Q_ASSERT(m_folders.contains(fullId)); 保留了原有的断言逻辑，确保传入的 ID 存在于容器中。
• 核心逻辑变更：
  • 原代码：m_folders.remove(fullId); 直接删除键值对。
  • 新代码：auto *page = m_folders.take(fullId); 使用 take 方法移除并返回对应的指针，然后调用 page->disconnect(this); 断开信号槽连接。
• 重复删除风险：
  • m_folders.take(fullId); 已经将 fullId 对应的条目从 m_folders 容器中移除了。
  • 在函数末尾，代码再次调用了 m_folders.remove(fullId);。
  • 问题：这是一个逻辑错误。QHash (或 QMap) 的 remove 操作在键不存在时通常是无害的（不报错也不崩溃），但这属于冗余代码，且容易引起维护者的困惑。如果意图是确保移除，take 已经完成了这个动作。

2. 代码质量分析

• 资源管理：修改后的代码增加了 page->disconnect(this);，这表明开发者意识到需要断开信号连接以防止悬空信号或野指针访问。这是一个改进，提高了代码的健壮性。
• 内存泄漏风险：
  • auto *page = m_folders.take(fullId); 获取了 page 的所有权。
  • 代码断开了连接，但没有删除 page 对象（即没有 delete page;）。
  • 问题：这会导致内存泄漏。除非 page 指向的对象由其他对象（如父 QObject）管理，否则必须手动释放。从上下文看，m_folders 似乎管理着这些页面的生命周期，移除时通常意味着不再需要该页面。
• 冗余代码：如上所述，末尾的 m_folders.remove(fullId); 是多余的。

3. 代码性能分析

• 查找效率：
  • m_folders.take(fullId); 执行了一次哈希查找。
  • m_folderModel.findItems(fullId); 执行了一次模型查找（通常是线性查找或树遍历，取决于模型实现）。
  • m_folders.remove(fullId); 执行了另一次哈希查找。
• 优化建议：移除末尾的 m_folders.remove(fullId); 可以减少一次不必要的哈希查找操作，虽然性能影响微乎其微，但在高频调用或大规模数据下值得优化。

4. 代码安全分析

• 空指针解引用风险：
  • auto *page = m_folders.take(fullId); 后，紧接着调用了 page->disconnect(this);。
  • 虽然 Q_ASSERT 确保了 m_folders 包含该键，但在 Release 模式下 Q_ASSERT 会被移除。如果由于某种原因 fullId 不存在，take 返回 nullptr，随后的 page->disconnect 将导致程序崩溃。
  • 建议：应增加空指针检查，例如 if (page) { page->disconnect(this); ... }。
• result.first() 的安全性：
  • QList<QStandardItem*> result = m_folderModel.findItems(fullId);
  • m_folderModel.removeRows(result.first()->row(), 1);
  • 问题：如果 findItems 返回空列表（虽然逻辑上不应发生，因为前面断言了 ID 存在），调用 result.first() 会导致未定义行为（崩溃或断言失败）。
  • 建议：检查 result.isEmpty()。

改进后的代码建议

基于以上分析，建议修改如下：

void ItemArrangementProxyModel::removeFolder(const QString &idNumber)
{
    QString fullId("internal/folders/" + idNumber);
    
    // 检查 ID 是否存在，防止 Release 模式下崩溃
    if (!m_folders.contains(fullId)) {
        return;
    }

    // take 会自动移除键值对，并返回指针
    auto *page = m_folders.take(fullId);
    
    // 断开信号槽连接，并检查空指针
    if (page) {
        page->disconnect(this);
        // 假设 page 不是 QObject 树的一部分，需要手动删除以防止内存泄漏
        // 如果 page 的父对象设置了且会自动删除，则不需要下面这行
        delete page; 
    }

    m_topLevel->removeItem(fullId);
    
    QList<QStandardItem*> result = m_folderModel.findItems(fullId);
    // 检查查找结果是否有效
    if (!result.isEmpty()) {
        m_folderModel.removeRows(result.first()->row(), 1);
    }
    
    // 删除了冗余的 m_folders.remove(fullId);
}

总结

这段代码的主要意图是修复潜在的信号槽连接问题，但引入了内存泄漏风险和冗余的删除操作。建议按照上述改进代码进行修复，以确保逻辑正确、资源释放安全且无冗余操作。

[wjyrich]

/forcemerge

[deepin-bot]

This pr force merged! (status: behind)