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
