Skip to content

refactor: fix app group management and folder IDs - #1784

Open
wjyrich wants to merge 1 commit into
linuxdeepin:masterfrom
wjyrich:pr/shared-application-folders
Open

wjyrich wants to merge 1 commit into
linuxdeepin:masterfrom
wjyrich:pr/shared-application-folders

Conversation

@wjyrich

@wjyrich wjyrich commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor
  1. Fix folder ID parsing off-by-one bug and standardize canonical prefix from "internal/folder/" to "internal/folders/"
  2. Support legacy ID formats ("internal/folder/", "internal/group/") during loading while always writing the canonical plural form
  3. Decouple AppGroupManager from AMAppItemModel, using QAbstractItemModel allowing in-memory test sources
  4. Add DConfig injection constructor for testability with memory backend
  5. Coalesce reference model changes into a single refresh using a zero- delay timer, avoiding mutations during begin/endRemoveRows transactions
  6. Guard against stale drag coordinates, out-of-range page indices, and nested folder drops
  7. Handle last-child removal from a folder when dropped onto its own folder slot
  8. Detect ready property on reference model to avoid pruning folders before app metadata is loaded
  9. Migrate legacy launchpad INI only once with ArrangementMigrated flag, keeping INI as backup
  10. Add QML-invokable APIs: itemPosition, folderInfo, pageCount, updateFolderName, createEmptyPage, removeEmptyPages, rearrange, bringToFront
  11. Rename bringToFromt to bringToFront and accept .desktop suffix
  12. Add comprehensive test suite appgroupmanagertests.cpp covering folder creation, renaming, moves, migration, and persistence
  13. Fix ItemsPage firstNItems(0) and bounds handling in insertItem/ insertItemToPage/moveItemPosition

Log: Fixed folder management stability, drag-drop edge cases, and legacy config migration

Influence:

  1. Verify folder creation by dragging one app onto another
  2. Test folder rename and persistence across restarts
  3. Test dragging apps in/out of folders at page boundaries
  4. Verify last child app can be moved out of a folder without losing position
  5. Test folder cannot be nested into another folder
  6. Verify stale drag drops are ignored gracefully
  7. Test launchpad INI migration runs only once and is preserved
  8. Verify apps with .desktop suffix are handled correctly
  9. Test empty page creation and removal
  10. Verify top-level always exists even with empty app set
  11. Run appgroupmanager_tests and itemspage_tests unit tests

refactor: 修复应用分组管理和文件夹ID

  1. 修复文件夹ID解析的偏移错误,并将标准前缀从 "internal/folder/" 改为 "internal/folders/"
  2. 加载时支持旧格式ID("internal/folder/"、"internal/group/"),但写入时 始终使用规范的复数形式
  3. 将AppGroupManager与AMAppItemModel解耦,改用QAbstractItemModel以支持内 存测试源
  4. 添加DConfig注入构造函数以便使用内存后端进行测试
  5. 使用零延迟定时器合并参考模型变更,避免在begin/endRemoveRows事务期间修 改数据
  6. 对过期的拖拽坐标、越界页码和嵌套文件夹拖放进行防护
  7. 处理文件夹中最后一个子项被拖放到其自身文件夹位置的情况
  8. 检测参考模型的ready属性,避免在应用元数据加载前清理文件夹
  9. 使用ArrangementMigrated标志确保旧版启动器INI仅迁移一次,同时保留INI作 为备份
  10. 添加QML可调用的API:itemPosition、folderInfo、pageCount、 updateFolderName、createEmptyPage、removeEmptyPages、rearrange、 bringToFront
  11. 将bringToFromt重命名为bringToFront并支持.desktop后缀
  12. 添加全面的测试套件appgroupmanagertests.cpp,覆盖文件夹创建、重命名、 移动、迁移和持久化
  13. 修复ItemsPage firstNItems(0)以及insertItem/insertItemToPage/ moveItemPosition的边界处理

Log: 修复文件夹管理稳定性、拖放边界情况和旧版配置迁移

Influence:

  1. 验证通过将一个应用拖到另一个应用上创建文件夹
  2. 测试文件夹重命名及重启后的持久化
  3. 测试在页面边界处将应用拖入/拖出文件夹
  4. 验证最后一个子应用可以移出文件夹且不丢失位置
  5. 测试文件夹不能嵌套到另一个文件夹中
  6. 验证过期拖放操作被优雅忽略
  7. 测试启动器INI迁移只运行一次并被保留
  8. 验证带.desktop后缀的应用被正确处理
  9. 测试空页面的创建和删除
  10. 验证即使应用集为空,顶层也始终存在
  11. 运行appgroupmanager_tests和itemspage_tests单元测试

PMS: TASK-396759

Summary by Sourcery

Stabilize app group and folder management with canonical IDs, safer rearrangement behavior, testable model/configuration dependencies, and one-time legacy arrangement migration.

New Features:

  • Expose QML-invokable APIs for querying folder state, managing pages, renaming folders, rearranging items, and bringing applications to the front.

Bug Fixes:

  • Correct folder ID parsing and canonicalization while retaining compatibility with legacy IDs.
  • Harden folder and application drag-and-drop handling against stale coordinates, invalid pages, nested folders, and last-child removal cases.
  • Preserve folder metadata until application sources are ready and keep the top-level folder available for empty sources.
  • Improve item pagination and movement bounds handling, including desktop-suffixed IDs.

Enhancements:

  • Decouple app group management from the application-specific model and support injected configuration backends.
  • Coalesce reference model updates to stabilize folder synchronization.
  • Migrate legacy launchpad arrangements once while preserving the original INI as a backup.

Tests:

  • Add comprehensive app group manager tests covering folder operations, persistence, migration, drag-and-drop edge cases, and source readiness.
  • Expand ItemsPage tests for empty-page insertion and invalid movement coordinates.

@deepin-ci-robot

Copy link
Copy Markdown

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

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

@sourcery-ai

sourcery-ai Bot commented Oct 27, 2026

Copy link
Copy Markdown

Reviewer's Guide

This refactor standardizes and migrates folder IDs while making AppGroupManager testable with generic in-memory models, deferring reference synchronization safely, exposing QML arrangement APIs, and hardening drag/drop and pagination edge cases with comprehensive regression tests.

Sequence diagram for deferred app arrangement synchronization

sequenceDiagram
    participant ReferenceModel
    participant AppGroupManager
    participant SyncTimer
    participant Config

    ReferenceModel->>AppGroupManager: rowsInserted / rowsRemoved / dataChanged
    AppGroupManager->>SyncTimer: scheduleReferenceSync()
    SyncTimer-->>AppGroupManager: timeout
    AppGroupManager->>AppGroupManager: onReferenceModelChanged()
    AppGroupManager->>Config: saveAppGroupInfo()
    AppGroupManager-->>AppGroupManager: notifyArrangementChanged()
Loading

Sequence diagram for legacy arrangement migration

sequenceDiagram
    participant AppGroupManager
    participant DConfig
    participant LegacyINI

    AppGroupManager->>DConfig: value(ArrangementMigrated)
    alt Migration already completed
        DConfig-->>AppGroupManager: true
    else Migration required
        AppGroupManager->>LegacyINI: read item-arrangement.ini
        LegacyINI-->>AppGroupManager: legacy groups and desktop IDs
        AppGroupManager->>DConfig: setValue(Groups)
        AppGroupManager->>DConfig: setValue(ArrangementMigrated, true)
    end
Loading

Flow diagram for folder-aware drag and drop

flowchart TD
    Start[rearrange dragId dropId operation pageHint] --> Normalize[Normalize desktop suffix]
    Normalize --> Find[findItem]
    Find --> Valid{Positions valid?}
    Valid -- No --> Ignore[Ignore operation]
    Valid -- Yes --> Operation{DndJoin?}
    Operation -- Yes --> JoinChecks[Validate folder and page bounds]
    JoinChecks --> MoveInto[Move item into destination folder]
    Operation -- No --> Nested{Nested folder drop?}
    Nested -- Yes --> Ignore
    Nested -- No --> Reorder[Reorder or move between pages]
    MoveInto --> Persist[saveAppGroupInfo]
    Reorder --> Persist
    Persist --> Notify[notifyArrangementChanged]
Loading

File-Level Changes

Change Details Files
Normalize folder identifiers while preserving compatibility with legacy persisted and launchpad formats.
  • Fix folder ID parsing and canonical plural prefix generation.
  • Load legacy folder/group prefixes and normalize stored child IDs.
  • Migrate the legacy INI once via a configuration flag while retaining the source file as backup.
  • Handle desktop filename suffixes during migration and public operations.
applets/dde-apps/appgroup.cpp
applets/dde-apps/appgroupmanager.cpp
applets/dde-apps/appgroupmanager.h
applets/dde-apps/configs/org.deepin.ds.dde-apps.json
Refactor arrangement management for generic model sources, safer synchronization, and expanded QML control.
  • Accept QAbstractItemModel references and injectable DConfig backends.
  • Defer and coalesce reference-model updates to avoid mutating during model transactions.
  • Respect reference-model readiness and preserve folder metadata when application metadata is unavailable.
  • Add invokable positioning, folder information, pagination, naming, page, rearrangement, and front-order APIs.
  • Add guards for stale drops, invalid pages, nested folders, deleted folders, and last-child moves.
applets/dde-apps/appgroupmanager.cpp
applets/dde-apps/appgroupmanager.h
Harden pagination and item movement against empty pages and invalid coordinates.
  • Return empty results for non-positive first-item requests and safely access invalid pages.
  • Clamp insertion positions and create a page when inserting at the page-count boundary.
  • Ignore invalid move and destination-page coordinates while preserving valid reorder behavior.
applets/dde-apps/itemspage.cpp
tests/applets/itemspagetests.cpp
Add regression coverage for folder lifecycle, migration, persistence, drag-and-drop, and in-memory operation.
  • Introduce an in-memory configuration backend and model source for manager tests.
  • Cover creation, renaming, folder/page moves, nested/stale drops, metadata readiness, uninstall cleanup, persistence, and legacy migration.
  • Register the app-group manager test target and runtime environment.
tests/applets/appgroupmanagertests.cpp
tests/applets/CMakeLists.txt

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

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="applets/dde-apps/appgroupmanager.cpp" line_range="356-389" />
<code_context>
+    const auto saved = m_config->value("Groups").toList();
</code_context>
<issue_to_address>
**Top-level folder is missing on first run**

When the configuration has no saved Groups entry, as on a fresh install without an imported arrangement, `loadAppGroupInfo()` appends only saved records, so with no saved groups it creates no top-level folder. `onReferenceModelChanged()` then cannot add applications because `appendItemToGroup()` returns when group 0 is absent; the launcher starts with no arranged apps.

Ensure group 0 is created during loading before adding or recovering any applications.
</issue_to_address>

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

Comment thread applets/dde-apps/appgroupmanager.cpp
@wjyrich
wjyrich force-pushed the pr/shared-application-folders branch 2 times, most recently from 2458733 to 7fbb039 Compare October 27, 2026 12:08
@deepin-bot

deepin-bot Bot commented Oct 28, 2026

Copy link
Copy Markdown

TAG Bot

New tag: 2.0.61
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #1786

1. Fix folder ID parsing off-by-one bug and standardize canonical prefix
from "internal/folder/" to "internal/folders/"
2. Support legacy ID formats ("internal/folder/", "internal/group/")
during loading while always writing the canonical plural form
3. Decouple AppGroupManager from AMAppItemModel, using
QAbstractItemModel allowing in-memory test sources
4. Add DConfig injection constructor for testability with memory backend
5. Coalesce reference model changes into a single refresh using a zero-
delay timer, avoiding mutations during begin/endRemoveRows transactions
6. Guard against stale drag coordinates, out-of-range page indices, and
nested folder drops
7. Handle last-child removal from a folder when dropped onto its own
folder slot
8. Detect ready property on reference model to avoid pruning folders
before app metadata is loaded
9. Migrate legacy launchpad INI only once with ArrangementMigrated flag,
keeping INI as backup
10. Add QML-invokable APIs: itemPosition, folderInfo, pageCount,
updateFolderName, createEmptyPage, removeEmptyPages, rearrange,
bringToFront
11. Rename bringToFromt to bringToFront and accept .desktop suffix
12. Add comprehensive test suite appgroupmanagertests.cpp covering
folder creation, renaming, moves, migration, and persistence
13. Fix ItemsPage firstNItems(0) and bounds handling in insertItem/
insertItemToPage/moveItemPosition

Log: Fixed folder management stability, drag-drop edge cases, and legacy
config migration

Influence:
1. Verify folder creation by dragging one app onto another
2. Test folder rename and persistence across restarts
3. Test dragging apps in/out of folders at page boundaries
4. Verify last child app can be moved out of a folder without losing
position
5. Test folder cannot be nested into another folder
6. Verify stale drag drops are ignored gracefully
7. Test launchpad INI migration runs only once and is preserved
8. Verify apps with .desktop suffix are handled correctly
9. Test empty page creation and removal
10. Verify top-level always exists even with empty app set
11. Run appgroupmanager_tests and itemspage_tests unit tests

refactor: 修复应用分组管理和文件夹ID

1. 修复文件夹ID解析的偏移错误,并将标准前缀从 "internal/folder/" 改为
"internal/folders/"
2. 加载时支持旧格式ID("internal/folder/"、"internal/group/"),但写入时
始终使用规范的复数形式
3. 将AppGroupManager与AMAppItemModel解耦,改用QAbstractItemModel以支持内
存测试源
4. 添加DConfig注入构造函数以便使用内存后端进行测试
5. 使用零延迟定时器合并参考模型变更,避免在begin/endRemoveRows事务期间修
改数据
6. 对过期的拖拽坐标、越界页码和嵌套文件夹拖放进行防护
7. 处理文件夹中最后一个子项被拖放到其自身文件夹位置的情况
8. 检测参考模型的ready属性,避免在应用元数据加载前清理文件夹
9. 使用ArrangementMigrated标志确保旧版启动器INI仅迁移一次,同时保留INI作
为备份
10. 添加QML可调用的API:itemPosition、folderInfo、pageCount、
updateFolderName、createEmptyPage、removeEmptyPages、rearrange、
bringToFront
11. 将bringToFromt重命名为bringToFront并支持.desktop后缀
12. 添加全面的测试套件appgroupmanagertests.cpp,覆盖文件夹创建、重命名、
移动、迁移和持久化
13. 修复ItemsPage firstNItems(0)以及insertItem/insertItemToPage/
moveItemPosition的边界处理

Log: 修复文件夹管理稳定性、拖放边界情况和旧版配置迁移

Influence:
1. 验证通过将一个应用拖到另一个应用上创建文件夹
2. 测试文件夹重命名及重启后的持久化
3. 测试在页面边界处将应用拖入/拖出文件夹
4. 验证最后一个子应用可以移出文件夹且不丢失位置
5. 测试文件夹不能嵌套到另一个文件夹中
6. 验证过期拖放操作被优雅忽略
7. 测试启动器INI迁移只运行一次并被保留
8. 验证带.desktop后缀的应用被正确处理
9. 测试空页面的创建和删除
10. 验证即使应用集为空,顶层也始终存在
11. 运行appgroupmanager_tests和itemspage_tests单元测试

PMS: TASK-396759
@wjyrich
wjyrich force-pushed the pr/shared-application-folders branch from 7fbb039 to dbea6e4 Compare October 29, 2026 03:43
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

🤖 AI 代码审查报告

总体评分: 98 分 (通过阈值: 70分)

Pass


📊 总体评价

项目 结果
审查结论 代码审查通过
评分详情 本次提交对 dde-shell 的应用分组管理进行了重构,修复了 folder ID 的 off-by-one bug,将硬编码的 AMAppItemModel 解耦为 QAbstractItemModel,用优雅的 null 检查替代了 Q_ASSERT 断言,并添加了完善的单元测试。四维度均通过,无安全漏洞,代码质量高,测试覆盖充分。

🔍 详细分析

1. 语法逻辑 ✅

评价: 语法正确,逻辑清晰 ✅ 通过

潜在问题:
✅ 未发现明显问题

建议: ['代码逻辑清晰,无需修改']


2. 代码质量 ✅

评价: 代码结构清晰,注释完整 ✅ 通过

潜在问题:
✅ 未发现明显问题

建议: ['建议将 commitRearrangeOperation 中的同文件夹移动、跨文件夹移动、加入文件夹三个分支提取为独立的私有方法', '建议为 launchpadArrangementConfigMigration 中的 importGroups lambda 提取为独立的私有方法']


3. 代码性能 ✅

评价: 性能良好,资源使用合理 ✅ 通过

潜在问题:
✅ 未发现明显问题

建议: ['性能表现良好,无需优化']


4. 代码安全 🔒

评价: 存在0个安全漏洞 ✅ 通过

🔐 发现 0 个安全漏洞

安全漏洞详情:
✅ 未发现安全漏洞

建议: ['安全合规,无需加固']


💡 改进建议代码示例

// 建议:将 commitRearrangeOperation 中的分支提取为子方法以提升可读性
// 示例(appgroupmanager.cpp):
//
// void AppGroupManager::commitRearrangeOperation(const QString &dragId, const QString &dropId,
//                                                DndOperation operation, int pageHint)
// {
//     // ... 前置校验 ...
//
//     if (operation != DndOperation::DndJoin) {
//         if (dragOrigPos.group() == dropOrigPos.group()) {
//             moveWithinFolder(dragOrigPos, dropOrigPos, operation);
//         } else {
//             moveAcrossFolders(dragId, dropId, dragOrigPos, dropOrigPos, operation);
//         }
//     } else {
//         joinOrCreateFolder(dragId, dropId, dragOrigPos, dropOrigPos, pageHint);
//     }
//
//     saveAppGroupInfo();
//     notifyArrangementChanged();
// }

本报告由 AI 代码审查工具自动生成

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants