fix: fix icon positioning with device pixel ratio#606
fix: fix icon positioning with device pixel ratio#60618202781743 merged 1 commit intolinuxdeepin:masterfrom
Conversation
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.
| 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.
| qreal dpr = parent->window()->devicePixelRatio(); | |
| qreal dpr = parent->window()->effectiveDevicePixelRatio(); |
There was a problem hiding this comment.
| 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.
| 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.
| 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.
| 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); | |
| } |
| 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.
wjyrich
force-pushed
the
fix-juchi
branch
2 times, most recently
from
e3d3f9f to
6e59361
Compare
| 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.
wjyrich
force-pushed
the
fix-juchi
branch
3 times, most recently
from
91c49a2 to
67a1bb5
Compare
The layout method for DQuickDciIconImage has been completely rewritten to fix positioning issues, particularly on high-DPI displays. Previously, the image was simply centered using anchors, which could cause misalignment. The new implementation calculates precise pixel- aligned positions by considering device pixel ratios and scene transformations. A timer-based scheduling mechanism was added to prevent excessive layout recalculations. Additionally, smooth rendering is now disabled by default during initialization to improve performance, and geometry/parent change handlers ensure proper layout updates. Log: Fixed DCI icon positioning issues on high-DPI displays Influence: 1. Test DCI icon display on monitors with different DPI settings 2. Verify icon centering remains correct when parent item size changes 3. Test icon positioning when moving between windows or screens 4. Verify smooth property is properly initialized to false 5. Test layout updates when icon image dimensions change 6. Verify no performance regression from timer-based layout scheduling fix: 修复 DCI 图标图像定位和布局问题 DQuickDciIconImage 的布局方法已完全重写，以修复定位问题，特别是在高 DPI 显示器上。之前，图像仅使用锚点居中，可能导致错位。新实现通过考虑设备像素 比和场景变换来计算精确的像素对齐位置。添加了基于定时器的调度机制以防止过 多的布局重新计算。此外，初始化期间默认禁用平滑渲染以提高性能，几何/父项 变更处理程序确保正确的布局更新。 Log: 修复高 DPI 显示器上的 DCI 图标定位问题 Influence: 1. 在不同 DPI 设置的显示器上测试 DCI 图标显示 2. 验证父项大小变化时图标居中保持正确 3. 测试在窗口或屏幕间移动时的图标定位 4. 验证 smooth 属性是否正确初始化为 false 5. 测试图标图像尺寸变化时的布局更新 6. 验证基于定时器的布局调度不会导致性能下降
deepin pr auto review这段代码主要修改了 以下是对该代码的审查意见，分为语法逻辑、代码质量、代码性能和代码安全四个方面： 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
总结与修改建议代码片段建议对 修改后的 void DQuickDciIconImagePrivate::layout()
{
Q_Q(DQuickDciIconImage);
if (!q->isComponentComplete())
return;
// 1. 计算理想的居中位置
qreal targetX = (q->width() - imageItem->width()) / 2.0;
qreal targetY = (q->height() - imageItem->height()) / 2.0;
if (!q->parentItem() || !q->window()) {
imageItem->setPosition(QPointF(targetX, targetY));
return;
}
// 2. 映射到场景坐标以处理变换和窗口偏移
QPointF scenePos = q->mapToScene(QPointF(targetX, targetY));
// 3. 获取设备像素比，用于物理像素对齐
qreal dpr = q->window()->effectiveDevicePixelRatio();
// 4. 对齐到物理像素网格，防止模糊
// 显式使用 int 接收 qRound 结果
int physicalX = qRound(scenePos.x() * dpr);
int physicalY = qRound(scenePos.y() * dpr);
// 5. 映射回本地坐标
QPointF localPos = q->mapFromScene(QPointF(physicalX / dpr, physicalY / dpr));
imageItem->setPosition(localPos);
}修改后的 DQuickDciIconImagePrivate::DQuickDciIconImagePrivate(DQuickDciIconImage *qq)
: DObjectPrivate(qq)
{
// 初始化定时器
layoutTimer = new QTimer(qq);
layoutTimer->setSingleShot(true);
layoutTimer->setInterval(50);
QObject::connect(layoutTimer, &QTimer::timeout, qq, [this]() {
layout();
});
// ... 其他初始化代码
}修改后的 void DQuickDciIconImagePrivate::scheduleLayout()
{
if (layoutTimer) {
layoutTimer->start();
}
} |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, 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 |
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: