fix: correct context menu coordinate mapping#448
fix: correct context menu coordinate mapping#448deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts 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 mappingsequenceDiagram
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)
Class diagram for PluginItem and DockContextMenu ownership changesclassDiagram
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"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since
m_menuis always initialized and only deleted in the destructor, the null-check and setting it tonullptrafterdeleteare unnecessary; you can safely simplify the destructor to justdelete m_menu;for clarity. - Now that
DockContextMenuis created with anullptrparent and manually deleted, consider whether using a parented QObject (e.g., a lightweight wrapper QObject owned byPluginItem) 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.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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
|
/forcemerge |
|
This pr force merged! (status: blocked) |
deepin pr auto review针对这段 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
总结与改进建议这段代码的主要问题在于放弃了 Qt 的自动内存管理机制,转而使用了不安全的 C 风格手动内存管理,且没有带来明显的收益。 建议方案 1:恢复 Qt 对象树管理(推荐) // 构造函数中
, m_menu(new DockContextMenu(this)) // 恢复父对象为 this
// 析构函数中
PluginItem::~PluginItem() = default; // 恢复默认析构建议方案 2:使用智能指针管理 // 头文件中
#include <QScopedPointer>
// ...
class PluginItem : public QWidget {
// ...
private:
QScopedPointer<DockContextMenu> m_menu; // 使用 QScopedPointer
// ...
};
// 源文件中
// 构造函数中
, m_menu(new DockContextMenu(nullptr)) // 父可以为 nullptr,由 QScopedPointer 接管
// 析构函数中
PluginItem::~PluginItem() = default; // QScopedPointer 会自动删除建议方案 3:如果必须保持现状(不推荐)
PluginItem::~PluginItem()
{
delete m_menu; // 直接删除,无需 if 判断(delete nullptr 是安全的)
// 无需 m_menu = nullptr;
}最终建议:优先采用 方案 1,除非有明确的架构需求证明菜单不能作为子对象,否则不要引入不必要的内存管理复杂度。 |
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:
Enhancements: