sync: from linuxdeepin/dde-session-shell#489
Merged
yixinshark merged 1 commit intomasterfrom Feb 26, 2026
Merged
Conversation
Synchronize source files from linuxdeepin/dde-session-shell. Source-pull-request: linuxdeepin/dde-session-shell#58
Contributor
Author
deepin pr auto review这份代码 diff 主要针对多屏管理和全屏背景窗口在 X11/Wayland 混合环境下的位置同步问题进行了修复和增强。代码通过引入 XCB 直接查询 X Server 的窗口几何信息，并增加了详细的日志记录和强制刷新逻辑。 以下是对该 diff 的详细审查意见： 1. 语法与逻辑
2. 代码质量
3. 代码性能
4. 代码安全
总结与改进建议这段代码有效地解决了 X11 环境下 Qt 窗口几何信息与 X Server 实际状态不一致的问题，通过引入 XCB 底层查询和强制刷新机制，增强了系统的鲁棒性。 主要改进建议：
修正后的坐标计算逻辑示例（假设 XCB 返回物理坐标）： // 假设 xcb_get_geometry 返回的是物理像素坐标
QRect xcbGeometry(reply->x, reply->y, reply->width, reply->height);
const qreal dpr = frame->devicePixelRatioF();
// 将 Qt 的逻辑几何转换为物理几何进行对比
QRect expectedPhysical(qRound(screen->geometry().x() * dpr),
qRound(screen->geometry().y() * dpr),
qRound(screen->geometry().width() * dpr),
qRound(screen->geometry().height() * dpr));如果实际情况确实是注释所说的“位置不乘DPR”，则请保留原样，但务必加上充分的单元测试或集成测试来覆盖多屏、高分屏场景。 |
yixinshark
approved these changes
Feb 26, 2026
Contributor
Author
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deepin-ci-robot, yixinshark 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 GuideAdds XCB/X11-based validation and detailed logging around fullscreen background and lock frame geometries to detect and correct X11/Qt geometry mismatches, ensuring lock screens bind to the correct screen and have accurate positions/sizes, especially on multi‑screen X11 setups. Sequence diagram for geometry reset and XCB validation in FullScreenBackgroundsequenceDiagram
participant Timer as ResetGeometryTimer
participant FullScreenBackground
participant QtWindow as QWindow_handle
participant QtScreen as QScreen
participant X11Conn as XCB_Connection
participant XServer
Timer->>FullScreenBackground: timeout()
activate FullScreenBackground
FullScreenBackground->>FullScreenBackground: currentGeometry = geometry()
FullScreenBackground->>FullScreenBackground: compare currentGeometry vs m_geometryRect
alt Qt_geometry_mismatch
FullScreenBackground->>FullScreenBackground: qCWarning(DDE_SHELL, geometry mismatch)
FullScreenBackground->>FullScreenBackground: setGeometry(m_geometryRect)
end
FullScreenBackground->>FullScreenBackground: if !m_model->isUseWayland() && windowHandle()
alt X11_backend
FullScreenBackground->>QtWindow: windowHandle()
QtWindow-->>FullScreenBackground: QWindow*
FullScreenBackground->>X11Conn: get X11 connection (QX11Info or QX11Application)
alt connection_available
X11Conn-->>FullScreenBackground: xcb_connection_t*
FullScreenBackground->>X11Conn: xcb_get_geometry(winId())
X11Conn->>XServer: query window geometry
XServer-->>X11Conn: xcb_get_geometry_reply
X11Conn-->>FullScreenBackground: reply(x, y, width, height)
FullScreenBackground->>FullScreenBackground: build xcbGeometry from reply
FullScreenBackground->>FullScreenBackground: dpr = devicePixelRatioF()
FullScreenBackground->>FullScreenBackground: expectedPhysical from m_geometryRect and dpr
alt XCB_geometry_mismatch
FullScreenBackground->>FullScreenBackground: qCWarning(DDE_SHELL, XCB mismatch)
FullScreenBackground->>QtWindow: windowHandle()->screen()
QtWindow-->>FullScreenBackground: currentScreen
alt screen_mismatch_and_m_screen_not_null
FullScreenBackground->>QtWindow: setScreen(m_screen)
end
FullScreenBackground->>FullScreenBackground: setGeometry(0,0,0,0)
FullScreenBackground->>FullScreenBackground: setGeometry(m_geometryRect)
else XCB_geometry_match
FullScreenBackground->>FullScreenBackground: qCInfo(DDE_SHELL, position match)
end
else no_connection
FullScreenBackground->>FullScreenBackground: skip XCB validation
end
else Wayland_backend
FullScreenBackground->>FullScreenBackground: skip XCB validation
end
deactivate FullScreenBackground
Sequence diagram for MultiScreenManager lock frame XCB validationsequenceDiagram
participant MSM as MultiScreenManager
participant QtApp as QGuiApplication
participant X11Conn as XCB_Connection
participant Screen as QScreen
participant Frame as LockFrame_QWidget
participant XServer
MSM->>MSM: checkLockFrameLocation()
MSM->>QtApp: platformName()
QtApp-->>MSM: platformName
alt not_Wayland
MSM->>X11Conn: get X11 connection (QX11Info or QX11Application)
X11Conn-->>MSM: xcb_connection_t*
else Wayland
MSM->>MSM: connection = nullptr
end
MSM->>MSM: for each screen in m_frames.keys()
loop each_screen
MSM->>MSM: frame = m_frames.value(screen)
MSM->>MSM: qCInfo(DDE_SHELL, screen and Qt frame geometry)
alt connection_and_frame_available
MSM->>X11Conn: xcb_get_geometry(frame->winId())
X11Conn->>XServer: query window geometry
XServer-->>X11Conn: xcb_get_geometry_reply
X11Conn-->>MSM: reply(x, y, width, height)
MSM->>MSM: xcbGeometry from reply
MSM->>Frame: devicePixelRatioF()
Frame-->>MSM: dpr
MSM->>Screen: geometry()
Screen-->>MSM: screenGeometry
MSM->>MSM: expectedPhysical from screenGeometry and dpr
MSM->>MSM: compare xcbGeometry vs expectedPhysical
alt geometry_match
MSM->>MSM: qCInfo(DDE_SHELL, XCB match)
else geometry_mismatch
MSM->>MSM: qCWarning(DDE_SHELL, POSITION MISMATCH DETECTED)
end
else no_connection_or_frame
MSM->>MSM: skip XCB validation for this screen
end
end
Updated class diagram for FullScreenBackground and MultiScreenManager geometry handlingclassDiagram
class SessionBaseModel
class QScreen
class QWidget
class QTimer
class QRect
class FullScreenBackground {
- SessionBaseModel* m_model
- QPointer~QScreen~ m_screen
- QTimer* m_resetGeometryTimer
- QRect m_geometryRect
+ FullScreenBackground(SessionBaseModel* model, QWidget* parent)
+ updateGeometry() void
+ setddeGeometry(const QRect& rect) void
}
class MultiScreenManager {
- QMap~QScreen*, QWidget*~ m_frames
- bool m_isCopyMode
+ MultiScreenManager(QObject* parent)
+ onScreenAdded(QPointer~QScreen~ screen) void
+ checkLockFrameLocation() void
}
class QWindow
class QGuiApplication
FullScreenBackground --|> QWidget
MultiScreenManager ..> FullScreenBackground : manages_lock_frames
MultiScreenManager --> QScreen : uses
MultiScreenManager --> QWidget : uses_as_frame
FullScreenBackground --> QScreen : binds_to
FullScreenBackground --> QTimer : uses_resetGeometryTimer
FullScreenBackground --> QRect : stores_target_geometry
MultiScreenManager --> QRect : uses_screen_geometry
FullScreenBackground --> QWindow : uses_windowHandle
MultiScreenManager --> QGuiApplication : queries_platform_and_X11_connection
FullScreenBackground --> SessionBaseModel : reads_backend_type_and_state
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 5 issues, and left some high level feedback:
- The XCB geometry inspection logic is duplicated between
FullScreenBackgroundandMultiScreenManager; consider extracting a shared helper to query/check X11 window geometry so future fixes stay in one place. - Several new logging statements dereference
windowHandle()->screen()->name()without verifyingscreen()is non-null; adding a null-check (or conditional logging) would avoid potential crashes when no screen is associated. - The newly added
qCInfo/qCWarninglogs in geometry timers and periodic checks will run very frequently and may spam logs in normal operation; consider downgrading some of these toqCDebugor gating them behind a verbosity flag.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The XCB geometry inspection logic is duplicated between `FullScreenBackground` and `MultiScreenManager`; consider extracting a shared helper to query/check X11 window geometry so future fixes stay in one place.
- Several new logging statements dereference `windowHandle()->screen()->name()` without verifying `screen()` is non-null; adding a null-check (or conditional logging) would avoid potential crashes when no screen is associated.
- The newly added `qCInfo`/`qCWarning` logs in geometry timers and periodic checks will run very frequently and may spam logs in normal operation; consider downgrading some of these to `qCDebug` or gating them behind a verbosity flag.
## Individual Comments
### Comment 1
<location path="src/widgets/fullscreenbackground.cpp" line_range="508-509" />
<code_context>
if (!m_screen.isNull()) {
- if(m_model->isUseWayland())
+ // X11 下也需要将 QWindow 关联到正确的 QScreen，否则 Qt 可能将窗口归属到错误的 screen
+ if (windowHandle() && windowHandle()->screen() != m_screen) {
+ qCInfo(DDE_SHELL) << "bindWindowToScreen, windowHandle screen:" << windowHandle()->screen()->name()
+ << ", target screen:" << m_screen->name() << ", this:" << this;
windowHandle()->setScreen(m_screen);
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against null QScreen when logging windowHandle()->screen()->name().
`windowHandle()->screen()` may be null (e.g. during screen changes or early initialization), so calling `.name()` directly risks a null dereference in this logging path. Capture the screen in a local pointer and check it before accessing `name()`, for example:
```cpp
if (auto *win = windowHandle()) {
QScreen *curScreen = win->screen();
qCInfo(DDE_SHELL) << "bindWindowToScreen, windowHandle screen:"
<< (curScreen ? curScreen->name() : "null")
<< ", target screen:" << m_screen->name() << ", this:" << this;
if (curScreen != m_screen)
win->setScreen(m_screen);
}
```
</issue_to_address>
### Comment 2
<location path="src/widgets/fullscreenbackground.cpp" line_range="799" />
<code_context>
+ << ", target rect:" << rect
+ << ", current geometry:" << geometry()
+ << ", screen:" << (m_screen ? m_screen->name() : "null")
+ << ", windowHandle screen:" << (windowHandle() ? windowHandle()->screen()->name() : "null")
+ << ", dpr:" << devicePixelRatioF();
setGeometry(rect);
</code_context>
<issue_to_address>
**issue (bug_risk):** Also guard windowHandle()->screen() in logging to prevent crashes when screen is null.
The current ternary only guards `windowHandle()`, not `windowHandle()->screen()`. If a window exists but has no screen yet, `screen()->name()` will dereference null and crash in this logging code. Please also guard the screen pointer, e.g. via a local `QScreen *winScreen` and logging `winScreen ? winScreen->name() : "null"`.
</issue_to_address>
### Comment 3
<location path="src/global_util/multiscreenmanager.cpp" line_range="271-272" />
<code_context>
+ }
+
for (QScreen *screen : m_frames.keys()) {
if (screen) {
- qCInfo(DDE_SHELL) << "Check lock frame location, screen:" << screen << ", location:" << screen->geometry()
- << ", lockframe:" << m_frames.value(screen) << ", location:" << m_frames.value(screen)->geometry();
+ QWidget *frame = m_frames.value(screen);
+ qCInfo(DDE_SHELL) << "Check lock frame location, screen:" << screen
+ << ", screen name:" << screen->name()
</code_context>
<issue_to_address>
**issue (bug_risk):** Check that the frame pointer is non-null before dereferencing it.
In this loop you only check `screen`, but then dereference `frame` (e.g. `frame->geometry()`) without ensuring it’s non-null. If `m_frames` can contain `nullptr`, this can crash. Please add a null check for `frame` (e.g. `if (!frame) continue;` or `if (screen && frame)` before use).
</issue_to_address>
### Comment 4
<location path="src/widgets/fullscreenbackground.cpp" line_range="91" />
<code_context>
+
+ // 通过 XCB 检查窗口在 X Server 中的实际位置
+ // 注意：X11 坐标系中，窗口位置不乘 DPR（与 X RandR 一致），窗口大小乘 DPR
+ if (!m_model->isUseWayland() && windowHandle()) {
+ xcb_connection_t *connection = nullptr;
+#ifndef ENABLE_DSS_SNIPE
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the X11/XCB connection and geometry reconciliation logic into helper functions and centralizing the repeated logging to keep the timer callback and geometry-related methods simpler and easier to read.
You can keep the new behavior but reduce complexity by extracting the XCB/X11 handling and geometry reconciliation into small helpers, and by centralizing the verbose logging.
### 1. Extract X11 connection acquisition
The timer lambda and possibly other files duplicate the `#ifndef ENABLE_DSS_SNIPE` + `QX11Info` vs `QNativeInterface::QX11Application` logic. Move that into a small helper:
```cpp
// In a suitable .cpp/.h (e.g. fullscreenbackground.cpp or a small X11 helper)
static xcb_connection_t *x11Connection()
{
xcb_connection_t *connection = nullptr;
#ifndef ENABLE_DSS_SNIPE
connection = QX11Info::connection();
#else
auto *x11App = qGuiApp->nativeInterface<QNativeInterface::QX11Application>();
if (x11App)
connection = x11App->connection();
#endif
return connection;
}
```
Then use it in the timer:
```cpp
if (!m_model->isUseWayland() && windowHandle()) {
if (auto *connection = x11Connection()) {
auto cookie = xcb_get_geometry(connection, static_cast<xcb_window_t>(winId()));
auto *reply = xcb_get_geometry_reply(connection, cookie, nullptr);
// ...
}
}
```
This removes preprocessor noise from the lambda and avoids duplication in other places.
### 2. Extract XCB geometry reconciliation
The DPR handling, XCB `get_geometry`, and resize-to-0 workaround can be moved out of the lambda into a dedicated method. That keeps the timer callback focused on “check geometry + fix if needed”.
```cpp
// In FullScreenBackground
void FullScreenBackground::ensureXcbGeometryMatches()
{
if (m_model->isUseWayland() || !windowHandle())
return;
xcb_connection_t *connection = x11Connection();
if (!connection)
return;
auto cookie = xcb_get_geometry(connection, static_cast<xcb_window_t>(winId()));
auto *reply = xcb_get_geometry_reply(connection, cookie, nullptr);
if (!reply)
return;
const QRect xcbGeometry(reply->x, reply->y, reply->width, reply->height);
free(reply);
const qreal dpr = devicePixelRatioF();
const QRect expectedPhysical(
m_geometryRect.x(),
m_geometryRect.y(),
qRound(m_geometryRect.width() * dpr),
qRound(m_geometryRect.height() * dpr));
if (xcbGeometry == expectedPhysical) {
qCInfo(DDE_SHELL) << "XCB position match, no need to set geometry";
return;
}
qCWarning(DDE_SHELL) << "XCB position mismatch! XCB geometry(physical):" << xcbGeometry
<< ", expected(physical):" << expectedPhysical
<< ", target(logical):" << m_geometryRect
<< ", dpr:" << dpr
<< ", this:" << this
<< ", screen:" << (m_screen ? m_screen->name() : "null");
if (!m_screen.isNull() && windowHandle()->screen() != m_screen)
windowHandle()->setScreen(m_screen);
setGeometry(0, 0, 0, 0);
setGeometry(m_geometryRect);
}
```
Timer becomes much clearer:
```cpp
connect(m_resetGeometryTimer, &QTimer::timeout, this, [this] {
const auto ¤tGeometry = geometry();
if (currentGeometry != m_geometryRect) {
qCWarning(DDE_SHELL) << "Geometry mismatch detected! Qt cached geometry:" << currentGeometry
<< ", target geometry:" << m_geometryRect << ", this:" << this;
setGeometry(m_geometryRect);
}
ensureXcbGeometryMatches();
});
```
### 3. Centralize repetitive logging
`setddeGeometry`, `updateGeometry`, and the timer lambda all log similar information about the frame, screen, and window handle. You can reduce repetition and visual noise by introducing a small logging helper:
```cpp
void FullScreenBackground::logGeometryInfo(const char *prefix, const QRect &target) const
{
qCInfo(DDE_SHELL) << prefix
<< "this:" << this
<< "target:" << target
<< "current geometry:" << geometry()
<< "screen:" << (m_screen ? m_screen->name() : "null")
<< "windowHandle screen:" << (windowHandle() ? windowHandle()->screen()->name() : "null")
<< "dpr:" << devicePixelRatioF();
}
```
Use it where logging is currently verbose:
```cpp
void FullScreenBackground::setddeGeometry(const QRect &rect)
{
logGeometryInfo("setddeGeometry", rect);
setGeometry(rect);
m_geometryRect = rect;
m_resetGeometryTimer->start(200);
QTimer::singleShot(400 * 5, m_resetGeometryTimer, &QTimer::stop);
}
```
And in `updateGeometry` / timer as needed, with short prefixes. This keeps all current information but makes the control flow easier to read and maintain.
</issue_to_address>
### Comment 5
<location path="src/global_util/multiscreenmanager.cpp" line_range="257" />
<code_context>
void MultiScreenManager::checkLockFrameLocation()
{
+ xcb_connection_t *connection = nullptr;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the XCB connection retrieval and geometry-checking logic into reusable helper functions so `checkLockFrameLocation` stays focused on high-level behavior.
You can keep all the new checks/logging but reduce complexity by extracting the XCB-specific and geometry logic into small helpers, and centralizing XCB connection acquisition (also avoids duplication with `fullscreenbackground.cpp`).
### 1. Centralize XCB connection retrieval
Create a small helper (in a shared `.cpp` or anonymous namespace) and use it both here and in `fullscreenbackground.cpp`:
```cpp
// e.g. in a common helper .cpp, or at top of this .cpp in an anonymous namespace
static xcb_connection_t *getXcbConnection()
{
if (QGuiApplication::platformName().startsWith("wayland", Qt::CaseInsensitive))
return nullptr;
#ifndef ENABLE_DSS_SNIPE
return QX11Info::connection();
#else
auto *x11App = qGuiApp->nativeInterface<QNativeInterface::QX11Application>();
return x11App ? x11App->connection() : nullptr;
#endif
}
```
Then `checkLockFrameLocation` becomes simpler:
```cpp
void MultiScreenManager::checkLockFrameLocation()
{
xcb_connection_t *connection = getXcbConnection();
for (QScreen *screen : m_frames.keys()) {
if (!screen)
continue;
QWidget *frame = m_frames.value(screen);
qCInfo(DDE_SHELL) << "Check lock frame location, screen:" << screen
<< ", screen name:" << screen->name()
<< ", screen geometry:" << screen->geometry()
<< ", lockframe:" << frame
<< ", Qt frame geometry:" << frame->geometry();
if (connection && frame)
checkAndLogXcbGeometry(connection, screen, frame);
}
}
```
### 2. Extract geometry calculation + logging
Move the DPR/geometry math and logging into a focused helper:
```cpp
static void checkAndLogXcbGeometry(xcb_connection_t *connection, QScreen *screen, QWidget *frame)
{
auto cookie = xcb_get_geometry(connection, static_cast<xcb_window_t>(frame->winId()));
auto *reply = xcb_get_geometry_reply(connection, cookie, nullptr);
if (!reply) {
qCWarning(DDE_SHELL) << " Failed to get XCB geometry for frame:" << frame;
return;
}
QRect xcbGeometry(reply->x, reply->y, reply->width, reply->height);
free(reply);
const qreal dpr = frame->devicePixelRatioF();
const QRect screenGeo = screen->geometry();
QRect expectedPhysical(screenGeo.x(), screenGeo.y(),
qRound(screenGeo.width() * dpr),
qRound(screenGeo.height() * dpr));
const bool positionMatch = (xcbGeometry == expectedPhysical);
qCInfo(DDE_SHELL) << " XCB actual geometry(physical):" << xcbGeometry
<< ", expected(physical):" << expectedPhysical
<< ", dpr:" << dpr
<< ", match:" << positionMatch;
if (!positionMatch) {
qCWarning(DDE_SHELL) << " *** POSITION MISMATCH DETECTED! ***"
<< "XCB geometry:" << xcbGeometry
<< "expected:" << expectedPhysical
<< "for screen" << screen->name();
}
}
```
This keeps all current behavior and logging, but makes `checkLockFrameLocation` read as high-level intent while encapsulating platform-specific and geometry details.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+508
to
+509
| if (windowHandle() && windowHandle()->screen() != m_screen) { | ||
| qCInfo(DDE_SHELL) << "bindWindowToScreen, windowHandle screen:" << windowHandle()->screen()->name() |
There was a problem hiding this comment.
issue (bug_risk): Guard against null QScreen when logging windowHandle()->screen()->name().
windowHandle()->screen() may be null (e.g. during screen changes or early initialization), so calling .name() directly risks a null dereference in this logging path. Capture the screen in a local pointer and check it before accessing name(), for example:
if (auto *win = windowHandle()) {
QScreen *curScreen = win->screen();
qCInfo(DDE_SHELL) << "bindWindowToScreen, windowHandle screen:"
<< (curScreen ? curScreen->name() : "null")
<< ", target screen:" << m_screen->name() << ", this:" << this;
if (curScreen != m_screen)
win->setScreen(m_screen);
}| << ", target rect:" << rect | ||
| << ", current geometry:" << geometry() | ||
| << ", screen:" << (m_screen ? m_screen->name() : "null") | ||
| << ", windowHandle screen:" << (windowHandle() ? windowHandle()->screen()->name() : "null") |
There was a problem hiding this comment.
issue (bug_risk): Also guard windowHandle()->screen() in logging to prevent crashes when screen is null.
The current ternary only guards windowHandle(), not windowHandle()->screen(). If a window exists but has no screen yet, screen()->name() will dereference null and crash in this logging code. Please also guard the screen pointer, e.g. via a local QScreen *winScreen and logging winScreen ? winScreen->name() : "null".
Comment on lines
271
to
+272
| if (screen) { | ||
| qCInfo(DDE_SHELL) << "Check lock frame location, screen:" << screen << ", location:" << screen->geometry() | ||
| << ", lockframe:" << m_frames.value(screen) << ", location:" << m_frames.value(screen)->geometry(); | ||
| QWidget *frame = m_frames.value(screen); |
There was a problem hiding this comment.
issue (bug_risk): Check that the frame pointer is non-null before dereferencing it.
In this loop you only check screen, but then dereference frame (e.g. frame->geometry()) without ensuring it’s non-null. If m_frames can contain nullptr, this can crash. Please add a null check for frame (e.g. if (!frame) continue; or if (screen && frame) before use).
|
|
||
| // 通过 XCB 检查窗口在 X Server 中的实际位置 | ||
| // 注意：X11 坐标系中，窗口位置不乘 DPR（与 X RandR 一致），窗口大小乘 DPR | ||
| if (!m_model->isUseWayland() && windowHandle()) { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the X11/XCB connection and geometry reconciliation logic into helper functions and centralizing the repeated logging to keep the timer callback and geometry-related methods simpler and easier to read.
You can keep the new behavior but reduce complexity by extracting the XCB/X11 handling and geometry reconciliation into small helpers, and by centralizing the verbose logging.
1. Extract X11 connection acquisition
The timer lambda and possibly other files duplicate the #ifndef ENABLE_DSS_SNIPE + QX11Info vs QNativeInterface::QX11Application logic. Move that into a small helper:
// In a suitable .cpp/.h (e.g. fullscreenbackground.cpp or a small X11 helper)
static xcb_connection_t *x11Connection()
{
xcb_connection_t *connection = nullptr;
#ifndef ENABLE_DSS_SNIPE
connection = QX11Info::connection();
#else
auto *x11App = qGuiApp->nativeInterface<QNativeInterface::QX11Application>();
if (x11App)
connection = x11App->connection();
#endif
return connection;
}Then use it in the timer:
if (!m_model->isUseWayland() && windowHandle()) {
if (auto *connection = x11Connection()) {
auto cookie = xcb_get_geometry(connection, static_cast<xcb_window_t>(winId()));
auto *reply = xcb_get_geometry_reply(connection, cookie, nullptr);
// ...
}
}This removes preprocessor noise from the lambda and avoids duplication in other places.
2. Extract XCB geometry reconciliation
The DPR handling, XCB get_geometry, and resize-to-0 workaround can be moved out of the lambda into a dedicated method. That keeps the timer callback focused on “check geometry + fix if needed”.
// In FullScreenBackground
void FullScreenBackground::ensureXcbGeometryMatches()
{
if (m_model->isUseWayland() || !windowHandle())
return;
xcb_connection_t *connection = x11Connection();
if (!connection)
return;
auto cookie = xcb_get_geometry(connection, static_cast<xcb_window_t>(winId()));
auto *reply = xcb_get_geometry_reply(connection, cookie, nullptr);
if (!reply)
return;
const QRect xcbGeometry(reply->x, reply->y, reply->width, reply->height);
free(reply);
const qreal dpr = devicePixelRatioF();
const QRect expectedPhysical(
m_geometryRect.x(),
m_geometryRect.y(),
qRound(m_geometryRect.width() * dpr),
qRound(m_geometryRect.height() * dpr));
if (xcbGeometry == expectedPhysical) {
qCInfo(DDE_SHELL) << "XCB position match, no need to set geometry";
return;
}
qCWarning(DDE_SHELL) << "XCB position mismatch! XCB geometry(physical):" << xcbGeometry
<< ", expected(physical):" << expectedPhysical
<< ", target(logical):" << m_geometryRect
<< ", dpr:" << dpr
<< ", this:" << this
<< ", screen:" << (m_screen ? m_screen->name() : "null");
if (!m_screen.isNull() && windowHandle()->screen() != m_screen)
windowHandle()->setScreen(m_screen);
setGeometry(0, 0, 0, 0);
setGeometry(m_geometryRect);
}Timer becomes much clearer:
connect(m_resetGeometryTimer, &QTimer::timeout, this, [this] {
const auto ¤tGeometry = geometry();
if (currentGeometry != m_geometryRect) {
qCWarning(DDE_SHELL) << "Geometry mismatch detected! Qt cached geometry:" << currentGeometry
<< ", target geometry:" << m_geometryRect << ", this:" << this;
setGeometry(m_geometryRect);
}
ensureXcbGeometryMatches();
});3. Centralize repetitive logging
setddeGeometry, updateGeometry, and the timer lambda all log similar information about the frame, screen, and window handle. You can reduce repetition and visual noise by introducing a small logging helper:
void FullScreenBackground::logGeometryInfo(const char *prefix, const QRect &target) const
{
qCInfo(DDE_SHELL) << prefix
<< "this:" << this
<< "target:" << target
<< "current geometry:" << geometry()
<< "screen:" << (m_screen ? m_screen->name() : "null")
<< "windowHandle screen:" << (windowHandle() ? windowHandle()->screen()->name() : "null")
<< "dpr:" << devicePixelRatioF();
}Use it where logging is currently verbose:
void FullScreenBackground::setddeGeometry(const QRect &rect)
{
logGeometryInfo("setddeGeometry", rect);
setGeometry(rect);
m_geometryRect = rect;
m_resetGeometryTimer->start(200);
QTimer::singleShot(400 * 5, m_resetGeometryTimer, &QTimer::stop);
}And in updateGeometry / timer as needed, with short prefixes. This keeps all current information but makes the control flow easier to read and maintain.
| @@ -244,10 +256,53 @@ void MultiScreenManager::onDisplayModeChanged(const QString &) | |||
|
|
|||
| void MultiScreenManager::checkLockFrameLocation() | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the XCB connection retrieval and geometry-checking logic into reusable helper functions so checkLockFrameLocation stays focused on high-level behavior.
You can keep all the new checks/logging but reduce complexity by extracting the XCB-specific and geometry logic into small helpers, and centralizing XCB connection acquisition (also avoids duplication with fullscreenbackground.cpp).
1. Centralize XCB connection retrieval
Create a small helper (in a shared .cpp or anonymous namespace) and use it both here and in fullscreenbackground.cpp:
// e.g. in a common helper .cpp, or at top of this .cpp in an anonymous namespace
static xcb_connection_t *getXcbConnection()
{
if (QGuiApplication::platformName().startsWith("wayland", Qt::CaseInsensitive))
return nullptr;
#ifndef ENABLE_DSS_SNIPE
return QX11Info::connection();
#else
auto *x11App = qGuiApp->nativeInterface<QNativeInterface::QX11Application>();
return x11App ? x11App->connection() : nullptr;
#endif
}Then checkLockFrameLocation becomes simpler:
void MultiScreenManager::checkLockFrameLocation()
{
xcb_connection_t *connection = getXcbConnection();
for (QScreen *screen : m_frames.keys()) {
if (!screen)
continue;
QWidget *frame = m_frames.value(screen);
qCInfo(DDE_SHELL) << "Check lock frame location, screen:" << screen
<< ", screen name:" << screen->name()
<< ", screen geometry:" << screen->geometry()
<< ", lockframe:" << frame
<< ", Qt frame geometry:" << frame->geometry();
if (connection && frame)
checkAndLogXcbGeometry(connection, screen, frame);
}
}2. Extract geometry calculation + logging
Move the DPR/geometry math and logging into a focused helper:
static void checkAndLogXcbGeometry(xcb_connection_t *connection, QScreen *screen, QWidget *frame)
{
auto cookie = xcb_get_geometry(connection, static_cast<xcb_window_t>(frame->winId()));
auto *reply = xcb_get_geometry_reply(connection, cookie, nullptr);
if (!reply) {
qCWarning(DDE_SHELL) << " Failed to get XCB geometry for frame:" << frame;
return;
}
QRect xcbGeometry(reply->x, reply->y, reply->width, reply->height);
free(reply);
const qreal dpr = frame->devicePixelRatioF();
const QRect screenGeo = screen->geometry();
QRect expectedPhysical(screenGeo.x(), screenGeo.y(),
qRound(screenGeo.width() * dpr),
qRound(screenGeo.height() * dpr));
const bool positionMatch = (xcbGeometry == expectedPhysical);
qCInfo(DDE_SHELL) << " XCB actual geometry(physical):" << xcbGeometry
<< ", expected(physical):" << expectedPhysical
<< ", dpr:" << dpr
<< ", match:" << positionMatch;
if (!positionMatch) {
qCWarning(DDE_SHELL) << " *** POSITION MISMATCH DETECTED! ***"
<< "XCB geometry:" << xcbGeometry
<< "expected:" << expectedPhysical
<< "for screen" << screen->name();
}
}This keeps all current behavior and logging, but makes checkLockFrameLocation read as high-level intent while encapsulating platform-specific and geometry details.
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
2 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.
Synchronize source files from linuxdeepin/dde-session-shell.
Source-pull-request: linuxdeepin/dde-session-shell#58
Summary by Sourcery
Improve fullscreen background and multi-screen lock frame positioning correctness and diagnostics, especially under X11.
Bug Fixes:
Enhancements: