It is not possible to handle ownership of QMenu the Qt way, because
QMenu takes QWidget as a parent, but KSNI is a QObject. In some
situations (Qt QPA plugin) we don't want QMenu to be destroyed.
CCBUG: 365105
davidedmundson | |
broulik | |
nicolasfella |
Frameworks |
It is not possible to handle ownership of QMenu the Qt way, because
QMenu takes QWidget as a parent, but KSNI is a QObject. In some
situations (Qt QPA plugin) we don't want QMenu to be destroyed.
CCBUG: 365105
No Linters Available |
No Unit Test Coverage |
Buildable 17895 | |
Build 17913: arc lint + arc unit |
Continuation of D24667. I know this solution sucks, but I have no idea how this can be done better.
I have another idea, probably even crazier: set "DO_NOT_DELETE" flag as a QObject property. Then in destructor check it:
if (d->m_menu->property("DO_NOT_DELETE") == true) // ignore
Obviously that would be undocumented, ugly hack... Any other idea how to fix bug 365105?
src/kstatusnotifieritemprivate_p.h | ||
---|---|---|
158 | It should be QPointer<QMenu>, no other changes are needed. |
src/kstatusnotifieritemprivate_p.h | ||
---|---|---|
158 | Sure, but this is not my code and this is not related. What I want to do here is more like hack/workaround, so it is better to keep it as simple as possible. |
src/kstatusnotifieritemprivate_p.h | ||
---|---|---|
158 | It's related and you don't need to any hacks here. QPointer takes care of ownership. |
src/kstatusnotifieritemprivate_p.h | ||
---|---|---|
158 | QPointer here won't solve the other hacks needed. We have MenuSource, Menu and KSNI QPointer would fix the case of MenuSource still owning the Menu and KSNI lasting longer than the Menu. But we need things the other way round. We have MenuSource lasting longer than KSNI we need to supply a menu to the KSNI but keep the menu after the KSNI is deleted. QPointer won't solve that. (might be worth switching this to a QPointer anyway, now that ksni doesn't control the menu lifespan though) |
src/kstatusnotifieritemprivate_p.h | ||
---|---|---|
158 | I will wrap m_menu in QPointer. When mane "ownership" is outside of the KSNI this is a valid case when menu is deleted before KSNI. That's a good practice anyway. |
Me neither. Probably the best would be to use shared pointer, but it is not backward compatible.
src/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. |
src/kstatusnotifieritem.cpp | ||
---|---|---|
454 | In theory that should be the correct approach, the "Qt way". But we have existing contract (also discussed in 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). | |
790 | "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. 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:
The final goal is to fix KDEPlatformSystemTrayIcon and there is no QWidget to use as a parent :( Exactly: 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. 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... |
src/kstatusnotifieritem.cpp | ||
---|---|---|
790 | That's easy checkable if (!qApp->closingDown() && (!d->menu->parent() || d->menu->parent() == d->associatedWidget) { delete d->menu; } We should not own the menu, that's not tight approach at least. |
src/kstatusnotifieritem.cpp | ||
---|---|---|
790 | This check is not reliable, assosiatedWidget can change. Anyway, this doesn't matter here. |
src/kstatusnotifieritem.cpp | ||
---|---|---|
790 | Probably KSNI should not own the menu |
There are two objects in QPA that live independently:
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...
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.
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.
This is a dead end, I will implement different solution in QPA (KDEPlatformSystemTrayIcon) plugin.