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

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D24755#551320 <https://phabricator.kde.org/D24755#551320>, @kmaterka wrote:
  
  > In D24755#550415 <https://phabricator.kde.org/D24755#550415>, @anthonyfieroni wrote:
  >
  > > That should be fine, in QPA we have a tray icon per sni, update menu should be on same object which will not be a problem, check it.
  >
  >
  > There are two objects in QPA that live independently:
  >
  > - KDEPlatformSystemTrayIcon (QPlatformSystemTrayIcon), with KSNI instance, KSNI and KDEPlatformSystemTrayIcon **are** destroyed on QSystemTrayIcon->hide() and new instance (with new KSNI) is created on QSystemTrayIcon->show()
  > - SystemTrayMenu (QPlatformMenu) is **not** destroyed on QSystemTrayIcon->hide() and will be reused later on QSystemTrayIcon->show()
  >
  >   kdeplatformsystemtrayicon.cpp#L339 <https://github.com/KDE/plasma-integration/blob/master/src/platformtheme/kdeplatformsystemtrayicon.cpp#L339>
  >
  >   ``` void KDEPlatformSystemTrayIcon::updateMenu(QPlatformMenu *menu) { //... if (SystemTrayMenu *ourMenu = qobject_cast<SystemTrayMenu*>(menu)) { m_sni->setContextMenu(ourMenu->menu()); } } ```
  >
  >   About you patch: I understand your idea, but it changes API contract and is not backward-compatible. Current documentation says:
  >
  > > The KStatusNotifierItem instance takes ownership of the menu, and will delete it upon its destruction.
  >
  > This is quite clear, I want to be really careful here - I don't want to be blamed for memory leaks :) I think that we need to keep:
  
  
  First, it will not have memory leaks, we take ownership on parentless menu, on menu that has a parent, it will destroy it by parent-child cleanups.
  I want to be more clear why SystemTrayMenu is not destroyed on hide, the idea behind that code is to be created a new try menu. On updateMenu you call it by same object that's why it's not destroyed, did you have stack strace, that's not normal to me.

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/20191021/28937e61/attachment.html>