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

kmaterka added a comment. Moved to D24843 REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D24755 To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2,

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

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

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

kmaterka abandoned this revision. kmaterka added a comment. This is a dead end, I will implement different solution in QPA (KDEPlatformSystemTrayIcon) plugin. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D24755 To: kmaterka, #frameworks, davidedmundson,

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

anthonyfieroni added a comment. In D24755#551320 , @kmaterka wrote: > In D24755#550415 , @anthonyfieroni wrote: > > > That should be fine, in QPA we have a tray icon per sni, update menu should be

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

kmaterka added a comment. In 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

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

anthonyfieroni added a comment. F7633806: p.patch 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. REPOSITORY R289 KNotifications REVISION DETAIL

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

anthonyfieroni added inline comments. INLINE COMMENTS > kmaterka wrote in kstatusnotifieritem.cpp:790 > This check is not reliable, assosiatedWidget can change. Anyway, this doesn't > matter here. > Did you read whole comment? Probably KSNI should not own the menu but it is > doing that for 10

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

kmaterka added inline comments. INLINE COMMENTS > anthonyfieroni wrote in 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,

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

anthonyfieroni added inline comments. INLINE COMMENTS > kmaterka wrote in kstatusnotifieritem.cpp: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

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

kmaterka added inline comments. INLINE COMMENTS > anthonyfieroni wrote in 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. In theory that should be the correct approach, the "Qt

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

anthonyfieroni added inline comments. INLINE COMMENTS > kstatusnotifieritem.cpp:454 > Qt::WindowFlags oldFlags = d->menu->windowFlags(); > d->menu->setParent(nullptr); > d->menu->setWindowFlags(oldFlags); Do not take ownership of the menu and delete it when it does not have a

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

kmaterka marked 5 inline comments as done. kmaterka added a comment. In D24755#550140 , @davidedmundson wrote: > I don't exactly like it, but I don't have anything better. +1 Me neither. Probably the best would be to use shared pointer,

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

kmaterka updated this revision to Diff 68292. kmaterka added a comment. Wrap menu in QPointer REPOSITORY R289 KNotifications CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24755?vs=68229=68292 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24755 AFFECTED FILES

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

kmaterka added inline comments. INLINE COMMENTS > davidedmundson wrote in 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

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

davidedmundson added a comment. I don't exactly like it, but I don't have anything better. +1 REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D24755 To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella Cc: anthonyfieroni, kde-frameworks-devel,

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

davidedmundson added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kstatusnotifieritemprivate_p.h:158 > It's related and you don't need to any hacks here. QPointer takes care of > ownership. QPointer here won't solve the other hacks needed. We have MenuSource, Menu and KSNI

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

anthonyfieroni added inline comments. INLINE COMMENTS > kmaterka wrote in 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. It's related and you

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

kmaterka added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kstatusnotifieritemprivate_p.h:158 > It should be QPointer, no other changes are needed. 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

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

anthonyfieroni added inline comments. INLINE COMMENTS > kstatusnotifieritemprivate_p.h:158 > > QMenu *menu; > QHash actionCollection; It should be QPointer, no other changes are needed. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D24755 To:

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

kmaterka added a comment. 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:

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

kmaterka created this revision. kmaterka added reviewers: Frameworks, davidedmundson, broulik, nicolasfella. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. kmaterka requested review of this revision. REVISION SUMMARY It is not possible to handle ownership