Skip to content

fix: rebuild desktop root native windows on topology change#3704

Open
Ivy233 wants to merge 1 commit intolinuxdeepin:masterfrom
Ivy233:fix/desktop-layout-root-rebuild
Open

fix: rebuild desktop root native windows on topology change#3704
Ivy233 wants to merge 1 commit intolinuxdeepin:masterfrom
Ivy233:fix/desktop-layout-root-rebuild

Conversation

@Ivy233
Copy link
Copy Markdown

@Ivy233 Ivy233 commented Apr 21, 2026

https://bbs.deepin.org/post/297315
该反馈的代码提交,是带调试log的,如果需要合并还需要处理,仅用作一个提示。
如果直接用该pr,需要删掉fmWarning;也可以基于该pr重新做一些修改后重新提。

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @Ivy233, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Ivy233

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@deepin-bot
Copy link
Copy Markdown
Contributor

deepin-bot Bot commented Apr 23, 2026

TAG Bot

New tag: 6.5.133
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #3715

@Johnson-zs Johnson-zs force-pushed the fix/desktop-layout-root-rebuild branch from a6fc9ee to 6c5b6e8 Compare April 24, 2026 05:08
@deepin-ci-robot
Copy link
Copy Markdown
Contributor

deepin pr auto review

这份代码的 diff 主要涉及桌面环境在多显示器环境下的窗口重建逻辑、显示更新机制以及信号处理。以下是对语法逻辑、代码质量、代码性能和代码安全的详细审查和改进意见:

1. 语法与逻辑审查

审查点:getScreenName 函数修改

inline QString getScreenName(QWidget *win)
{
-    Q_ASSERT(win);
+    if (!win)
+        return QString();
    return win->property(DesktopFrameProperty::kPropScreenName).toString();
}
  • 意见:这是一个非常好的改进
  • 理由:原代码使用 Q_ASSERT 仅在 Debug 模式下生效。如果 Release 版本传入空指针,程序将直接崩溃。改为显式检查并返回空字符串,增强了代码的健壮性,防止了空指针解引用。

审查点:BaseWindow::recreateNativeWindow

void BaseWindow::recreateNativeWindow(const QRect &geometry)
{
    hide();
    setGeometry(geometry);
    destroy(true, false);
    create();
    setGeometry(geometry);
}
  • 意见:逻辑基本正确,但存在潜在风险。
  • 理由
    1. destroy(true, false) 会销毁窗口但不销毁对象。紧接着调用 create() 创建新的原生窗口。
    2. 连续调用两次 setGeometry 是必要的。第一次是在销毁前设置(虽然可能无效),第二次是在创建后设置。
    3. 风险:如果在多线程环境下调用,或者在其他槽函数中依赖该窗口的句柄,这种"销毁-重建"过程可能导致短暂的悬空指针访问或竞态条件。鉴于这是桌面插件,通常在主线程运行,风险较低,但需确保调用时机安全。

审查点:WindowFrame::needRecreateNativeWindows

bool WindowFrame::needRecreateNativeWindows(const QList<ScreenPointer> &screens) const
{
    // ... 省略部分代码 ...
    for (auto it = currentGeometries.cbegin(); it != currentGeometries.cend(); ++it) {
        const QRect previous = d->screenGeometries.value(it.key());
        const QRect current = it.value();
        if (previous.topLeft() != current.topLeft()) {
            return true;
        }
    }
    // ...
}
  • 意见:逻辑判断略显单一,可能存在边界情况。
  • 理由:该函数仅通过屏幕顺序和左上角坐标来判断是否需要重建。如果屏幕分辨率改变(宽高变化但左上角不变),或者屏幕旋转,该函数可能返回 false,导致窗口几何更新而非重建。如果某些特定的窗口合成器或窗口管理器在分辨率变化时需要重建窗口句柄才能正确渲染,这里可能会出 Bug。不过,对于常规的显示模式切换(如复制/扩展),这个逻辑是足够的。

2. 代码质量审查

审查点:onWindowShowed 的实现
BackgroundManager, CanvasManager, FrameManager 中都有类似实现:

void BackgroundManager::onWindowShowed()
{
    for (auto it = d->backgroundWidgets.begin(); it != d->backgroundWidgets.end(); ++it) {
        const BackgroundWidgetPointer &bwp = it.value();
        bwp->show();
        bwp->update();
    }
}
  • 意见:代码重复,且 show()update() 的调用顺序和必要性值得商榷。
  • 理由
    1. 重复代码:三个管理器都在做几乎相同的事情(遍历容器,调用 show 和 update)。可以考虑在基类中实现,或者使用统一的信号处理机制。
    2. 性能show() 通常会触发 paintEvent,紧接着手动调用 update() 可能会导致重绘两次。建议先 show(),如果确实需要强制刷新,再调用 repaint()(同步)或依赖 Qt 的自动更新机制。
    3. 逻辑show() 会设置 Qt::WA_WState_Visible 位。如果窗口已经可见,show() 基本不做操作。但 update() 会调度重绘。如果是为了解决窗口重建后不显示的问题,确保 show() 被调用是关键。

审查点:WindowFrame::buildBaseWindow 中的锁与标志位

    QWriteLocker lk(&d->locker);
+    const bool recreateNativeWindows = d->recreateNativeWindows;
+    d->recreateNativeWindows = false;
  • 意见:良好的实践。
  • 理由:在持有写锁的情况下读取并重置标志位,避免了竞态条件。这表明开发者考虑到了线程安全。

3. 代码性能审查

审查点:频繁的屏幕拓扑缓存更新

void WindowFrame::cacheScreenTopology(const QList<ScreenPointer> &screens)
{
    d->screenOrder.clear();
    d->screenGeometries.clear();
    // ... 填充数据 ...
}
  • 意见:在 onGeometryChangedbuildBaseWindow 结束时都会调用此函数。
  • 理由
    1. 如果 onGeometryChanged 触发非常频繁(例如用户拖拽窗口改变大小时,虽然这里是屏幕几何变化,但某些驱动可能会报告频繁的中间态),频繁的 clear()insert() 操作会带来微小的内存分配开销。
    2. 改进建议:由于屏幕数量通常很少(1-4个),这个开销可以忽略不计。但如果要极致优化,可以检查 currentOrdercurrentGeometries 是否真的发生了变化再进行赋值,不过这又增加了一次计算开销,得不偿失。目前的实现是可以接受的。

审查点:WindowFrame::onGeometryChanged 的逻辑流

void WindowFrame::onGeometryChanged()
{
    bool changed = false;
+    const auto screens = ddplugin_desktop_util::screenProxyLogicScreens();
+    if (needRecreateNativeWindows(screens)) {
+        d->recreateNativeWindows = true;
+        buildBaseWindow();
+        return;
+    }
    // ...
}
  • 意见:逻辑清晰,但要注意递归调用风险。
  • 理由
    1. onGeometryChanged 调用 buildBaseWindow
    2. buildBaseWindow 内部会调用 setGeometry
    3. setGeometry 可能会再次触发 onGeometryChanged 信号(取决于信号连接方式,Qt 中 setGeometry 通常不会自动触发 QScreen 的几何变化信号,除非是底层平台事件导致的)。
    4. 关键点:如果 buildBaseWindow 中的 setGeometry 触发了系统级事件,进而导致 onGeometryChanged 再次被调用,且 needRecreateNativeWindows 依然返回 true,可能会导致递归调用栈溢出。
    5. 改进建议:目前的逻辑中,buildBaseWindow 结束时会调用 cacheScreenTopology,这会更新 d->screenOrderd->screenGeometries。因此,下一次 onGeometryChanged 被调用时,needRecreateNativeWindows 应该返回 false。逻辑上是闭环的,风险较低。

4. 代码安全审查

审查点:空指针检查
WindowFramePrivate::recreateNativeWindow 中:

void WindowFramePrivate::recreateNativeWindow(BaseWindowPointer win, ScreenPointer screen)
{
    if (!win || !screen)
        return;
    // ...
}
  • 意见:安全措施到位。
  • 理由:在操作智能指针和底层窗口句柄前进行了空指针检查,防止崩溃。

审查点:窗口句柄的有效性

    if (QWindow *handle = win->windowHandle()) {
        handle->setOpacity(0.99);
        traceWindow(handle);
    }
  • 意见:安全。
  • 理由:检查 windowHandle() 是否存在。虽然刚刚调用了 create(),理论上应该存在,但显式检查是好习惯。

总结与改进建议

  1. 优化 onWindowShowed 的重绘逻辑

    • 建议确认 show() 后是否必须立即调用 update()。如果是,可以尝试合并为 show() 后调用 repaint()(如果同步刷新是必须的),或者仅依赖 show() 触发的自动重绘,减少不必要的绘制调度。
  2. needRecreateNativeWindows 的判断条件

    • 建议补充注释说明为什么只检查 topLeft()。如果是为了处理屏幕位置移动而不关心分辨率变化,应明确注释。如果分辨率变化也需要重建,应修改为 previous != current
  3. 信号槽连接的生命周期管理

    • BackgroundManagerCanvasManager 的析构函数中,代码显式取消了订阅信号。这是非常好的做法,防止了悬空槽函数的调用。
  4. 代码重复

    • BackgroundManagerCanvasManagerFrameManager 中的 onWindowShowed 逻辑高度相似。建议考虑引入一个共同的基类或者辅助函数来处理这些窗口的显示更新,以减少代码冗长。
  5. recreateNativeWindow 的副作用

    • recreateNativeWindow 中设置了 setOpacity(0.99)。这是一个"魔法数字"。
    • 建议:将其定义为常量,并添加注释解释为什么设置为 0.99(通常是为了规避某些窗口管理器的合成问题或强制刷新)。
// 示例改进:定义常量
static constexpr qreal kWindowRefreshOpacity = 0.99;
// 使用
handle->setOpacity(kWindowRefreshOpacity);

总体而言,这段代码在处理多显示器动态切换和窗口重建方面逻辑清晰,安全性考虑周全(空指针检查、锁机制)。主要的改进空间在于减少代码重复、明确魔法数字的含义以及微调重绘逻辑。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants