Fix: the screen size error.#625
Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:develop/eaglefrom Mar 26, 2026
Merged
Conversation
Reviewer's GuideRefactors EDID screen size parsing to gather physical dimensions from all Detailed Timing Descriptors (DTDs), store them in a new DTDSizeInfo list, and then choose the final screen size by comparing each DTD size against the base block 15H/16H size, while also cleaning up headers and adding supporting data structures. Sequence diagram for EDIDParser screen size calculationsequenceDiagram
participant Caller
participant EDIDParser
participant DTDSizeInfoList as m_DTDSizeInfoList
Caller->>EDIDParser: parseScreenSize()
activate EDIDParser
EDIDParser->>EDIDParser: parseDTDs()
activate DTDSizeInfoList
loop for i in 0..3
EDIDParser->>EDIDParser: parseOneDTD(startByte, i, true)
EDIDParser->>DTDSizeInfoList: append(DTDSizeInfo)
end
deactivate DTDSizeInfoList
EDIDParser->>EDIDParser: read width15, height16 from base block
loop for each info in m_DTDSizeInfoList
EDIDParser->>EDIDParser: m_Width = info.width
EDIDParser->>EDIDParser: m_Height = info.height
EDIDParser->>EDIDParser: compare with width15, height16
alt difference > 10mm
EDIDParser->>EDIDParser: m_Width = width15
EDIDParser->>EDIDParser: m_Height = height16
end
end
deactivate EDIDParser
EDIDParser-->>Caller: return
Class diagram for updated EDIDParser screen size parsingclassDiagram
class EDIDParser {
- int m_Width
- int m_Height
- QStringList m_ListEdid
- QMap~QString,QString~ m_MapCh
- QVector~DTDSizeInfo~ m_DTDSizeInfoList
+ 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)
}
class DTDSizeInfo {
int dtdIndex
int width
int height
bool isBaseBlock
DTDSizeInfo()
}
EDIDParser "1" --> "*" DTDSizeInfo : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
parseScreenSize, whenm_DTDSizeInfoListis empty (no valid DTDs),m_Width/m_Heightremain uninitialized and you never fall back to the 15H/16H values as before; consider explicitly usingwidth15/height16when there are no DTD entries. - The loop in
parseScreenSizesetsm_Width/m_Heightfor each DTD and then possibly overrides them with the base 15H/16H size, so the final values depend only on the last DTD and any single DTD with a >10mm difference forces the base size; if the intent is to choose the best-matching DTD, consider computing all differences first and selecting the closest (or the first valid) instead of overwriting in a loop.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `parseScreenSize`, when `m_DTDSizeInfoList` is empty (no valid DTDs), `m_Width`/`m_Height` remain uninitialized and you never fall back to the 15H/16H values as before; consider explicitly using `width15`/`height16` when there are no DTD entries.
- The loop in `parseScreenSize` sets `m_Width`/`m_Height` for each DTD and then possibly overrides them with the base 15H/16H size, so the final values depend only on the last DTD and any single DTD with a >10mm difference forces the base size; if the intent is to choose the best-matching DTD, consider computing all differences first and selecting the closest (or the first valid) instead of overwriting in a loop.
## Individual Comments
### Comment 1
<location path="deepin-devicemanager/src/Tool/EDIDParser.cpp" line_range="203-198" />
<code_context>
+ // 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) {
+ m_Width = m_DTDSizeInfoList.at(i).width;
+ m_Height = m_DTDSizeInfoList.at(i).height;
+
+ if (abs(m_Width - width15) > 10 || abs(m_Height - height16) > 10) {
+ m_Width = width15;
+ m_Height = height16;
+ }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Loop logic can yield unexpected final width/height and does nothing when there are no DTDs.
Currently each iteration overwrites `m_Width`/`m_Height`, and only the last DTD determines whether `width15`/`height16` are used.
1) When `m_DTDSizeInfoList` is empty, `m_Width`/`m_Height` are never updated here and may retain stale/uninitialized values; `width15`/`height16` are never applied.
2) When DTDs vary around `width15`/`height16`, the outcome depends solely on the last element rather than a deliberate choice (e.g. best match or first valid one).
If the goal is to prefer DTD sizes when available and fall back to bytes 0x15/0x16 only when no DTD is within 10 mm, it would be safer to:
- Handle the empty `m_DTDSizeInfoList` case explicitly (e.g. use `width15`/`height16`).
- Pick a single DTD candidate (e.g. first base-block DTD or closest to `width15`/`height16`) and apply the tolerance check once, instead of per-iteration overwrites.
</issue_to_address>
### Comment 2
<location path="deepin-devicemanager/src/Tool/EDIDParser.cpp" line_range="207-205" />
<code_context>
+ m_Width = m_DTDSizeInfoList.at(i).width;
+ m_Height = m_DTDSizeInfoList.at(i).height;
+
+ if (abs(m_Width - width15) > 10 || abs(m_Height - height16) > 10) {
+ m_Width = width15;
+ m_Height = height16;
+ }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Fallback to 0x15/0x16 can override valid DTD sizes even when 0x15/0x16 are zero or clearly worse.
This changes the fallback behavior for 0x15/0x16: the old code only replaced DTD sizes when the basic-block dimensions were clearly larger (`DTD + 10 < basic`), so 0 or missing 0x15/0x16 values would not override valid DTD data. With the new `abs(...) > 10` checks, 0x15/0x16 values of 0 will almost always trigger, replacing valid DTD sizes with 0. For devices that legitimately report 0 here (e.g., some TVs/projectors), this is a regression.
Consider explicitly ignoring zero/obviously-invalid 0x15/0x16 values (and constraining to a reasonable range), and/or restoring the previous one-sided tolerance if that precedence was intentional, so the DTD remains the primary source when it’s valid.
</issue_to_address>
### Comment 3
<location path="deepin-devicemanager/src/Tool/EDIDParser.cpp" line_range="180" />
<code_context>
void EDIDParser::parseScreenSize()
{
-
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a helper to read EDID bytes by absolute index and simplifying parseScreenSize to use a single chosen DTD entry with clear fallback to 15H/16H values.
You can keep the new DTD-based parsing while cutting quite a bit of complexity and fixing the control flow in `parseScreenSize()`.
### 1. Replace manual line/byte math with a small helper
All the `startByte / 16` and `% 16` computations in `parseOneDTD` obscure the intent and are easy to get wrong. You can centralise this into a helper that reads a single EDID byte by absolute index, including the endianness handling:
```cpp
// in EDIDParser
int EDIDParser::readEdidByte(int absIndex) const
{
int line = absIndex / 16;
int byteInLine = absIndex % 16;
QString s = getBytes(line, byteInLine);
return s.isEmpty() ? -1 : hexToDec(s).toInt();
}
```
Then `parseOneDTD` becomes simpler and more obviously correct:
```cpp
void EDIDParser::parseOneDTD(int startByte, int dtdIndex, bool isBaseBlock)
{
// pixel clock at bytes 0–1 within the DTD
int pixelClockByte0Index = m_LittleEndianMode ? startByte : startByte + 1;
int pixelClockByte1Index = m_LittleEndianMode ? startByte + 1 : startByte;
int b0 = readEdidByte(pixelClockByte0Index);
int b1 = readEdidByte(pixelClockByte1Index);
if (b0 < 0 || b1 < 0)
return;
int pixelClock = b0 + (b1 << 8);
if (pixelClock == 0)
return;
int byte12Index = m_LittleEndianMode ? (startByte + 12) : (startByte + 13);
int byte13Index = m_LittleEndianMode ? (startByte + 13) : (startByte + 12);
int byte14Index = m_LittleEndianMode ? (startByte + 14) : (startByte + 15);
int b12 = readEdidByte(byte12Index);
int b13 = readEdidByte(byte13Index);
int b14 = readEdidByte(byte14Index);
if (b12 < 0 || b13 < 0 || b14 < 0)
return;
int width = ((b14 & 0xF0) << 4) + b12;
int height = ((b14 & 0x0F) << 8) + b13;
DTDSizeInfo info;
info.dtdIndex = dtdIndex;
info.width = width;
info.height = height;
info.isBaseBlock = isBaseBlock;
m_DTDSizeInfoList.append(info);
}
```
This keeps all functionality, but removes most of the repeated line/column arithmetic.
### 2. Simplify `parseScreenSize` control flow
`parseScreenSize` only needs one width/height, but it currently loops over `m_DTDSizeInfoList`, overwriting `m_Width`/`m_Height` each time and finally keeping the last one (possibly snapped to 15H/16H). That’s hard to reason about and not clearly intentional.
If you only care about “some valid DTD” for size, you can use the first valid entry and do a single comparison against 15H/16H:
```cpp
void EDIDParser::parseScreenSize()
{
parseDTDs();
int width15 = hexToDec(getBytes(1, m_LittleEndianMode ? 5 : 4)).toInt() * 10;
int height16 = hexToDec(getBytes(1, m_LittleEndianMode ? 6 : 7)).toInt() * 10;
// default to 15H/16H
m_Width = width15;
m_Height = height16;
if (!m_DTDSizeInfoList.isEmpty()) {
const DTDSizeInfo &dtd = m_DTDSizeInfoList.first();
// only use DTD if it’s close enough to 15H/16H
if (std::abs(dtd.width - width15) <= 10 &&
std::abs(dtd.height - height16) <= 10) {
m_Width = dtd.width;
m_Height = dtd.height;
}
}
if (Common::specialComType == Common::kSpecialType7) { // sepcial task:378963
m_Width = 296;
m_Height = 197;
}
double inch = sqrt((m_Width / 2.54) * (m_Width / 2.54) +
(m_Height / 2.54) * (m_Height / 2.54)) / 10;
m_ScreenSize = QString("%1 %2(%3mm×%4mm)")
.arg(QString::number(inch, '0',
Common::specialComType == Common::kSpecialType7 ? 0 : 1))
.arg(QObject::tr("inch")).arg(m_Width).arg(m_Height);
}
```
This:
- avoids looping just to overwrite `m_Width`/`m_Height` repeatedly,
- makes the “fallback to 15H/16H” intent explicit, and
- still uses the DTD parsing infrastructure.
If later you need “best DTD by some criteria” (e.g. max pixel clock), you can encapsulate that into a small selector function that returns one `DTDSizeInfo`, keeping `parseScreenSize()` simple.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
GongHeng2017
force-pushed
the
202603261653-dev-eagle-fix
branch
3 times, most recently
from
4e9acd1 to
f99f92d
Compare
add-uos
approved these changes
Mar 26, 2026
GongHeng2017
force-pushed
the
202603261653-dev-eagle-fix
branch
2 times, most recently
from
9bd6465 to
1823d0d
Compare
-- Get all screen size and judge it by base screen size. Log: fix issue Bug: https://pms.uniontech.com/bug-view-354275.html
GongHeng2017
force-pushed
the
202603261653-dev-eagle-fix
branch
from
1823d0d to
d3a2ea2
Compare
max-lvs
approved these changes
Mar 26, 2026
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/forecemerge |
Contributor
Author
|
/forcemerge |
Contributor
|
This pr force merged! (status: unstable) |
deepin-bot
bot
merged commit 1bc7b4a
into
linuxdeepin:develop/eagle
20 of 22 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
-- 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 parsing to derive screen size from all Detailed Timing Descriptors and reconcile it against the base block size fields to avoid incorrect screen size detection.
Bug Fixes:
Enhancements: