D24755: [KStatusNotifierItem] Optionaly, do not take menu ownership

[Anthony Fieroni]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191019/9a49ac46/attachment.html>