anthonyfieroni added inline comments. INLINE COMMENTS
> kmaterka wrote in kstatusnotifieritem.cpp:790 > "associatedWidget" is a KSNI parent (line 780). It might be or might not be > set. If parent is not set, then "associatedWidget" is null and QMenu does not > have parent. This is fine, we will delete it. But if there is parent then > menu won't be deleted and we will have memory leak. Eventually this menu will > be deleted, when parent is destroyed, but current contract is different. > To make things even worse, associatedWidget can change, so we can't compare > the parent of the menu with associatedWidget during the destruction... > Let's say we will change it to: > > new QMenu() > > Then it will be removed, because there is no parent. It should not have any > important side effects. So far so good. > > What we want to achieve is have an ability to retake ownership after it is > passed to setContextMenu. With your approach, it could be done this way: > QMenu *menu =new QMenu(); // null parent, doesn't matter here > tray->setContextMenu(menu); > menu->setParent(parent); > This way menu won't be deleted. There are two problems with this approach: > > - we don't know if no-one is doing that - most probably not and this can be > ignored > - the parent needs to be a QWidget type. This is serious issue, because there > are cases when it is not possible. > > The final goal is to fix KDEPlatformSystemTrayIcon and there is no QWidget to > use as a parent :( Exactly: > kdeplatformsystemtrayicon.cpp:339 > > m_sni->setContextMenu(ourMenu->menu()); > > ourMenu can live longer than KStatusNotifierItem *m_sni and can be reused for > another KStatusNotifierItem instance. The situation is described in BUG > 365105 <https://bugs.kde.org/show_bug.cgi?id=365105>. In other word, in QPA, > menu can and should live independently to system tray icon, which is not the > case in KStatusNotifierItem. > > I really like your idea! Maybe I'm missing something obvious and is possible > to set parent in KDEPlatformSystemTrayIcon... That's easy checkable if (!qApp->closingDown() && (!d->menu->parent() || d->menu->parent() == d->associatedWidget) { delete d->menu; } We should not own the menu, that's not tight approach at least. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D24755 To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
