Skip to content

fix: ensure remove tone for pinyin first letter search#656

Merged
BLumia merged 1 commit into
linuxdeepin:masterfrom
BLumia:pms-333411
Oct 10, 2025
Merged

fix: ensure remove tone for pinyin first letter search#656
BLumia merged 1 commit into
linuxdeepin:masterfrom
BLumia:pms-333411

Conversation

@BLumia

@BLumia BLumia commented Oct 10, 2025

Copy link
Copy Markdown
Member

确保简拼获取时没有音调.

Summary by Sourcery

Ensure pinyin initials are generated without tone for first-letter searches and add a test case for the '安全中心' app to validate the change

Bug Fixes:

  • Remove tone from pinyin first-letter generation to ensure accurate search matching

Tests:

  • Add the 'org.deepin.defender' (安全中心) test item and verify pinyin initials search for 'aqzx'

确保简拼获取时没有音调.

PMS: BUG-333411
Log:
@BLumia BLumia requested review from robertkill and wjyrich October 10, 2025 08:37
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

我来对这次代码变更进行审查:

  1. debian/control 文件变更:
  • 将 libdtk6core-dev 的依赖关系修改为指定版本号 (>= 6.0.43)
    这是一个好的变更,因为它确保了依赖的最低版本要求,避免因版本过低导致兼容性问题。
  1. src/models/searchfilterproxymodel.cpp 文件变更:
  • 在调用 Dtk::Core::firstLetters() 函数时添加了 TS_NoneTone 参数
    这个修改表明函数接口发生了变化,添加了音调处理选项。这是一个必要的更新,以保持与依赖库的新版本兼容。
  1. tests/searchfilterproxymodeltest.cpp 文件变更:
  • 添加了新的测试用例,测试安全中心应用的拼音首字母搜索
  • 添加了对"aqzx"(安全中心的拼音首字母)的测试

改进建议:

  1. 代码质量:
  • 在测试文件中,建议为新增的测试用例添加更详细的注释,说明测试的目的和预期结果
  • 测试数据中的权重值(4)建议使用有意义的命名常量代替魔法数字
  1. 代码性能:
  • SearchFilterProxyModel::calculateWeight() 函数中频繁的字符串操作可能会影响性能,建议考虑缓存计算结果
  • 对于拼音搜索,如果数据量大,可以考虑使用更高效的搜索算法或数据结构
  1. 代码安全:
  • 在处理用户输入的搜索字符串时,建议进行适当的输入验证和清理,防止潜在的注入攻击
  • 对于拼音转换函数,建议添加错误处理机制,当输入包含非中文字符时能够优雅处理
  1. 测试覆盖率:
  • 建议增加边界测试用例,如空字符串、特殊字符、超长字符串等
  • 可以添加对异常情况的测试,如无效的输入参数
  1. 兼容性:
  • 由于依赖库版本升级,建议验证所有使用 Dtk::Core::firstLetters() 的地方是否都需要更新参数
  • 考虑添加版本检查代码,以确保在旧版本库上运行时也能正常工作

总体而言,这次变更主要是为了适应依赖库的更新,并增加了相应的测试用例。变更本身是合理的,但建议关注上述提到的改进点,以提高代码的健壮性和性能。

@sourcery-ai

sourcery-ai Bot commented Oct 10, 2025

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Update pinyin initial extraction to strip tone marks and extend tests to verify tone-less initials for the new 'defender' item.

Class diagram for updated pinyin initial extraction in SearchFilterProxyModel

classDiagram
    class SearchFilterProxyModel {
        +calculateWeight(modelIndex: QModelIndex) : int
    }
    class Dtk::Core {
        +firstLetters(text: QString, toneOption: ToneOption) : QStringList
    }
    SearchFilterProxyModel --> Dtk::Core: uses
    class ToneOption {
        <<enumeration>>
        TS_NoneTone
    }
Loading

File-Level Changes

Change Details Files
Strip tone marks when computing pinyin initials
  • Pass TS_NoneTone to Dtk::Core::firstLetters in calculateWeight
src/models/searchfilterproxymodel.cpp
Extend unit tests for first-letter pinyin search without tones
  • Add a new test item for org.deepin.defender
  • Add initials-based search test for "aqzx" expecting the defender item
tests/searchfilterproxymodeltest.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

@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 there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@BLumia BLumia requested a review from 18202781743 October 10, 2025 08:38
@deepin-ci-robot

Copy link
Copy Markdown

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

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

@BLumia BLumia merged commit 1a936eb into linuxdeepin:master Oct 10, 2025
8 of 10 checks passed
@BLumia BLumia deleted the pms-333411 branch October 10, 2025 08:54
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