Skip to content

Conversation

@JWWTSL
Copy link
Contributor

@JWWTSL JWWTSL commented Dec 29, 2025

log: Optimize filtering rules to exclude Chinese

bug: https://pms.uniontech.com/bug-view-344067.html

Summary by Sourcery

Bug Fixes:

  • Prevent Chinese and other non-English letters from being accepted in calculator input fields.

@sourcery-ai
Copy link

sourcery-ai bot commented Dec 29, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Restricts calculator input filtering to allow only digits, ASCII English letters, and a defined set of operator/symbol characters, explicitly excluding Chinese characters that were previously admitted via QChar::isLetter().

Class diagram for InputEdit with updated handleTextChanged filtering

classDiagram
    class InputEdit {
        +void handleTextChanged(text: QString)
    }
Loading

File-Level Changes

Change Details Files
Tighten input character filtering to exclude Chinese letters while still allowing digits, English letters, and specific symbols.
  • Clarified comment to specify that only English letters are allowed and documented why QChar::isLetter() cannot be used due to matching Chinese characters.
  • Introduced an explicit ASCII English-letter check using character ranges 'A'-'Z' and 'a'-'z'.
  • Replaced the prior QChar::isLetter() usage in the filter condition with the new English-letter predicate while retaining checks for digits, configured decimal/grouping symbols, and the static allowed symbol set.
src/widgets/inputedit.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

deepin pr auto review

我来对这段代码进行审查:

  1. 语法逻辑:
  • 代码逻辑清晰,通过遍历字符串并过滤非法字符
  • 新增的 isEnglishLetter 判断逻辑正确,避免了中文字符的误判
  • 代码流程合理,从输入处理到字符过滤的步骤清晰
  1. 代码质量:
  • 使用了 std::as_const(reformatStr) 是好的实践,避免了不必要的拷贝
  • filtered.reserve(reformatStr.size()) 预分配内存,提高性能
  • 注释清晰,说明了不能使用 isLetter() 的原因
  • 变量命名清晰,如 allowedStatic、filtered 等命名合理
  1. 代码性能:
  • 使用 QChar 的直接比较而不是 isLetter() 提高了性能
  • 预分配内存减少了动态扩容的开销
  • allowedStatic 使用 QString::fromUtf8 是合理的做法
  1. 代码安全:
  • 字符过滤逻辑严谨,只允许特定字符通过
  • 使用范围检查 (ch >= 'A' && ch <= 'Z') 确保只匹配英文字母
  • decSym 和 grpSym 的使用考虑了本地化需求

改进建议:

  1. 可以考虑将 allowedStatic 定义为 const static 成员变量,避免每次调用都创建新字符串:
static const QString allowedStatic = QString::fromUtf8("+-×÷.,%()/:^!");
  1. 可以考虑使用 QChar 的 unicode() 方法进行更精确的字符范围检查:
bool isEnglishLetter = (ch.unicode() >= 'A' && ch.unicode() <= 'Z') || 
                      (ch.unicode() >= 'a' && ch.unicode() <= 'z');
  1. 可以考虑将字符过滤逻辑提取为单独的函数,提高代码复用性和可测试性:
bool InputEdit::isAllowedChar(const QChar& ch) const {
    bool isEnglishLetter = (ch.unicode() >= 'A' && ch.unicode() <= 'Z') || 
                          (ch.unicode() >= 'a' && ch.unicode() <= 'z');
    return ch.isDigit() || isEnglishLetter || allowedStatic.contains(ch) || 
           decSym.contains(ch) || grpSym.contains(ch);
}
  1. 建议添加单元测试来验证字符过滤逻辑的正确性,特别是边界情况。

  2. 考虑添加对 Unicode 空白字符的处理,如果需要的话。

总体来说,这段代码质量良好,改进建议主要是为了提高代码的可维护性和性能。

Copy link

@sourcery-ai sourcery-ai bot left a comment

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, and left some high level feedback:

  • The ASCII range check for English letters works but is a bit ad hoc; consider using ch.isLatin1() combined with ch.isLetter() (and possibly an explicit QChar::Script_Latin check if available) to more clearly and robustly express the intent to allow only Latin letters while excluding Chinese.
  • To avoid duplicating this logic if similar filtering is needed elsewhere, you might extract the "is allowed character" check into a small helper function or lambda, which will also make the loop body easier to read.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The ASCII range check for English letters works but is a bit ad hoc; consider using `ch.isLatin1()` combined with `ch.isLetter()` (and possibly an explicit `QChar::Script_Latin` check if available) to more clearly and robustly express the intent to allow only Latin letters while excluding Chinese.
- To avoid duplicating this logic if similar filtering is needed elsewhere, you might extract the "is allowed character" check into a small helper function or lambda, which will also make the loop body easier to read.

## Individual Comments

### Comment 1
<location> `src/widgets/inputedit.cpp:551-552` </location>
<code_context>
     filtered.reserve(reformatStr.size());
     for (const QChar &ch : std::as_const(reformatStr)) {
-        if (ch.isDigit() || ch.isLetter() || allowedStatic.contains(ch) || decSym.contains(ch) || grpSym.contains(ch)) {
+        bool isEnglishLetter = (ch >= 'A' && ch <= 'Z') || (ch >= 'a' && ch <= 'z');
+        if (ch.isDigit() || isEnglishLetter || allowedStatic.contains(ch) || decSym.contains(ch) || grpSym.contains(ch)) {
             filtered.append(ch);
         }
</code_context>

<issue_to_address>
**question:** Clarify whether restricting letters to ASCII only is intentional; consider whether extended Latin letters should be accepted.

This change now only accepts ASCII A–Z/a–z and rejects extended Latin characters like `é`, `ø`, `ß` or full-width Latin letters that `ch.isLetter()` previously allowed. If non-ASCII Latin input is expected, consider a broader condition (e.g. `QChar::isLatin1()` or another Unicode-aware check) rather than a pure ASCII range.
</issue_to_address>

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.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JWWTSL, 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

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