Skip to content

Conversation

@add-uos
Copy link
Contributor

@add-uos add-uos commented Dec 31, 2025

  • Fixed monitor resolution display logic (negated isHwPlatform condition)
  • Enhanced smartctl regex to better match device identifier patterns

log: correct platform condition logic and regex pattern
bug: https://pms.uniontech.com/bug-view-340917.html

Summary by Sourcery

Fix monitor platform-specific resolution handling and improve smartctl output parsing.

Bug Fixes:

  • Correct monitor resolution processing to run only on non-HW platforms.
  • Adjust smartctl attribute parsing regex to more robustly match device identifiers and values.

- Fixed monitor resolution display logic (negated isHwPlatform condition)
- Enhanced smartctl regex to better match device identifier patterns

log: correct platform condition logic and regex pattern
bug: https://pms.uniontech.com/bug-view-340917.html
@sourcery-ai
Copy link

sourcery-ai bot commented Dec 31, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adjusts monitor info handling to only process and display supported resolutions on non-HW platforms and refines the smartctl parsing regex to more accurately capture device identifiers and values.

Sequence diagram for updated monitor resolution handling in setInfoFromHwinfo

sequenceDiagram
    participant Caller
    participant DeviceMonitor
    participant Common

    Caller->>DeviceMonitor: setInfoFromHwinfo(mapInfo)
    activate DeviceMonitor
    DeviceMonitor->>DeviceMonitor: setAttribute(mapInfo, "", m_DisplayInput)
    DeviceMonitor->>DeviceMonitor: setAttribute(mapInfo, Size, m_ScreenSize)
    DeviceMonitor->>DeviceMonitor: setAttribute(mapInfo, "", m_MainScreen)

    DeviceMonitor->>Common: isHwPlatform()
    Common-->>DeviceMonitor: bool
    alt non_HW_platform (!isHwPlatform)
        DeviceMonitor->>DeviceMonitor: setAttribute(mapInfo, Resolution, m_SupportResolution)
    end

    DeviceMonitor->>DeviceMonitor: parse m_ScreenSize into size(width, height)

    DeviceMonitor->>Common: isHwPlatform()
    Common-->>DeviceMonitor: bool
    alt non_HW_platform (!isHwPlatform)
        DeviceMonitor->>DeviceMonitor: split m_SupportResolution into listResolution
        DeviceMonitor->>DeviceMonitor: rebuild m_SupportResolution from listResolution
    end

    DeviceMonitor->>DeviceMonitor: caculateScreenRatio()

    DeviceMonitor->>Common: isHwPlatform()
    Common-->>DeviceMonitor: bool
    alt non_HW_platform (!isHwPlatform)
        DeviceMonitor->>DeviceMonitor: m_SupportResolution.replace(QRegularExpression(", $"), "")
    end

    DeviceMonitor-->>Caller: return
    deactivate DeviceMonitor
Loading

File-Level Changes

Change Details Files
Invert hardware-platform condition so monitor resolution attributes are only populated and post-processed on non-HW platforms.
  • Negated the Common::isHwPlatform() check before assigning the SupportResolution attribute from hwinfo data.
  • Negated the Common::isHwPlatform() check around the logic that rebuilds the support-resolution string from a tokenized list.
  • Negated the Common::isHwPlatform() check before trimming the trailing comma and space from the processed support-resolution string.
deepin-devicemanager/src/DeviceManager/DeviceMonitor.cpp
Tighten smartctl output parsing by broadening allowed characters in the attribute name and value regex groups.
  • Updated the first capture group to allow hyphen and underscore in either order within the character class for the attribute name.
  • Expanded the second capture group to accept digits, word characters, slashes, dashes, and parentheses in both the leading and trailing segments of the value field, correcting escaping where needed.
deepin-devicemanager/src/GenerateDevice/CmdTool.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

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

  • In DeviceMonitor::setInfoFromHwinfo, the repeated if (!Common::isHwPlatform()) blocks around resolution handling suggest the condition may be inverted relative to the method name; consider renaming isHwPlatform or adding a clarifying comment to avoid future misuse.
  • In CmdTool::getMapInfoFromSmartctl, the QRegularExpression is constructed on every iteration and rx.match(line) is called three times; consider constructing the regex once (e.g., static or outside the loop) and storing the match result in a local variable before reusing it.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `DeviceMonitor::setInfoFromHwinfo`, the repeated `if (!Common::isHwPlatform())` blocks around resolution handling suggest the condition may be inverted relative to the method name; consider renaming `isHwPlatform` or adding a clarifying comment to avoid future misuse.
- In `CmdTool::getMapInfoFromSmartctl`, the `QRegularExpression` is constructed on every iteration and `rx.match(line)` is called three times; consider constructing the regex once (e.g., static or outside the loop) and storing the match result in a local variable before reusing it.

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: add-uos

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

@deepin-ci-robot
Copy link

deepin pr auto review

我来对这个diff进行详细的代码审查:

  1. DeviceMonitor.cpp 中的修改:
- if (Common::isHwPlatform()){
+ if (!Common::isHwPlatform()){

这个修改在代码中出现了3次,都是相同的改动。

改进建议:

  1. 代码逻辑:这个改动反转了条件判断的逻辑,需要确认这是否符合实际需求。如果这个改动是正确的,建议:

    • 添加注释说明为什么需要反转这个条件
    • 考虑将这个条件提取为一个单独的变量或函数,避免重复的判断
    • 例如:
    bool shouldProcessResolution = !Common::isHwPlatform();
    if (shouldProcessResolution) {
        // 处理分辨率的代码
    }
  2. CmdTool.cpp 中的修改:

- QRegularExpression rx("^[ ]*[0-9]+[ ]+([\\w-_]+)[ ]+0x[0-9a-fA-F-]+[ ]+[0-9]+[ ]+[0-9]+[ ]+[0-9]+[ ]+[\\w-]+[ ]+[\\w-]+[ ]+[\\w-]+[ ]+([0-9\\/w-]+[ ]*[ 0-9\\/w-()]*)$");
+ QRegularExpression rx("^[ ]*[0-9]+[ ]+([\\w_-]+)[ ]+0x[0-9a-fA-F-]+[ ]+[0-9]+[ ]+[0-9]+[ ]+[0-9]+[ ]+[\\w-]+[ ]+[\\w-]+[ ]+[\\w-]+[ ]+([0-9\\w\\/-]+[ ]*[0-9\\w\\/\\-\\(\\)]*)$");

改进建议:

  1. 正则表达式可读性:

    • 建议将正则表达式提取为常量,并添加注释说明其用途
    • 可以使用原始字符串(raw string)来提高可读性
    • 例如:
    const QString SMARTCTL_REGEX = R"(^[ ]*[0-9]+[ ]+([\w_-]+)[ ]+0x[0-9a-fA-F-]+[ ]+[0-9]+[ ]+[0-9]+[ ]+[0-9]+[ ]+[\w-]+[ ]+[\w-]+[ ]+[\w-]+[ ]+([0-9\w\/-]+[ ]*[0-9\w\/\-\(\)]*)$)";
    QRegularExpression rx(SMARTCTL_REGEX);
  2. 正则表达式性能:

    • 正则表达式编译可能比较耗时,建议将其作为类的成员变量,只编译一次
    • 例如:
    class CmdTool {
    private:
        const QRegularExpression smartctlRegex{SMARTCTL_REGEX};
    };
  3. 正则表达式安全性:

    • 建议对正则表达式进行单元测试,确保它能正确匹配预期的输入
    • 添加边界情况的处理,比如空输入或格式错误的输入
  4. 代码维护性:

    • 建议将复杂的正则表达式拆分成多个部分,并添加注释说明每个部分的用途
    • 可以考虑使用命名捕获组来提高代码的可读性

总体建议:

  1. 对所有修改添加单元测试
  2. 更新相关文档,说明这些修改的原因和影响
  3. 考虑添加更多的日志记录,帮助调试和问题排查
  4. 如果这些修改涉及到重要的业务逻辑,建议进行代码审查并添加更多的注释说明

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