Proper lifetime support for one-time menu objects with contextMenuExtension
ClosedPublic

Authored by kossebau on Jul 18 2017, 11:44 AM.

Details

Summary

For actions and submenus created only for the given context for the menu,
as opposed to persistent context-ignorant actions kept around all the time
and owned by the plugin instance, in IPlugin::contextMenuExtension()
overrides was no access to a QObject/QWidget element which could be used
as QObject-style memory management for the given context menu event.
So the old code for that either used instances like the plugin objects
themselves or even none at all, which both de-facto results in
accumulating lots of out-of-use QActions and QMenus during the runtime
of KDevelop, only to be cleaned up either on destruction of the plugin
instances at process end or never.

Passing to IPlugin::contextMenuExtension a QWidget* object to be used
as memory management parent matching the lifetime of the context menu
allows to fix that.

Test Plan

Invoked the context menus changed by the patch, and the temporarily
added slots connected to QObject::destroyed handlers of the
per-context-menu QAction & QMenu objects were invoked at the right time.
No new crashes seen.

Diff Detail

Repository
R33 KDevPlatform
Branch
fixcontextmenuleakage
Lint
No Linters Available
Unit
No Unit Test Coverage
kossebau created this revision.Jul 18 2017, 11:44 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJul 18 2017, 11:44 AM

I considered adding the QWidget parent to the KDevelop::Context class, so just one argument is passed around as before in the actual context menu content query calls.
Decided against as that meant to me mixing of concerns, for one the object describing read-only the context in which the menu is to appear, while the parent object is to be actively used for memory management of the elements added.
So it felt more clean to have that as explicit separate arguments in the contextMenuExtension() method, instead of having invokers of the contextmenu collection stuffing this into the context description object and providers fetching it again out of that object.

Also went for QWidget as parent and not the QMenu object, even if in the current use-cases it is always the menu, because this way on the callee side it follows a usual parent type with widget classes, so common pattern, and it will avoid implementers being confused about what other things to do with the menu perhaps, when they should not and just use it for memory management purposes.

kossebau updated this revision to Diff 16869.Jul 18 2017, 12:24 PM
  • remove default nullptr (only used during development)
  • small api dox fix with VcsPluginHelper
kfunk accepted this revision.Jul 21 2017, 7:10 PM
kfunk added a subscriber: kfunk.

Thanks, great find.

This revision is now accepted and ready to land.Jul 21 2017, 7:10 PM
kfunk added a comment.Jul 21 2017, 7:11 PM

Please bump KDEV_PLUGIN_VERSION in CMakeLists.txt after pushing this patch.

This revision was automatically updated to reflect the committed changes.