Skip to content

fix(main_window): enhance tool bar management and cleanup#840

Merged
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
dengzhongyuan365-dev:treeland-fix-v3
May 19, 2026
Merged

fix(main_window): enhance tool bar management and cleanup#840
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
dengzhongyuan365-dev:treeland-fix-v3

Conversation

@dengzhongyuan365-dev
Copy link
Copy Markdown
Contributor

  • Refactor tool bar handling to ensure proper initialization and destruction, preventing potential memory leaks and dangling pointers.
  • Implement checks for tool bar existence before performing actions, improving stability during screen capture operations.
  • Add a new method to destroy the tool bar, allowing for better resource management when reselecting capture regions.

This update addresses issues related to tool bar visibility and lifecycle, enhancing the overall user experience during screen recording.

TASK: https://pms.uniontech.com/task-view-389563.html

- Refactor tool bar handling to ensure proper initialization and destruction, preventing potential memory leaks and dangling pointers.
- Implement checks for tool bar existence before performing actions, improving stability during screen capture operations.
- Add a new method to destroy the tool bar, allowing for better resource management when reselecting capture regions.

This update addresses issues related to tool bar visibility and lifecycle, enhancing the overall user experience during screen recording.

TASK: https://pms.uniontech.com/task-view-389563.html
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这份 Git Diff 主要针对截屏工具在 Wayland(特别是 Treeland)环境下的生命周期管理进行了重构。核心改动是将 ToolBar 的创建时机从主窗口初始化延迟到截屏区域确定后(updateCaptureRegion),并在重选区时销毁重建。同时,为了适配这种动态生命周期,全面增加了对 m_toolBarm_toolbarWidget 的空指针检查,并引入 QPointer 修复了 ImageBorderHelper 中潜在的悬空指针问题。

以下是对该 Diff 的详细代码审查及改进建议:

一、 语法与逻辑

1. ToolBar 生命周期的竞态条件与重复创建
updateCaptureRegion 中,逻辑是“先销毁旧的,再创建新的”。但在 Wayland 环境下,信号 captureRegionChanged 可能会因为窗口管理器的状态变化被高频触发。如果两次触发间隔极短,可能会导致 ToolBar 被频繁 new/delete,引发界面闪烁或内存抖动。

  • 改进建议:如果选区只是位置/大小变化,而工具栏的配置无需重置,可以考虑复用现有的 ToolBar,仅更新其位置和绑定的 subsurface。如果必须重建(例如 Wayland subsurface 必须重新绑定),建议增加防抖逻辑或在 destroyTreelandToolBar 中校验是否确实需要销毁。

2. destroyTreelandToolBar 中的信号阻断过于粗暴

toolBar->blockSignals(true);
disconnect(toolBar, nullptr, nullptr, nullptr);
disconnect(nullptr, nullptr, toolBar, nullptr);
  • 问题blockSignals(true)disconnect 同时使用是冗余的。此外,disconnect(nullptr, nullptr, toolBar, nullptr) 会断开所有发送者指向 toolBar 的信号,如果 toolBar 内部有动态连接的信号,可能会影响 Qt 的对象树通信机制。
  • 改进建议:在 updateCaptureRegion 创建 ToolBar 时,使用命名的连接方式(如 lambda 或专门的槽函数),在销毁时只断开特定的连接。通常,delete toolBar 会自动断开与该对象相关的所有信号连接,因此在 delete 之前只需确保不会有槽函数在 delete 过程中被触发即可,直接 disconnect(toolBar, nullptr, this, nullptr); 更精准。

3. ImageBorderHelper::getBorderMenu 每次调用都执行 pruneBorderMenus()

ImageMenu *ImageBorderHelper::getBorderMenu(const BorderType type, const QString title, QWidget *parent) {
    pruneBorderMenus(); // 每次获取都遍历清理
    // ...
}
  • 问题getBorderMenu 是一个高频查询/获取接口,在这里面进行全表扫描清理是不合适的,会带来不必要的性能开销。
  • 改进建议pruneBorderMenus() 应该只在确实发生销毁动作时调用(例如在 destroyTreelandToolBar 结束后主动调用一次),或者利用 QPointer 的特性,在访问时直接判断 isNull() 即可,无需提前 erase

二、 代码质量

1. 空指针检查的代码冗余
Diff 中引入了大量的 if (m_toolBar)if (m_toolbarWidget) 检查,虽然保证了安全性,但让代码显得非常臃肿。

  • 改进建议
    • 对于 m_toolbarWidget:考虑在 ToolBar 类内部提供统一的空指针安全包装方法,或者确保 ToolBar 的公共接口在 m_toolbarWidget 为空时具有默认安全行为(例如直接返回默认值,或在函数入口统一 Q_ASSERT(m_toolbarWidget))。
    • 对于 m_toolBar:提取公共的检查逻辑。例如:
    // 修改前
    if (m_toolBar) { m_toolBar->hide(); }
    // 建议封装辅助宏或函数,或利用 Qt 的安全调用特性
    不过考虑到这是 C++ 且涉及 UI 操作,当前的显式检查是最稳妥的,但建议统一代码风格,部分简单逻辑可以使用三元运算符或短路与(&&)来减少行数。

2. updateCaptureRegion 中的代码组织
updateCaptureRegion 函数现在承担了销毁旧工具栏、创建新工具栏、设置属性、更新位置、绑定父窗口等职责,违反了单一职责原则。

  • 改进建议:将“创建并初始化工具栏”的逻辑抽取为 createTreelandToolBar(const QRect& region),将“更新选区变量”的逻辑抽离,使 updateCaptureRegion 作为一个高阶协调者,代码会更清晰。

3. 遗留的冗余代码

m_toolBar->showWidget();
const QPoint pos(region.x(), qMin(region.bottom(), height() - 2 * m_toolBar->height()));
m_toolBar->showAt(pos);
// m_toolBar->showWidget(); // 这行在 Diff 中被删除了,很好,之前是重复调用

这部分清理得很好,但注意 height() - 2 * m_toolBar->height() 中的 height() 指的是 MainWindow 的高度,建议显式调用 this->height() 以增加可读性,避免与局部变量混淆。

三、 代码性能

1. Wayland 下 setParent 引起的隐藏/显示重绘

if (windowHandle() && m_toolBar->windowHandle()
    && m_toolBar->windowHandle()->parent() != windowHandle()) {
    m_toolBar->windowHandle()->setParent(windowHandle());
    m_toolBar->hide();
    m_toolBar->show();
}
  • 问题:在 Wayland 下,QWindow::setParent 会触发底层 wl_surface 的重建或重排,紧接着的 hide()show() 会导致连续的重绘和合成器交互,可能导致画面闪烁。
  • 改进建议:在 setParent 之前就 hide(),设置完成后再 show()。或者确认 Qt 在 Wayland 下 setParent 是否必须伴随显式的 hide/show,有时调用 show() 会自动完成必要的窗口重组。

2. QMap 的遍历与删除
pruneBorderMenus() 中使用了 QMap::erase 返回值进行迭代,这是正确的且高效的(O(1) 均摊时间复杂度)。但如前所述,调用频率需要优化。

四、 代码安全

1. 信号与槽的 Qt::UniqueConnection

connect(manager->context(), &TreelandCaptureContext::captureRegionChanged,
        this, &MainWindow::updateCaptureRegion, Qt::UniqueConnection);
  • 优点:防止了多次连接导致 updateCaptureRegion 被重复执行,这是一个很好的安全加固。

2. QPointer 解决悬空指针
m_allBorderMenu 的值类型从 ImageMenu* 改为 QPointer<ImageMenu> 是本次 PR 的一大亮点。

  • 优点:当 ToolBar 销毁导致 ImageMenu 被 Qt 对象树析构时,QPointer 会自动置为 nullptr,彻底解决了由于缓存导致的野指针崩溃问题。
  • 注意点:在 getBorderMenu 中:
    if (ImageMenu *cached = m_allBorderMenu.value(type)) {
        if (cached->parent() == parent) {
            return cached;
        }
        m_allBorderMenu.remove(type);
        delete cached; // 这里的 delete 是安全的,但如果 cached 是外部传入的 parent 的子对象,需小心重复删除
    }
    这里 delete cached 后,由于它是 QPointer,映射表中的值会自动变为 nullptr。紧接着 m_allBorderMenu.insert(type, menu) 会覆盖这个 nullptr。逻辑是安全的。

3. delete toolBar 前的 windowHandle()->setParent(nullptr)

if (toolBar->windowHandle()) {
    toolBar->windowHandle()->setParent(nullptr);
}
  • 优点:在 Wayland/Treeland 环境下,这非常关键。它确保了在销毁 ToolBar 的 C++ 对象前,先解绑底层的 Wayland subsurface,避免合成器收到 wl_surface 的意外销毁事件或访问已释放的底层资源,是极其重要的安全措施。

总结与核心修改建议代码示例

本次改动整体质量很高,思路清晰,对 Wayland 下 ToolBar 的生命周期管理更加合理。建议在以下方面进行微调:

// 1. 优化 updateCaptureRegion 的职责划分
void MainWindow::updateCaptureRegion()
{
    auto context = TreelandCaptureManager::instance()->context();
    if (!context) return;

    const QRect region = context->captureRegion().toRect();
    if (region.width() <= 0 || region.height() <= 0) {
        destroyTreelandToolBar();
        return;
    }

    // 如果位置没变且工具栏存在,可以考虑不重建(视 Wayland 绑定逻辑而定)
    // 如果必须重建:
    destroyTreelandToolBar();
    createAndShowTreelandToolBar(region); // 抽离创建逻辑
}

// 2. 精简 destroyTreelandToolBar 的断开连接逻辑
void MainWindow::destroyTreelandToolBar()
{
    if (!m_toolBar) return;

    ToolBar *toolBar = m_toolBar;
    m_toolBar = nullptr;
    m_toolBarInit = false;

    if (toolBar->windowHandle()) {
        toolBar->windowHandle()->setParent(nullptr);
    }
    
    // 精准断开与 MainWindow 的连接,不用全局断开
    disconnect(toolBar, nullptr, this, nullptr); 
    
    toolBar->hide();
    delete toolBar; // delete 会自动断开所有与 toolBar 相关的信号
    
    // 清理悬空指针
    ImageBorderHelper::instance()->pruneBorderMenus();
}

// 3. ImageBorderHelper::getBorderMenu 移除高频的 pruneBorderMenus 调用
ImageMenu *ImageBorderHelper::getBorderMenu(const BorderType type, const QString title, QWidget *parent)
{
    // 不再每次都 pruneBorderMenus,利用 QPointer 的特性直接判断
    QPointer<ImageMenu>& cachedRef = m_allBorderMenu[type];
    if (!cachedRef.isNull()) {
        if (cachedRef->parent() == parent) {
            return cachedRef;
        }
        // 父对象变了,删除旧的
        delete cachedRef; 
        cachedRef = nullptr;
    }

    ImageMenu *menu = new ImageMenu(type, title, parent);
    cachedRef = menu; // QPointer 自动接管
    return menu;
}

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengzhongyuan365-dev, lzwind

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

@dengzhongyuan365-dev
Copy link
Copy Markdown
Contributor Author

/merge

@deepin-bot deepin-bot Bot merged commit e9f3d2b into linuxdeepin:master May 19, 2026
10 checks passed
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.

3 participants