fix: fix icon positioning with device pixel ratio#606
Open
wjyrich wants to merge 1 commit intolinuxdeepin:masterfrom
Open
fix: fix icon positioning with device pixel ratio#606wjyrich wants to merge 1 commit intolinuxdeepin:masterfrom
wjyrich wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wjyrich 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 |
Reviewer's GuideRefactors DQuickDciIconImage layout from anchor-based centering to a DPR-aware, coordinate-mapping based manual layout triggered via geometry-change-driven deferred timers, to ensure pixel-perfect icon positioning on high-DPI displays without geometry feedback loops. Sequence diagram for deferred DPR-aware icon layout on geometry changessequenceDiagram
participant ParentItem as QQuickItem_parent
participant QQPrivate as QQuickItemPrivate
participant Listener as DQuickDciIconImagePrivate
participant LT as QTimer_layoutTimer
participant IconItem as DQuickIconImage
participant Window as QQuickWindow
participant ContentItem as QQuickItem_contentItem
ParentItem->>QQPrivate: geometryChanged
QQPrivate->>Listener: itemGeometryChanged(item, change, geom)
activate Listener
Listener->>LT: setSingleShot(true), setInterval(100)
Listener->>LT: start()
deactivate Listener
LT-->>Listener: timeout()
activate Listener
Listener->>Listener: layout()
alt parentItem or window missing
Listener-->>Listener: return
else perform_centering
Listener->>IconItem: parentItem = parentItem()
Listener->>ParentItem: width(), height()
Listener->>IconItem: width(), height()
Listener->>Listener: compute targetX, targetY (logical center)
Listener->>Window: window()
Window->>ContentItem: contentItem()
Listener->>ParentItem: mapToItem(ContentItem, targetPoint)
Listener->>Window: devicePixelRatio()
Listener->>Listener: physicalX, physicalY = round(scenePos * dpr)
Listener->>ParentItem: mapFromItem(ContentItem, physicalX / dpr, scenePos.y)
Listener->>ParentItem: mapFromItem(ContentItem, scenePos.x, physicalY / dpr)
Listener->>IconItem: setX(localPosX.x)
Listener->>IconItem: setY(localPosY.y)
end
deactivate Listener
Sequence diagram for component completion and initial deferred layoutsequenceDiagram
participant QmlEngine as QML_Engine
participant Icon as DQuickDciIconImage
participant Private as DQuickDciIconImagePrivate
QmlEngine->>Icon: componentComplete()
activate Icon
Icon->>Private: imageItem->componentComplete()
Icon->>Icon: QQuickItem::componentComplete()
Icon->>Icon: QTimer::singleShot(200, this, lambda)
deactivate Icon
Icon-->>Private: deferred_lambda_invoked_after_200ms
activate Private
Private->>Private: layout()
deactivate Private
Class diagram for updated DQuickDciIconImage layout handlingclassDiagram
class DQuickDciIconImage {
+componentComplete() void
}
class DQuickDciIconImagePrivate {
+DQuickDciIconImagePrivate(qq DQuickDciIconImage*)
+~DQuickDciIconImagePrivate() void
+layout() void
+updateImageSourceUrl() void
+play(mode DQMLGlobalObject_ControlState) void
+itemGeometryChanged(item QQuickItem*, change QQuickGeometryChange, geom QRectF) void
-- attributes --
+palette DDciIconPalette
+imageItem DQuickIconImage*
+mode DQMLGlobalObject_ControlState
+theme DGuiApplicationHelper_ColorType
+fallbackToQIcon bool
+layoutTimer QTimer
}
class DQuickDciIconImageItemPrivate {
+DQuickDciIconImageItemPrivate(parent DQuickDciIconImagePrivate*)
-- attributes --
+controlLayers QRect
+iconPathCache QString
}
class DQuickIconImage {
}
class QQuickItemChangeListener {
<<interface>> QQuickItemChangeListener
+itemGeometryChanged(item QQuickItem*, change QQuickGeometryChange, geom QRectF) void
}
class QTimer {
+setSingleShot(singleShot bool) void
+setInterval(msec int) void
+start() void
}
DQuickDciIconImagePrivate ..> DQuickDciIconImage : q_ptr
DQuickDciIconImagePrivate ..> DQuickIconImage : imageItem
DQuickDciIconImagePrivate ..> QTimer : layoutTimer
DQuickDciIconImageItemPrivate --|> DQuickIconImagePrivate
DQuickDciIconImagePrivate --|> DObjectPrivate
DQuickDciIconImagePrivate --|> QQuickItemChangeListener
DQuickDciIconImage o-- DQuickDciIconImagePrivate
DQuickDciIconImagePrivate o-- DQuickDciIconImageItemPrivate
DQuickDciIconImage ..> QTimer : uses_singleShot_for_deferred_layout
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 2 issues, and left some high level feedback:
- In
itemGeometryChanged, theQQuickItem*,QQuickGeometryChange, andQRectFparameters are unused; consider omitting their names or usingQ_UNUSEDto avoid warnings and make this explicit. - The
layout()method callsmapFromItemtwice to compute X and Y separately; you could simplify and reduce work by mapping a single rounded point(physicalX / dpr, physicalY / dpr)and using that for both coordinates. - There are now two deferred layout mechanisms (the
layoutTimerand theQTimer::singleShot(200, ...)incomponentComplete); consider whether both are necessary or if they can be unified to reduce timing complexity and potential race conditions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `itemGeometryChanged`, the `QQuickItem*`, `QQuickGeometryChange`, and `QRectF` parameters are unused; consider omitting their names or using `Q_UNUSED` to avoid warnings and make this explicit.
- The `layout()` method calls `mapFromItem` twice to compute X and Y separately; you could simplify and reduce work by mapping a single rounded point `(physicalX / dpr, physicalY / dpr)` and using that for both coordinates.
- There are now two deferred layout mechanisms (the `layoutTimer` and the `QTimer::singleShot(200, ...)` in `componentComplete`); consider whether both are necessary or if they can be unified to reduce timing complexity and potential race conditions.
## Individual Comments
### Comment 1
<location path="src/private/dquickdciiconimage.cpp" line_range="181" />
<code_context>
: DObjectPrivate(qq)
, imageItem(new DQuickIconImage(*new DQuickDciIconImageItemPrivate(this), qq))
{
+ QQuickItemPrivate::get(qq)->addItemChangeListener(this, QQuickItemPrivate::Geometry);
+ layoutTimer.setSingleShot(true);
+ layoutTimer.setInterval(100);
</code_context>
<issue_to_address>
**suggestion:** Use consistent change flags when adding and removing the item change listener.
The listener is added with `QQuickItemPrivate::Geometry` but removed with `QQuickItemPrivate::AllChanges`. Even if this works (e.g. if the mask is ignored or `AllChanges` is treated as a superset), it’s clearer and safer to use the same flag for both operations, or to add a short comment explaining why `AllChanges` is required on removal.
Suggested implementation:
```cpp
QQuickItemPrivate::get(qq)->addItemChangeListener(this, QQuickItemPrivate::Geometry);
```
```cpp
QQuickItemPrivate::get(qq)->removeItemChangeListener(this, QQuickItemPrivate::Geometry);
```
1. If there are multiple constructors or initialization paths for this class, ensure they all use `QQuickItemPrivate::Geometry` when calling `addItemChangeListener`.
2. Likewise, verify that every destructor or teardown path that calls `removeItemChangeListener` uses `QQuickItemPrivate::Geometry` to keep the semantics consistent.
3. If for some reason `AllChanges` was originally required (e.g. a known Qt quirk), consider adding a brief comment near these calls to document that, instead of changing the flag.
</issue_to_address>
### Comment 2
<location path="src/private/dquickdciiconimage.cpp" line_range="213" />
<code_context>
+ if (!parent)
+ return;
+
+ qreal targetX = qRound(parent->width() / 2.0) - qRound(imageItem->width() / 2.0);
+ qreal targetY = qRound(parent->height() / 2.0) - qRound(imageItem->height() / 2.0);
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider mapping and rounding the DPR-adjusted position in a single step instead of per-axis mapping.
Rounding and mapping X and Y independently (with mixed coordinates in two `mapFromItem` calls) assumes the transform is separable and can become incorrect when rotation, shear, or non-uniform scaling is involved. Instead, build a single DPR-adjusted `QPointF` in scene coordinates and call `mapFromItem` once, using both components of that result to keep the transform coherent.
Suggested implementation:
```cpp
QQuickItem *parent = imageItem->parentItem();
if (!parent)
return;
// Compute a single DPR-adjusted target position to keep the transform coherent
const qreal targetX = qRound(parent->width() / 2.0 - imageItem->width() / 2.0);
const qreal targetY = qRound(parent->height() / 2.0 - imageItem->height() / 2.0);
const QPointF dprAdjustedTarget(targetX, targetY);
// Map the full point in one go instead of per-axis to avoid mixing coordinate spaces
const QPointF mappedTarget = parent->mapFromItem(imageItem, dprAdjustedTarget);
imageItem->setPosition(mappedTarget);
```
I only see a small portion of `layout()`, so if there is existing code below this that:
1. Separately calls `mapFromItem`/`mapToItem` for X and Y, or
2. Performs additional DPR rounding per-axis,
you should remove or adjust that code to rely on the new `mappedTarget` point and `setPosition()` so that the full transform (including any rotation, shear, or non-uniform scaling) is applied consistently in a single mapping step.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR updates the DCI icon image layout logic to achieve pixel-perfect centering on high-DPI displays by replacing anchor-based centering with DPR-aware coordinate mapping and deferred relayout on geometry changes.
Changes:
- Added a
QQuickItemChangeListenerto detect geometry changes and trigger relayout. - Replaced
anchors()->setCenterIn(...)with explicit pixel-aware positioning using scene/content mapping + DPR rounding. - Introduced a
QTimerto defer layout recalculation and avoid geometry feedback loops.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/private/dquickdciiconimage_p_p.h |
Adds Qt Quick change-listener + timer members to support deferred, geometry-driven layout. |
src/private/dquickdciiconimage.cpp |
Implements deferred layout triggering and DPR-aware centering logic in layout(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+222
to
+225
| qreal dpr = parent->window()->devicePixelRatio(); | ||
| qreal physicalX = std::round(scenePos.x() * dpr); | ||
| qreal physicalY = std::round(scenePos.y() * dpr); | ||
| QPointF localPos = parent->mapFromItem(contentItem, QPointF(physicalX / dpr, physicalY / dpr)); |
There was a problem hiding this comment.
std::round is used here but this translation unit does not include . Relying on indirect includes is brittle and can break compilation on some platforms/toolchains; please add the appropriate standard header (or switch to Qt rounding helpers).
| QQuickItem *contentItem = parent->window()->contentItem(); | ||
| QPointF scenePos = parent->mapToItem(contentItem, QPointF(targetX, targetY)); | ||
|
|
||
| qreal dpr = parent->window()->devicePixelRatio(); |
There was a problem hiding this comment.
This uses QQuickWindow::devicePixelRatio(), but elsewhere in this codebase icon rendering uses QQuickWindow::effectiveDevicePixelRatio() (e.g., src/private/dquickiconimage.cpp:150-154). Using a different DPR source can reintroduce positioning mismatches under additional scale factors; consider using effectiveDevicePixelRatio() here to stay consistent.
Suggested change
| qreal dpr = parent->window()->devicePixelRatio(); | |
| qreal dpr = parent->window()->effectiveDevicePixelRatio(); |
Contributor
There was a problem hiding this comment.
Comment on lines
+213
to
+214
| qreal targetX = qRound(parent->width() / 2.0) - qRound(imageItem->width() / 2.0); | ||
| qreal targetY = qRound(parent->height() / 2.0) - qRound(imageItem->height() / 2.0); |
There was a problem hiding this comment.
targetX/targetY are rounded in logical coordinates and then the mapped scene position is rounded again in physical pixels. This double-rounding can shift the center by 1px (especially for non-integer DPR like 1.5). To keep “pixel-perfect” behavior, compute the center using full-precision qreal values and only round once in physical pixel space.
Suggested change
| qreal targetX = qRound(parent->width() / 2.0) - qRound(imageItem->width() / 2.0); | |
| qreal targetY = qRound(parent->height() / 2.0) - qRound(imageItem->height() / 2.0); | |
| qreal targetX = (parent->width() - imageItem->width()) / 2.0; | |
| qreal targetY = (parent->height() - imageItem->height()) / 2.0; |
| { | ||
| QQuickItemPrivate::get(qq)->addItemChangeListener(this, QQuickItemPrivate::Geometry); | ||
| layoutTimer.setSingleShot(true); | ||
| layoutTimer.setInterval(100); |
There was a problem hiding this comment.
layoutTimer is configured with a hard-coded 100ms delay. This means the initial layout in componentComplete() and subsequent resize-driven relayouts can visibly “lag” by up to 100ms, leaving the icon temporarily mispositioned. Consider using a 0ms single-shot (event-loop coalescing) or triggering an immediate layout() and only deferring subsequent updates.
Suggested change
| layoutTimer.setInterval(100); | |
| layoutTimer.setInterval(0); |
| qreal targetX = qRound(parent->width() / 2.0) - qRound(imageItem->width() / 2.0); | ||
| qreal targetY = qRound(parent->height() / 2.0) - qRound(imageItem->height() / 2.0); | ||
|
|
||
| if (!parent->window() || !parent->window()->contentItem()) { |
There was a problem hiding this comment.
layout() bails out when window()/contentItem() is not available, and the only triggers are the geometry listener and the one-shot timer started in componentComplete(). If the item gets a window after this initial attempt (without a geometry change), layout may never run. Consider restarting layoutTimer (or calling layout) from QQuickItem::windowChanged as well.
Suggested change
| if (!parent->window() || !parent->window()->contentItem()) { | |
| if (!parent->window() || !parent->window()->contentItem()) { | |
| static const char kWindowChangedConnectionPendingProperty[] = "_d_dciIconImageWindowChangedConnectionPending"; | |
| if (!parent->property(kWindowChangedConnectionPendingProperty).toBool()) { | |
| parent->setProperty(kWindowChangedConnectionPendingProperty, true); | |
| QObject::connect(parent, &QQuickItem::windowChanged, q_func(), | |
| [this, parent] { | |
| parent->setProperty("_d_dciIconImageWindowChangedConnectionPending", false); | |
| layoutTimer.start(); | |
| }, | |
| Qt::SingleShotConnection); | |
| } |
Comment on lines
+202
to
+205
| void DQuickDciIconImagePrivate::itemGeometryChanged(QQuickItem *, QQuickGeometryChange, const QRectF &) | ||
| { | ||
| layoutTimer.start(); | ||
| } |
There was a problem hiding this comment.
Relayout is only scheduled on the wrapper item's geometry changes. With anchors removed, changes in imageItem size (e.g., when the source loads, sourceSize changes, or implicit size updates) will no longer automatically re-center the icon. Consider also triggering layoutTimer when imageItem width/height (or implicit size) changes to keep centering correct.
Changed the layout mechanism for DCI icon images to properly handle device pixel ratio and ensure pixel-perfect positioning. Previously used anchors for centering which didn't account for DPR scaling, causing blurry or misaligned icons on high-DPI displays. Now calculates physical pixel positions and maps between coordinate systems to maintain crisp rendering. The implementation adds a QTimer for deferred layout to avoid geometry change loops, and includes a QQuickItemChangeListener to detect geometry changes. The layout() method now computes exact pixel positions by considering window devicePixelRatio and mapping between scene and local coordinates. Log: Fixed blurry icon rendering on high-DPI displays Influence: 1. Test icon rendering on displays with different DPR values (1.0, 1.5, 2.0, etc.) 2. Verify icons remain sharp and properly centered when resizing windows 3. Test icon positioning in various container sizes and aspect ratios 4. Verify performance with multiple icons during window geometry changes 5. Test theme switching and palette changes to ensure positioning stability fix: 修复图标在高DPI显示下的定位问题 更改了DCI图标图像的布局机制，以正确处理设备像素比并确保像素级精确定位。 之前使用锚点进行居中，未考虑DPR缩放，导致在高DPI显示器上图标模糊或错位。 现在通过计算物理像素位置并在坐标系之间映射来保持清晰渲染。 实现添加了QTimer用于延迟布局以避免几何变化循环，并包含 QQuickItemChangeListener来检测几何变化。layout()方法现在通过考虑窗口 devicePixelRatio并在场景和本地坐标之间映射来计算精确的像素位置。 Log: 修复了高DPI显示器上图标渲染模糊的问题 Influence: 1. 在不同DPR值（1.0、1.5、2.0等）的显示器上测试图标渲染 2. 验证调整窗口大小时图标保持清晰和正确居中 3. 测试各种容器大小和宽高比下的图标定位 4. 验证窗口几何变化期间多个图标的性能表现 5. 测试主题切换和调色板更改以确保定位稳定性
Contributor
deepin pr auto review这段代码主要对 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
综合改进建议
总结： |
18202781743
reviewed
Apr 16, 2026
| #include <DPlatformTheme> | ||
|
|
||
| #include <QUrlQuery> | ||
| #include <qpoint.h> |
| QQuickItem *contentItem = parent->window()->contentItem(); | ||
| QPointF scenePos = parent->mapToItem(contentItem, QPointF(targetX, targetY)); | ||
|
|
||
| qreal dpr = parent->window()->devicePixelRatio(); |
Contributor
There was a problem hiding this comment.
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
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.
Changed the layout mechanism for DCI icon images to properly handle device pixel ratio and ensure pixel-perfect positioning. Previously used anchors for centering which didn't account for DPR scaling, causing blurry or misaligned icons on high-DPI displays. Now calculates physical pixel positions and maps between coordinate systems to maintain crisp rendering.
The implementation adds a QTimer for deferred layout to avoid geometry change loops, and includes a QQuickItemChangeListener to detect geometry changes. The layout() method now computes exact pixel positions by considering window devicePixelRatio and mapping between scene and local coordinates.
Log: Fixed blurry icon rendering on high-DPI displays
Influence:
fix: 修复图标在高DPI显示下的定位问题
更改了DCI图标图像的布局机制，以正确处理设备像素比并确保像素级精确定位。
之前使用锚点进行居中，未考虑DPR缩放，导致在高DPI显示器上图标模糊或错位。
现在通过计算物理像素位置并在坐标系之间映射来保持清晰渲染。
实现添加了QTimer用于延迟布局以避免几何变化循环，并包含
QQuickItemChangeListener来检测几何变化。layout()方法现在通过考虑窗口 devicePixelRatio并在场景和本地坐标之间映射来计算精确的像素位置。
Log: 修复了高DPI显示器上图标渲染模糊的问题
Influence:
Summary by Sourcery
Improve DCI icon layout to achieve pixel-perfect centering on high-DPI displays.
Bug Fixes:
Enhancements:
Tests: