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

[Konrad Materka]

kmaterka added a comment.


  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:
  
  > d->menu->setParent(nullptr);
  
  in setContextMenu.
  
  Then, to prevent menu deletion, developer needs to explicitly retake ownership, for example:
  
    QMenu *menu = ourMenu->menu()
    QWidget *parent = menu->parent();
    m_sni->setContextMenu(menu);
    menu->setParent(parent);
  
  The problem is that SystemTrayMenu->menu() has no parent and there is no QWidget to use...

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