Fix: the screen size error.#626
Conversation
Reviewer's GuideRefactors EDID screen size parsing to compute screen dimensions from all Detailed Timing Descriptors (DTDs), choose the descriptor closest to the base 15H/16H size, and fall back to the base size when no close match exists, introducing reusable DTD parsing helpers and a DTD size info struct. Class diagram for updated EDIDParser and DTDSizeInfoclassDiagram
class DTDSizeInfo {
int dtdIndex
int width
int height
bool isBaseBlock
DTDSizeInfo()
}
class EDIDParser {
// key fields
int m_Width
int m_Height
QStringList m_ListEdid
QMap~QString,QString~ m_MapCh
QVector~DTDSizeInfo~ m_DTDSizeInfoList
bool m_LittleEndianMode
// relevant methods
void parseScreenSize()
void parseMonitorName()
void parseDTDs()
void parseOneDTD(int startByte, int dtdIndex, bool isBaseBlock)
QString binToDec(QString strBin)
QString hexToDec(QString strHex)
QString getBytes(int line, int byteInLine)
}
EDIDParser --> "*" DTDSizeInfo : aggregates
Flow diagram for updated parseScreenSize logicflowchart TD
A[Start parseScreenSize] --> B[Call parseDTDs to fill m_DTDSizeInfoList]
B --> C[Read width15 from EDID byte 15H and convert to mm]
C --> D[Read height16 from EDID byte 16H and convert to mm]
D --> E[Initialize rightWidth = -1 and rightHeight = -1]
E --> F{Iterate each DTDSizeInfo in m_DTDSizeInfoList}
F -->|More DTDSizeInfo| G{Is width and height within 10mm of width15 and height16}
G -->|Yes| H[Set rightWidth and rightHeight from this DTDSizeInfo and break loop]
G -->|No| F
F -->|No more DTDSizeInfo| I{rightWidth == -1 and rightHeight == -1}
H --> I
I -->|True| J[Set m_Width = width15 and m_Height = height16]
I -->|False| K[Set m_Width = rightWidth and m_Height = rightHeight]
J --> L[Apply specialComType adjustment to m_Width and m_Height if needed]
K --> L
L --> M[Compute inch from m_Width and m_Height]
M --> N[Format m_ScreenSize string]
N --> O[End parseScreenSize]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider keeping
parseDTDsandparseOneDTDprivate (and potentially nestingDTDSizeInfoinsideEDIDParser) to avoid exposing internal parsing details that are only used as implementation helpers. - In
parseScreenSize, you only fall back to the base size when bothrightWidthandrightHeightremain-1; if one dimension matches the base size tolerance and the other does not, the mismatch is silently ignored—clarify whether this is intentional or adjust the logic to handle partial matches more explicitly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider keeping `parseDTDs` and `parseOneDTD` private (and potentially nesting `DTDSizeInfo` inside `EDIDParser`) to avoid exposing internal parsing details that are only used as implementation helpers.
- In `parseScreenSize`, you only fall back to the base size when *both* `rightWidth` and `rightHeight` remain `-1`; if one dimension matches the base size tolerance and the other does not, the mismatch is silently ignored—clarify whether this is intentional or adjust the logic to handle partial matches more explicitly.
## Individual Comments
### Comment 1
<location path="deepin-devicemanager/src/Tool/EDIDParser.cpp" line_range="207-214" />
<code_context>
- m_Width = ((byte68_val & 0xF0) << 4) + hexToDec(s66).toInt();
- // 高度 = (byte68低4位 << 8) + s67
- m_Height = ((byte68_val & 0x0F) << 8) + hexToDec(s67).toInt();
+ parseDTDs();
+ int rightWidth = -1;
+ int rightHeight = -1;
+ // edid中的 15H和16H是基本屏幕大小(单位cm), 与 Detailed Timing 相差超10mm 则用15H和16H的。
+ int width15 = hexToDec(getBytes(1, m_LittleEndianMode ? 5 : 4)).toInt() * 10;
+ int height16 = hexToDec(getBytes(1, m_LittleEndianMode ? 6 : 7)).toInt() * 10;
+ // 从所有的详细尺寸中寻找接近基本屏幕大小的值
+ for (int i = 0; i < m_DTDSizeInfoList.size(); ++i) {
+ if (abs(m_DTDSizeInfoList.at(i).width - width15) < 10
+ && abs(m_DTDSizeInfoList.at(i).height - height16) < 10) {
</code_context>
<issue_to_address>
**issue:** Guard against empty or zero-valued DTD size entries when deriving the final screen size.
Because `parseScreenSize` now relies entirely on `m_DTDSizeInfoList`, cases where DTDs (or 15H/16H) report 0 for width/height can lead to `m_Width/m_Height` being set to 0, yielding a 0-inch screen. To avoid this, filter out DTD entries with width/height == 0 when populating `m_DTDSizeInfoList` or before the selection loop, and similarly ignore 0-valued 15H/16H when matching or falling back.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| parseDTDs(); | ||
| int rightWidth = -1; | ||
| int rightHeight = -1; | ||
| // edid中的 15H和16H是基本屏幕大小(单位cm), 与 Detailed Timing 相差超10mm 则用15H和16H的。 | ||
| int width15 = hexToDec(getBytes(1, m_LittleEndianMode ? 5 : 4)).toInt() * 10; | ||
| int height16 = hexToDec(getBytes(1, m_LittleEndianMode ? 6 : 7)).toInt() * 10; | ||
| // 从所有的详细尺寸中寻找接近基本屏幕大小的值 | ||
| for (int i = 0; i < m_DTDSizeInfoList.size(); ++i) { |
There was a problem hiding this comment.
issue: Guard against empty or zero-valued DTD size entries when deriving the final screen size.
Because parseScreenSize now relies entirely on m_DTDSizeInfoList, cases where DTDs (or 15H/16H) report 0 for width/height can lead to m_Width/m_Height being set to 0, yielding a 0-inch screen. To avoid this, filter out DTD entries with width/height == 0 when populating m_DTDSizeInfoList or before the selection loop, and similarly ignore 0-valued 15H/16H when matching or falling back.
-- Get all screen size and judge it by base screen size. Log: fix issue Bug: https://pms.uniontech.com/bug-view-354275.html
53b1b55 to
b0fb9cc
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: GongHeng2017, lzwind The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/forcemerge |
-- Get all screen size and judge it by base screen size.
Log: fix issue
Bug: https://pms.uniontech.com/bug-view-354275.html
Summary by Sourcery
Improve EDID screen size parsing to select the most accurate dimensions based on detailed timing descriptors and base size fields.
Bug Fixes:
Enhancements: