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

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kstatusnotifieritem.cpp:454
> Do not take ownership of the menu and delete it when it does not have a parent. takeOwnership is wrong approach, you can remove it.

In theory that should be the correct approach, the "Qt way". But we have existing contract (also discussed in D24667 <https://phabricator.kde.org/D24667>), documentation is clear, menu ownership is always transferred and menu removed, regardless of the parent (please check my comment in line 790, parent might be null or might not be null).

> kstatusnotifieritem.cpp:790
>      //create a default menu, just like in KSystemtrayIcon
>      QMenu *m = new QMenu(associatedWidget);
>  

"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...

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/28731c4c/attachment-0001.html>