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

[Konrad Materka]

kmaterka added a comment.


  > 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.
  
  Yes, eventually menu will be deleted. I'm thinking about the situation when someone has:
  
    if (configuration-trayIconEnabled) {
       m_sni = new KStatusNotifierItem();
       m_sni->setContextMenu(new QMenu(**this**));
    } else {
      delete m_sni;
    }
  
  and user is playing with this setting and changes it 100 times during one session. It will create 100 QMenu instances.
  
  > 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.
  
  That's how QSystemTrayIcon works, I checked the source code. QSystemTrayIcon::show() and QSystemTrayIcon::hide will create and destroy KDEPlatformSystemTrayIcon (this is a little bit more complicated, because there are also two additional methods involved: init and cleanup). 
  For menu, QSystemTrayIcon uses:
  
    QPlatformMenu* KDEPlatformSystemTrayIcon::createMenu()
  
  which is kept alive until QSystemTrayIcon instance exist.
  So something like this:
  
    QSystemTrayIcon *sysTray = new QSystemTrayIcon(this);
    sysTray->setContextMenu(new QMenu()); // create QPlatformMenu (AFAIR it is postponed, but you get the idea)
    sysTray->show(); // create QPlatformSystemTrayIcon
    sysTray->hide(); // delete QPlatformSystemTrayIcon
    sysTray->show(); // create second QPlatformSystemTrayIcon and reuse QPlatformMenu
  
  Will create two instances of KDEPlatformSystemTrayIcon (QPlatformSystemTrayIcon) and only one of SystemTrayMenu (QPlatformMenu).
  
  Anyway, this whole "do not take ownership" is a dead end. Even if menu is not deleted, it won't work, there is another issue in dbusmenu-qt library... I abandon this idea, sorry for taking your time. I have another solution, in:
  
    void KDEPlatformSystemTrayIcon::updateMenu(QPlatformMenu *menu)
    {
        if (SystemTrayMenu *ourMenu = qobject_cast<SystemTrayMenu*>(menu)) {
            m_sni->setContextMenu(ourMenu->menu());
        }
    }
  
  SystemTrayMenu::menu() will return new QMenu and keep track of changes.

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