Skip to content

Conversation

@GongHeng2017
Copy link
Contributor

@GongHeng2017 GongHeng2017 commented Nov 6, 2025

-- code logic error.
-- Not judge the speciall platform success.

Log: fix issue
Bug:

Summary by Sourcery

Replace ad-hoc numeric specialComType checks with a unified isHwPlatform() method to correctly control display logic for memory, storage, and monitor tables and fix the memory list not showing on hardware platforms.

Bug Fixes:

  • Fix memory list display issue by correcting the platform check condition.

Enhancements:

  • Use Common::isHwPlatform() instead of direct Common::specialComType comparisons in PageMultiInfo, DeviceMonitor, and DeviceCpu.
  • Replace magic number check for special task (type 7) with Common::kSpecialType7 in EDIDParser.
  • Reformat multi-line QString construction for screen size output in EDIDParser.

-- code logic error.
-- Not judge the speciall platform success.

Log: fix issue
Bug:
@sourcery-ai
Copy link

sourcery-ai bot commented Nov 6, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

This PR fixes the memory list visibility issue by correcting platform detection logic across multiple components—replacing raw specialComType checks with Common::isHwPlatform(), refactoring table update/resizing conditions, and updating EDIDParser to use a named constant with clearer string formatting.

Sequence diagram for memory list visibility update in PageMultiInfo

sequenceDiagram
participant PageMultiInfo
participant Common
participant mp_Table
participant mp_Label
PageMultiInfo->>Common: isHwPlatform()
alt isHwPlatform() == true
    PageMultiInfo->>mp_Label: text()
    alt text is "Storage" or "Memory" or "Monitor"
        PageMultiInfo->>mp_Table: setVisible(false)
        PageMultiInfo->>mp_Table: setFixedHeight(0)
    else text is other
        PageMultiInfo->>mp_Table: setVisible(true)
        PageMultiInfo->>mp_Table: setFixedHeight(TABLE_HEIGHT)
        PageMultiInfo->>mp_Table: updateTable(...)
    end
else isHwPlatform() == false
    PageMultiInfo->>mp_Table: setVisible(true)
    PageMultiInfo->>mp_Table: setFixedHeight(TABLE_HEIGHT)
    PageMultiInfo->>mp_Table: updateTable(...)
end
Loading

Class diagram for updated platform detection logic

classDiagram
class Common {
  +specialComType : int
  +kSpecialType7 : int
  +isHwPlatform() : bool
}
class PageMultiInfo {
  +updateInfo(lst : QList<DeviceBaseInfo*>)
  +resizeEvent(e : QResizeEvent*)
  -mp_Table
  -mp_Label
  -m_deviceList
  -m_menuControlList
}
class DeviceMonitor {
  +setInfoFromHwinfo(mapInfo : QMap<QString, QString>)
  +available() : bool
  +subTitle() : QString
  -m_DisplayInput
  -m_ScreenSize
  -m_MainScreen
  -m_SupportResolution
  -m_Name
}
class DeviceCpu {
  +setInfoFromDmidecode(mapInfo : QMap<QString, QString>)
  -m_Name
}
class EDIDParser {
  +parseScreenSize()
  +parseMonitorName()
  -m_Width
  -m_Height
  -m_ScreenSize
}
Common <|-- PageMultiInfo
Common <|-- DeviceMonitor
Common <|-- DeviceCpu
Common <|-- EDIDParser
Loading

File-Level Changes

Change Details Files
Unified hardware platform detection
  • Replace Common::specialComType comparisons with Common::isHwPlatform() in PageMultiInfo updateInfo
  • Adjust resizeEvent branches in PageMultiInfo to use isHwPlatform() and simplify fallback updateTable calls
  • Swap specialComType checks for isHwPlatform() in DeviceMonitor setInfoFromHwinfo, resolution parsing, and subTitle()
  • Replace specialComType check with isHwPlatform() in DeviceCpu setInfoFromDmidecode
src/Page/PageMultiInfo.cpp
src/DeviceManager/DeviceMonitor.cpp
src/DeviceManager/DeviceCpu.cpp
Refactor EDIDParser special type handling and formatting
  • Use Common::kSpecialType7 instead of hardcoded literal 7
  • Refactor QString::arg chain into a multi-line call for readability
src/Tool/EDIDParser.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 there - I've reviewed your changes - here's some feedback:

  • The nested if/else branches in resizeEvent introduce a lot of duplicated updateTable calls—consider extracting the common logic or inverting the conditions to simplify and DRY up the code.
  • Repeated string comparisons against mp_Label->text() for "Storage", "Memory", and "Monitor" could be consolidated into a helper function or constant set to reduce literal duplication and improve clarity.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The nested if/else branches in resizeEvent introduce a lot of duplicated updateTable calls—consider extracting the common logic or inverting the conditions to simplify and DRY up the code.
- Repeated string comparisons against mp_Label->text() for "Storage", "Memory", and "Monitor" could be consolidated into a helper function or constant set to reduce literal duplication and improve clarity.

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

deepin pr auto review

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

  1. 代码改进点分析:
  • 主要改动是将 Common::specialComType 的直接比较替换为 Common::isHwPlatform() 函数调用
  • 在 EDIDParser.cpp 中引入了 Common::kSpecialType7 常量替换硬编码的数字 7
  1. 优点:
  • 代码可读性提升:使用函数 isHwPlatform() 比直接比较 specialComType > 0 更直观
  • 代码维护性增强:使用命名常量 kSpecialType7 替代魔法数字 7
  • 统一了条件判断逻辑,减少了代码重复
  1. 建议改进:

a) 代码安全方面:

  • 建议在 Common::isHwPlatform() 函数中添加参数验证
  • 对于 specialComType 的比较应该考虑使用枚举类型而不是整数

b) 代码质量方面:

  • 建议在 EDIDParser.cpp 中将特殊类型的判断逻辑封装成单独的函数
  • 可以考虑使用策略模式来处理不同特殊类型的逻辑

c) 代码性能方面:

  • 虽然改动很小,但频繁调用 isHwPlatform() 可能会有轻微性能影响,建议在关键路径上缓存结果

d) 具体修改建议:

// Common.h 中建议添加
enum class SpecialType {
    Normal = 0,
    Type7 = 7
    // 其他特殊类型...
};

class Common {
public:
    static bool isHwPlatform() {
        return specialComType > static_cast<int>(SpecialType::Normal);
    }
    
    static bool isSpecialType7() {
        return specialComType == static_cast<int>(SpecialType::Type7);
    }
};

// EDIDParser.cpp 中修改
void EDIDParser::parseScreenSize()
{
    // ... 其他代码 ...
    
    if (Common::isSpecialType7()) {
        m_Width = 296;
        m_Height = 197;
    }
    
    double inch = calculateScreenInch();
    int precision = Common::isSpecialType7() ? 0 : 1;
    m_ScreenSize = formatScreenSize(inch, precision);
}

private:
    double calculateScreenInch() {
        return sqrt((m_Width / 2.54) * (m_Width / 2.54) + 
                   (m_Height / 2.54) * (m_Height / 2.54))/10;
    }
    
    QString formatScreenSize(double inch, int precision) {
        return QString("%1 %2(%3mm×%4mm)")
               .arg(QString::number(inch, '0', precision))
               .arg(QObject::tr("inch"))
               .arg(m_Width)
               .arg(m_Height);
    }
  1. 其他建议:
  • 添加单元测试来验证特殊类型的处理逻辑
  • 考虑添加日志记录,方便调试特殊类型的处理流程
  • 建议在代码注释中说明特殊类型的具体用途和业务逻辑

这些改进可以提高代码的可维护性、可读性和安全性,同时保持代码的性能。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: add-uos, GongHeng2017, max-lvs

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

@GongHeng2017
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Nov 6, 2025

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit 29ffb3d into linuxdeepin:develop/eagle Nov 6, 2025
16 of 18 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.

4 participants