-
-
Notifications
You must be signed in to change notification settings - Fork 328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC attempt to fix #744 #750
base: master
Are you sure you want to change the base?
Conversation
The idea of threading the main window's pointer to the plugin so it can be set as the parent seems reasonable at first blush, I've been pretty busy at work, but I'll try to take a closer look asap. |
Thank you for the response.
That's totally fine. I don't mind if the PR is reviewed later and is reviewed better. There is no rush in doing this at all. Take your time. Thanks! |
I started to look at this PR, as I said, threading the parent to the plugins seems like a generally good solution. Though I'm unsure about the |
That's why I'm not sure about it either. But I don't know better solution. The problem I had was the following. Conceptually there are 2 types of actions: ones that are created at plugin initialization time and they live while the plugin is loaded, others that are created when menu is invoked and are destroyed when menu is dismissed. For action to have a global shortcut it must be of the first type. Unfortunately the previous interface doesn't allow getting only these global actions. That's why I invented this globalShortcuts function. Let me know if you have any other ideas. As I said I'm not sure about the concept, but I don't have any better ideas. |
I'll think about it, and see if we can come up with a better solution. 👍 |
The issue was about leaks reported by leak sanitizer: Indirect leak of 48 byte(s) in 1 object(s) allocated from: #0 0x7f5c95377d80 in operator new(unsigned long) eteran#1 0x7f5c7e695267 in HardwareBreakpointsPlugin::HardwareBreakpoints::stackContextMenu() HardwareBreakpoints.cpp:253 eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485 eteran#3 0x4e683a in start_debugger main.cpp:96 eteran#4 0x4e683a in main main.cpp:255 Indirect leak of 48 byte(s) in 1 object(s) allocated from: #0 0x7f5c95377d80 in operator new(unsigned long) eteran#1 0x7f5c7e69ba38 in HardwareBreakpointsPlugin::HardwareBreakpoints::cpuContextMenu() HardwareBreakpoints.cpp:324 eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485 eteran#3 0x4e683a in start_debugger main.cpp:96 eteran#4 0x4e683a in main main.cpp:255 Indirect leak of 48 byte(s) in 1 object(s) allocated from: #0 0x7f5c95377d80 in operator new(unsigned long) eteran#1 0x7f5c7e6985d7 in HardwareBreakpointsPlugin::HardwareBreakpoints::dataContextMenu() HardwareBreakpoints.cpp:288 eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485 eteran#3 0x4e683a in start_debugger main.cpp:96 eteran#4 0x4e683a in main main.cpp:255 The most idiomatic way to fix the issue was to assign a parent to the submenus created in these functions. Unfortunately there is no suitable parent available in these functions. This commit fixes the issue in the most straight-forward way possible: it just adds QMenu* parent parameter to the functions * IPlugin::cpuContextMenu * IPlugin::registerContextMenu * IPlugin::stackContextMenu * IPlugin::dataContextMenu This allows plugins to set proper parent to all submenus they choose to create in order for them to be properly destroyed when root menu is destroyed. This commit also sets parents to the QActions that are created in these functions so they are timedly destroyed. As a possible follow-up these QActions can be created once in plugin constructor and not everytime the menu is created.
9427838
to
36125f4
Compare
I updated the PR by removing unneeded usage of std::cerr that I used for debug.
|
@sorokin as you can imagine, it's been a hectic and sometimes very unproductive year. But I'm getting back into a groove and plan to work on this issue in the near future! Thanks for your contribution :-). |
This is more like a request for comments than a real pull request.
Basically what I did is I passed QMenu* parent to all plugin functions that create menus. Full description is in the commit message.
What do you think?