Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sorokin
Copy link
Contributor

@sorokin sorokin commented Feb 18, 2020

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?

@eteran
Copy link
Owner

eteran commented Feb 20, 2020

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.

@sorokin
Copy link
Contributor Author

sorokin commented Feb 20, 2020

Thank you for the response.

I've been pretty busy at work, but I'll try to take a closer look asap.

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!

@eteran
Copy link
Owner

eteran commented Feb 26, 2020

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 globalShortcuts concept. I see what you're going for with it, but it kinda makes sense that shortcuts which happen to have a shortcut will "just work" and not need to be part of a separate list that could be forgotten to get updated.

@sorokin
Copy link
Contributor Author

sorokin commented Feb 27, 2020

Though I'm unsure about the globalShortcuts concept.

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.

@eteran
Copy link
Owner

eteran commented Feb 28, 2020

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.
@sorokin
Copy link
Contributor Author

sorokin commented Feb 29, 2020

I updated the PR by removing unneeded usage of std::cerr that I used for debug.

diff --git a/src/Debugger.cpp b/src/Debugger.cpp
index e3590e70..0acb725e 100644
--- a/src/Debugger.cpp
+++ b/src/Debugger.cpp
@@ -863,7 +863,6 @@ void Debugger::finishPluginSetup() {
                        for (QAction *action : actions) {
                                QKeySequence shortcut = action->shortcut();
                                if (!shortcut.isEmpty()) {
-                                       std::cerr << shortcut.toString().toUtf8().constData() << std::endl;
                                        connect(new QShortcut(shortcut, this), &QShortcut::activated, action, &QAction::trigger);
                                }
                        }

@eteran
Copy link
Owner

eteran commented Dec 14, 2020

@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 :-).

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.

None yet

2 participants