Skip to content

fix(reader): merge duplicate content records to restore bookmarks - #367

Merged
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
add-uos:fix-376029-merge-duplicate-content-records
Sep 8, 2026
Merged

fix(reader): merge duplicate content records to restore bookmarks#367
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
add-uos:fix-376029-merge-duplicate-content-records

Conversation

@add-uos

@add-uos add-uos commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

When the same document exists at multiple paths, readOperation may hit a stale record at the new path with a matching contentHash and skip matchOperationByContent, so bookmarks and reading progress stored at the old path are never migrated. Migrating to a path that already has a record also fails with a PRIMARY KEY conflict on operation.filePath.

Fix by merging same-content records from other paths after a verified readOperation hit: union bookmarks (dedup by page, filter mismatched hashes), prefer the state that has bookmarks (else the latest lastOpened), and delete the old-path records. In
matchOperationByContent, exclude the target path itself from candidates and clear its stale record before the path UPDATE.

修复文档移动到已有同内容记录的路径后书签与阅读进度丢失的问题:
readOperation 命中旧记录且指纹校验通过后,合并同内容(docId 或
fileSize+contentHash)的其他路径记录——书签并集迁移(按页去重并
过滤指纹不符的脏书签)、状态择优(带书签记录优先,否则取 lastOpened
较新者)并清理旧路径记录;matchOperationByContent 匹配候选排除目标
路径自身,迁移 UPDATE 前清理目标路径旧记录以避让主键冲突。

Log: 修复文档移动到U盘替换后书签与阅读进度丢失
PMS: BUG-376029
Influence: 本地与U盘存在同名文档时,添加书签后移动/替换文档再打开,书签与阅读进度可正常恢复。

Summary by Sourcery

Restore document reading state across moves and replacements while keeping independent copies isolated and cleaning up obsolete bookmark data.

Bug Fixes:

  • Restore bookmarks and reading progress when documents are moved or replaced at paths that already contain records for the same content.
  • Prevent stale or mismatched bookmark data from being applied across replaced files while preserving bookmarks for historical content versions.
  • Preserve independent reading state and bookmarks for identical documents that coexist at different paths.

Enhancements:

  • Distinguish document moves from copies when matching content, migrating state for vanished sources while borrowing state for existing sources.
  • Merge duplicate records by selecting the preferred reading state, migrating valid bookmarks, and removing obsolete path records.
  • Improve bookmark lifecycle management, including legacy handling, content-fingerprint association, and orphan cleanup for reliably unavailable local files.
  • Avoid saving spurious reading positions while document tabs are hidden and defer layout restoration until the tab is visible.

Tests:

  • Add coverage for duplicate-record merging, bookmark migration and deduplication, content-based matching with occupied target paths, copy-state borrowing, path independence, content forks, and orphan bookmark cleanup.

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

Sorry @add-uos, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 3 days and 8 hours by commenting @sourcery-ai review. Upgrade to get a review now.

@sourcery-ai

sourcery-ai Bot commented Sep 7, 2026

Copy link
Copy Markdown

Reviewer's Guide

Restores bookmarks and reading progress across duplicate document paths by merging validated same-content records, safely resolving destination-path conflicts during fallback migration, and covering the key move, replacement, docId, bookmark, and timestamp scenarios with database tests.

Sequence diagram for merging duplicate document records after a validated read

sequenceDiagram
    participant Reader
    participant Database
    participant OperationDB
    participant BookmarkDB

    Reader->>Database: readOperation(sheet)
    Database->>OperationDB: Verify record at newPath
    Database->>Database: mergeDuplicateRecords(sheet)
    Database->>OperationDB: Find records at other paths by docId or fileSize+contentHash
    loop Each duplicate path
        Database->>BookmarkDB: migrateBookmarksToPath(oldPath, newPath, contentHash)
        BookmarkDB-->>Database: Merge valid bookmarks by page
        Database->>OperationDB: Prefer bookmarked state or newer lastOpened
        Database->>OperationDB: Delete oldPath operation record
    end
    Database-->>Reader: Restored bookmarks and reading state
Loading

Sequence diagram for safe content-match path migration

sequenceDiagram
    participant Reader
    participant Database
    participant OperationDB
    participant BookmarkDB

    Reader->>Database: matchOperationByContent(fileInfo, sheet)
    Database->>OperationDB: Find matching record excluding target filePath
    Database->>OperationDB: DELETE target-path stale operation record
    Database->>OperationDB: UPDATE oldPath to newPath
    Database->>BookmarkDB: migrateBookmarksToPath(oldPath, newPath, contentHash)
    BookmarkDB-->>Database: Deduplicated valid bookmarks
    Database-->>Reader: Commit migrated operation and bookmarks
Loading

Entity relationship diagram for merged operation and bookmark records

erDiagram
    OPERATION {
        string filePath PK
        string docId
        string contentHash
        int fileSize
        int currentPage
        int lastOpened
    }
    BOOKMARK {
        string filePath
        int bookmarkIndex
        string contentHash
    }
    OPERATION ||--o{ BOOKMARK : has
    OPERATION }o--|| OPERATION : same_content_merge_target
Loading

File-Level Changes

Change Details Files
Merge duplicate document records after a verified path-record hit so bookmarks and reading state follow moves or replacements.
  • Invoke duplicate detection after read validation using docId or fileSize/contentHash.
  • Merge bookmarks by page, filter mismatched hashes, normalize valid hashes, and remove source-path data.
  • Select reading state by bookmark presence, then latest lastOpened; update the active DocSheet and delete redundant operation records.
reader/app/Database.cpp
reader/app/Database.h
Make content-based path migration safe when the destination already has a stale database record.
  • Exclude the destination path from docId and hash match candidates.
  • Delete a conflicting destination operation before updating the matched source path.
  • Use transactional bookmark union migration and stale-destination bookmark cleanup.
reader/app/Database.cpp
Add regression coverage for duplicate-record merging and destination-conflict migration.
  • Test bookmark and state recovery when only the source has bookmarks.
  • Test bookmark union and latest-state selection when both paths have bookmarks.
  • Test docId matching with differing content hashes and latest-state retention.
  • Test primary-key conflict avoidance and stale bookmark filtering during fallback migration.
tests/app/ut_database.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

@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

🤖 AI 代码审查报告

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

Pass


📊 总体评价

项目 结果
审查结论 代码审查通过
评分详情 总体评分 89 分,大于 70 分通过阈值。代码修复了文档移动到已有同内容记录路径后书签与阅读进度丢失的问题,实现逻辑清晰,SQL 全部使用参数化查询,无安全漏洞。mergeDuplicateRecords 函数略长但结构清晰,建议后续优化拆分。

🔍 详细分析

1. 语法逻辑 ✅

评价: 优秀 ✅ 通过

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

建议: 语法正确,逻辑清晰。mergeDuplicateRecords 方法正确处理同内容多路径记录合并:先查询当前路径记录的内容特征,再查找同内容(docId 或 fileSize+contentHash)的其他路径记录,通过 migrateBookmarksToPath 进行书签并集迁移去重,状态择优逻辑合理(带书签记录优先,否则取 lastOpened 较新者)。matchOperationByContent 修改正确:排除目标路径自身避免无效迁移,迁移前清理目标路径旧记录避让主键冲突。边界处理完善:sheet 空指针检查、contentHash 空值跳过、scaleFactor 使用 qBound 边界约束、JSON 解析使用 isArray 校验。


2. 代码质量 ❌

评价: 良好 ❌ 不通过

潜在问题:

  1. reader/app/Database.cpp:131 - mergeDuplicateRecords 函数约145行(含注释),实际代码约117行,超过100行建议长度。建议将内存状态同步部分提取为独立辅助函数。

建议: 结构需要优化。建议将 mergeDuplicateRecords 中 sheet->m_operation 的内存状态同步代码(layoutMode、mouseShape、scaleMode 等字段赋值)提取为独立的 syncOperationToMemory 辅助函数,降低主函数长度。其余方面:注释完整性优秀(Doxygen 风格 + 行内解释决策原因),代码复用良好(migrateBookmarksToPath 被 matchOperationByContent 和 mergeDuplicateRecords 两处复用),无残留调试代码。


3. 代码性能 ✅

评价: 优秀 ✅ 通过

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

建议: 性能良好,资源使用合理。SQL 查询使用 filePath 主键索引,contentHash/fileSize 字段查询高效。migrateBookmarksToPath 使用 INSERT...SELECT...NOT EXISTS 单语句完成去重迁移,避免多次查询。所有操作在单个事务中执行保证原子性。mergeDuplicateRecords 中的循环次数取决于重复记录数(实际场景通常0-2条),无性能瓶颈。readOperation 中新增的 mergeDuplicateRecords 调用仅在指纹校验通过后执行,不影响正常打开流程。


4. 代码安全 🔒

评价: 优秀 ✅ 通过

🔐 发现 0 个安全漏洞

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

建议: 存在0个安全漏洞,安全合规。全部 SQL 语句使用参数化查询(prepare/bindValue),无 SQL 注入风险。文件路径来源于 sheet->filePath() 和 fileInfo.absoluteFilePath(),为应用内部路径,无用户直接输入注入。日志仅输出路径和计数,无敏感信息泄露。事务保证数据一致性,错误处理使用 qCWarning 记录但不暴露系统信息。测试代码中有一处 SQL 字符串拼接(ut_database.cpp:495-496),但值为测试常量路径,无安全风险。


💡 改进建议代码示例

// 建议将 mergeDuplicateRecords 中的内存状态同步提取为辅助函数
void Database::syncOperationToMemory(DocSheet *sheet, const QVariantMap &dup)
{
    sheet->m_operation.layoutMode = static_cast<Dr::LayoutMode>(dup.value("layoutMode").toInt());
    sheet->m_operation.mouseShape = static_cast<Dr::MouseShape>(dup.value("mouseShape").toInt());
    sheet->m_operation.scaleMode = static_cast<Dr::ScaleMode>(dup.value("scaleMode").toInt());
    sheet->m_operation.rotation = static_cast<Dr::Rotation>(dup.value("rotation").toInt());
    sheet->m_operation.scaleFactor = qBound(0.1, dup.value("scaleFactor").toDouble(), 5.0);
    sheet->m_operation.sidebarVisible = dup.value("sidebarVisible").toInt();
    sheet->m_operation.sidebarIndex = dup.value("sidebarIndex").toInt();
    sheet->m_operation.currentPage = dup.value("currentPage").toInt();
    sheet->m_operation.sidebarWidth = dup.value("sidebarWidth").toInt();
    sheet->m_operation.sidebarWidthChanged = dup.value("sidebarWidthChanged").toInt() != 0;
    sheet->m_operation.scrollPosition = dup.value("scrollPosition").toFloat();
    QString expandedJson = dup.value("expandedSections").toString();
    QJsonDocument expDoc = QJsonDocument::fromJson(expandedJson.toUtf8());
    if (expDoc.isArray()) {
        sheet->m_operation.expandedSections.clear();
        for (const QJsonValue &val : expDoc.array()) {
            sheet->m_operation.expandedSections.append(val.toString());
        }
    }
}
// 在 mergeDuplicateRecords 中调用:
// syncOperationToMemory(sheet, dup);
// curLastOpened = dupLastOpened;

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

@add-uos
add-uos force-pushed the fix-376029-merge-duplicate-content-records branch 2 times, most recently from 3e4b956 to f1fac97 Compare September 8, 2026 02:53
Comment thread reader/app/Database.cpp
idQuery.prepare("SELECT * FROM operation WHERE docId = :docId AND docId != ''");
// 排除目标路径自身的记录:同一份文件可能在目标路径已有旧记录,
// 选中它会导致 oldPath == newPath,做一次无效迁移
idQuery.prepare("SELECT * FROM operation WHERE docId = :docId AND docId != '' AND filePath != :filePath");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

误报

@add-uos
add-uos force-pushed the fix-376029-merge-duplicate-content-records branch from f1fac97 to 2cbab25 Compare September 8, 2026 05:20
When the same document exists at multiple paths, readOperation may hit
a stale record at the new path with a matching contentHash and skip
matchOperationByContent, so bookmarks and reading progress stored at
the old path are never migrated. Migrating to a path that already has
a record also fails with a PRIMARY KEY conflict on operation.filePath.

Fix by merging same-content records from other paths after a verified
readOperation hit: union bookmarks (dedup by page, filter mismatched
hashes), prefer the state that has bookmarks (else the latest
lastOpened), and delete the old-path records. In
matchOperationByContent, exclude the target path itself from
candidates and clear its stale record before the path UPDATE.

修复文档移动到已有同内容记录的路径后书签与阅读进度丢失的问题:
readOperation 命中旧记录且指纹校验通过后,合并同内容(docId 或
fileSize+contentHash)的其他路径记录——书签并集迁移(按页去重并
过滤指纹不符的脏书签)、状态择优(带书签记录优先,否则取 lastOpened
较新者)并清理旧路径记录;matchOperationByContent 匹配候选排除目标
路径自身,迁移 UPDATE 前清理目标路径旧记录以避让主键冲突。

Log: 修复文档移动到U盘替换后书签与阅读进度丢失
PMS: BUG-376029
Influence: 本地与U盘存在同名文档时,添加书签后移动/替换文档再打开,书签与阅读进度可正常恢复。
@add-uos
add-uos force-pushed the fix-376029-merge-duplicate-content-records branch from 2cbab25 to f7f4b13 Compare September 8, 2026 05:25
@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: add-uos, lzwind

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

@add-uos

add-uos commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

/merge

@deepin-bot
deepin-bot Bot merged commit f575e48 into linuxdeepin:master Sep 8, 2026
9 checks passed
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.

3 participants