Skip to content

fix: correct context menu coordinate mapping#448

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
mhduiy:menu
Apr 16, 2026
Merged

fix: correct context menu coordinate mapping#448
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
mhduiy:menu

Conversation

@mhduiy
Copy link
Copy Markdown
Contributor

@mhduiy mhduiy commented Apr 10, 2026

Changed the DockContextMenu parent from 'this' to 'nullptr' in PluginItem constructor to fix coordinate mapping issues when right-click menu is shown. The issue occurred because using 'this' as parent caused the menu's coordinate system to be relative to the PluginItem widget, leading to incorrect position mapping when the menu was displayed. By setting parent to nullptr, the menu becomes a top-level widget with screen coordinates, ensuring proper positioning. Also added explicit cleanup in destructor to safely delete the menu pointer.

Log: Fixed incorrect right-click menu position for dock plugin items

fix: 修复上下文菜单坐标映射问题

将 PluginItem 构造函数中 DockContextMenu 的父对象从 'this' 改为 'nullptr',以修复显示右键菜单时的坐标映射问题。问题原因是使用 'this' 作
为父对象会导致菜单的坐标系相对于 PluginItem 小部件,从而在显示菜单时产
生错误的位置映射。通过将父对象设置为 nullptr,菜单成为使用屏幕坐标的顶
层小部件,确保正确定位。同时在析构函数中添加了显式清理逻辑以安全删除菜单
指针。

Log: 修复了任务栏插件项右键菜单位置不正确的问题

PMS: BUG-352159

Summary by Sourcery

Fix context menu positioning for dock plugin items by adjusting the menu’s ownership and lifecycle management.

Bug Fixes:

  • Correct right-click context menu coordinate mapping for dock plugin items so menus appear at the correct screen position.

Enhancements:

  • Update plugin item source file copyright year range to include 2026.

@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented Apr 10, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adjusts the DockContextMenu lifetime and parenting for PluginItem so the context menu uses global screen coordinates instead of widget-relative coordinates, and adds explicit cleanup to avoid leaks after making it a top-level widget.

Sequence diagram for updated context menu coordinate mapping

sequenceDiagram
    actor User
    participant PluginItem
    participant DockContextMenu

    User->>PluginItem: rightClick(eventPosInWidget)
    PluginItem->>PluginItem: mapToGlobal(eventPosInWidget)
    PluginItem->>DockContextMenu: setPosition(globalPos)
    PluginItem->>DockContextMenu: exec()

    Note over DockContextMenu: Parent is nullptr, menu is top level
    DockContextMenu-->>User: showAt(globalPos)
Loading

Class diagram for PluginItem and DockContextMenu ownership changes

classDiagram
    class QWidget
    class DockContextMenu
    class PluginsItemInterface

    class PluginItem {
        -QString m_itemKey
        -PluginsItemInterface* m_pluginsItemInterface
        -DockContextMenu* m_menu
        -QTimer* m_tooltipTimer
        -QWidget* m_tipsWidget
        +PluginItem(PluginsItemInterface* pluginItemInterface, QString itemKey, QWidget* parent)
        +~PluginItem()
        +QWidget* itemPopupApplet()
    }

    PluginItem --|> QWidget
    PluginItem o-- DockContextMenu : owns
    PluginItem o-- PluginsItemInterface : uses

    DockContextMenu --|> QWidget

    note for PluginItem "m_menu is created with parent nullptr and explicitly deleted in destructor"
Loading

File-Level Changes

Change Details Files
Make DockContextMenu a top-level menu using screen coordinates instead of PluginItem-relative coordinates.
  • Change DockContextMenu parent in PluginItem constructor from the PluginItem instance to nullptr so the menu is created as a top-level widget using global screen coordinates.
  • Keep other constructor initializations (tooltip timer, tips widget, signals) intact while only adjusting the menu instantiation.
src/loader/pluginitem.cpp
Add explicit destruction logic for the DockContextMenu instance now that it is not parented by PluginItem.
  • Replace the defaulted PluginItem destructor with a custom destructor that deletes m_menu when non-null and then nulls the pointer.
  • Ensure that menu cleanup is explicit to prevent memory leaks once QObject parent-child ownership is removed.
src/loader/pluginitem.cpp
Update SPDX header metadata to reflect the extended copyright year range.
  • Extend SPDX-FileCopyrightText year from 2011 - 2022 to 2011 - 2026.
src/loader/pluginitem.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

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.

Hey - I've left some high level feedback:

  • Since m_menu is always initialized and only deleted in the destructor, the null-check and setting it to nullptr after delete are unnecessary; you can safely simplify the destructor to just delete m_menu; for clarity.
  • Now that DockContextMenu is created with a nullptr parent and manually deleted, consider whether using a parented QObject (e.g., a lightweight wrapper QObject owned by PluginItem) or a smart pointer would better express ownership and reduce the risk of future lifetime/cleanup mistakes if the menu’s lifecycle gets more complex.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Since `m_menu` is always initialized and only deleted in the destructor, the null-check and setting it to `nullptr` after `delete` are unnecessary; you can safely simplify the destructor to just `delete m_menu;` for clarity.
- Now that `DockContextMenu` is created with a `nullptr` parent and manually deleted, consider whether using a parented QObject (e.g., a lightweight wrapper QObject owned by `PluginItem`) or a smart pointer would better express ownership and reduce the risk of future lifetime/cleanup mistakes if the menu’s lifecycle gets more complex.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, mhduiy

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

Changed the DockContextMenu parent from 'this' to 'nullptr' in
PluginItem constructor to fix coordinate mapping issues when right-click
menu is shown. The issue occurred because using 'this' as parent caused
the menu's coordinate system to be relative to the PluginItem widget,
leading to incorrect position mapping when the menu was displayed. By
setting parent to nullptr, the menu becomes a top-level widget with
screen coordinates, ensuring proper positioning. Also added explicit
cleanup in destructor to safely delete the menu pointer.

Log: Fixed incorrect right-click menu position for dock plugin items

fix: 修复上下文菜单坐标映射问题

将 PluginItem 构造函数中 DockContextMenu 的父对象从 'this' 改为
'nullptr',以修复显示右键菜单时的坐标映射问题。问题原因是使用 'this' 作
为父对象会导致菜单的坐标系相对于 PluginItem 小部件,从而在显示菜单时产
生错误的位置映射。通过将父对象设置为 nullptr,菜单成为使用屏幕坐标的顶
层小部件,确保正确定位。同时在析构函数中添加了显式清理逻辑以安全删除菜单
指针。

Log: 修复了任务栏插件项右键菜单位置不正确的问题

PMS: BUG-352159
@mhduiy
Copy link
Copy Markdown
Contributor Author

mhduiy commented Apr 16, 2026

/forcemerge

@deepin-bot
Copy link
Copy Markdown

deepin-bot Bot commented Apr 16, 2026

This pr force merged! (status: blocked)

@deepin-bot deepin-bot Bot merged commit 00a6ea9 into linuxdeepin:master Apr 16, 2026
7 of 11 checks passed
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

针对这段 git diff 的代码审查,我将从语法逻辑、代码质量、代码性能和代码安全四个维度进行分析,并提供改进意见。

1. 语法逻辑

  • 版权年份更新
    • 现状:将 SPDX-FileCopyrightText 从 2011-2022 更新至 2011-2026。
    • 评价:逻辑正确,符合维护长期项目版权信息的规范。建议确保 2026 是预期的维护截止年份,或者如果项目持续维护,应使用当前年份或未来较近的年份(如 2024),以免过早更新导致后续年份需要频繁修改。
  • 析构函数逻辑
    • 现状:删除了 = default,改为显式检查 m_menu 指针并执行 delete
    • 评价:逻辑上是成立的,但在现代 C++ (C++11 及以后) 中,这种写法属于"反模式"(Anti-pattern)。

2. 代码质量

  • 父子对象关系变更
    • 现状m_menu(new DockContextMenu(nullptr)) 将菜单的父对象设为 nullptr
    • 问题:在 Qt 中,QObject 派生类通常使用父子对象树来管理内存。将父设为 nullptr 意味着放弃了 Qt 的自动内存管理机制。
    • 改进意见:如果是为了防止菜单随 PluginItem 销毁而销毁(即菜单需要在 PluginItem 析构后依然存在),则需要显式管理其生命周期(如代码中在析构函数里 delete)。但如果菜单的生命周期完全依附于 PluginItem,将其父对象设为 this 是更好的选择,这样无需手写析构函数。
  • 手动内存管理
    • 现状:在析构函数中手动 delete m_menu 并置空。
    • 问题
      1. 冗余:如果 m_menu 的父对象是 this(即之前的代码),Qt 会自动销毁子对象,无需手动 delete。
      2. 不安全:如果 m_menu 的父对象不是 this(即现在的代码),手动 delete 是必须的,但代码中 delete 后置空 m_menu = nullptr 在析构函数中是多余的,因为对象即将被销毁,访问成员变量的机会不存在了。
    • 改进意见:如果必须手动管理(父为 nullptr),只需 delete m_menu; 即可。如果可以使用 Qt 对象树,应恢复 new DockContextMenu(this) 并使用 = default 析构函数。

3. 代码性能

  • 析构函数开销
    • 现状:增加了一个 if 判断。
    • 评价:性能影响微乎其微,可以忽略不计。

4. 代码安全

  • 悬空指针风险
    • 现状:将 m_menu 的父设为 nullptr,并在析构函数中删除。
    • 风险:这种做法引入了手动内存管理的风险。如果在 PluginItem 析构之前,有其他代码路径导致 m_menu 被删除,或者 m_menu 被移动/转移给其他对象,析构函数中的 delete 将导致 Double Free(双重释放),引发程序崩溃。
  • 异常安全
    • 现状:析构函数中抛出异常是未定义行为。虽然 delete 通常不抛异常,但手动管理内存增加了复杂性。
    • 改进意见:使用智能指针(如 std::unique_ptrQScopedPointer)来管理 m_menu,可以彻底杜绝内存泄漏和 Double Free 的风险,且无需编写析构函数。

总结与改进建议

这段代码的主要问题在于放弃了 Qt 的自动内存管理机制,转而使用了不安全的 C 风格手动内存管理,且没有带来明显的收益。

建议方案 1:恢复 Qt 对象树管理(推荐)
如果菜单不需要在 PluginItem 销毁后继续存在,这是最安全、最简洁的方式。

// 构造函数中
, m_menu(new DockContextMenu(this)) // 恢复父对象为 this

// 析构函数中
PluginItem::~PluginItem() = default; // 恢复默认析构

建议方案 2:使用智能指针管理
如果菜单必须独立于 PluginItem 的生命周期(例如需要在插件卸载后保留菜单一段时间),请使用智能指针代替裸指针。

// 头文件中
#include <QScopedPointer>
// ...
class PluginItem : public QWidget {
    // ...
private:
    QScopedPointer<DockContextMenu> m_menu; // 使用 QScopedPointer
    // ...
};

// 源文件中
// 构造函数中
, m_menu(new DockContextMenu(nullptr)) // 父可以为 nullptr,由 QScopedPointer 接管

// 析构函数中
PluginItem::~PluginItem() = default; // QScopedPointer 会自动删除

建议方案 3:如果必须保持现状(不推荐)
如果由于特殊架构原因必须保持父为 nullptr 且手动 delete,请确保:

  1. 确保没有任何其他地方会 delete 这个指针。
  2. 简化析构函数代码:
PluginItem::~PluginItem()
{
    delete m_menu; // 直接删除,无需 if 判断(delete nullptr 是安全的)
    // 无需 m_menu = nullptr;
}

最终建议:优先采用 方案 1,除非有明确的架构需求证明菜单不能作为子对象,否则不要引入不必要的内存管理复杂度。

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