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

2019-10-21 Thread Konrad Materka
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, michaelh, ngraham, 
bruns


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

2019-10-21 Thread 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(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


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

2019-10-21 Thread Konrad Materka
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, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns


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

2019-10-21 Thread Anthony Fieroni
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 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 

  >
  >   ``` void KDEPlatformSystemTrayIcon::updateMenu(QPlatformMenu *menu) { 
//... if (SystemTrayMenu *ourMenu = qobject_cast(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:
  
  
  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.

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


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

2019-10-21 Thread Konrad Materka
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 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 

  
void KDEPlatformSystemTrayIcon::updateMenu(QPlatformMenu *menu)
{
//...
if (SystemTrayMenu *ourMenu = qobject_cast(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


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

2019-10-19 Thread Anthony Fieroni
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
  https://phabricator.kde.org/D24755

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns


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

2019-10-19 Thread Anthony Fieroni
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 (more?) years. It is even documented in the API.
> Your idea will not fix the main issue, we can't set a parent to menu in 
> KDEPlatformSystemTrayIcon. Main purpose of this hack is to prevent deletion 
> of menu when it is *not* possible to set parent.

`Probably KSNI should not own the menu`
Yes, widget that creates the menu should, like in bug report example. Make the 
changes here, then we should find a way to parent the menu, which is the right 
approach.

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


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

2019-10-19 Thread Konrad Materka
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, that's not tight approach at least.

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 (more?) years. It is even documented in the API.
Your idea will not fix the main issue, we can't set a parent to menu in 
KDEPlatformSystemTrayIcon. Main purpose of this hack is to prevent deletion of 
menu when it is *not* possible to set parent.

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


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

2019-10-19 Thread Anthony Fieroni
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 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.
> To make things even worse, associatedWidget can change, so we can't compare 
> the parent of the menu with associatedWidget during the destruction...
> Let's say we will change it to:
> 
>   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:
> QMenu *menu =new QMenu(); // null parent, doesn't matter here
> tray->setContextMenu(menu);
> menu->setParent(parent);
> This way menu won't be deleted. There are two problems with this approach:
> 
> - we don't know if no-one is doing that - most probably not and this can be 
> ignored
> - the parent needs to be a QWidget type. This is serious issue, because there 
> are cases when it is not possible.
> 
> The final goal is to fix KDEPlatformSystemTrayIcon and there is no QWidget to 
> use as a parent :( Exactly:
> kdeplatformsystemtrayicon.cpp:339
> 
>   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...

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.

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


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

2019-10-19 Thread Konrad Materka
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 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).

> kstatusnotifieritem.cpp:790
>  //create a default menu, just like in KSystemtrayIcon
>  QMenu *m = new QMenu(associatedWidget);
>  

"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.
To make things even worse, associatedWidget can change, so we can't compare the 
parent of the menu with associatedWidget during the destruction...
Let's say we will change it to:

  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:
QMenu *menu =new QMenu(); // null parent, doesn't matter here
tray->setContextMenu(menu);
menu->setParent(parent);
This way menu won't be deleted. There are two problems with this approach:

- we don't know if no-one is doing that - most probably not and this can be 
ignored
- the parent needs to be a QWidget type. This is serious issue, because there 
are cases when it is not possible.

The final goal is to fix KDEPlatformSystemTrayIcon and there is no QWidget to 
use as a parent :( Exactly:
kdeplatformsystemtrayicon.cpp:339

  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...

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


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

2019-10-19 Thread Anthony Fieroni
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 parent. 
takeOwnership is wrong approach, you can remove it.

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


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

2019-10-19 Thread Konrad Materka
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, but it is not 
backward compatible.

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


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

2019-10-19 Thread Konrad Materka
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
  src/kstatusnotifieritem.cpp
  src/kstatusnotifieritem.h
  src/kstatusnotifieritemprivate_p.h

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns


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

2019-10-19 Thread Konrad Materka
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 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)

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.

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


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

2019-10-19 Thread David Edmundson
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, LeGast00n, GB_2, michaelh, ngraham, 
bruns


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

2019-10-19 Thread David Edmundson
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

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)

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


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

2019-10-19 Thread Anthony Fieroni
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 don't need to any hacks here. QPointer takes care of 
ownership.

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


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

2019-10-18 Thread Konrad Materka
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 it as simple as possible.

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


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

2019-10-18 Thread Anthony Fieroni
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: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns


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

2019-10-18 Thread Konrad Materka
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:
  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?

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D24755

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns


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

2019-10-18 Thread Konrad Materka
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 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

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24755

AFFECTED FILES
  src/kstatusnotifieritem.cpp
  src/kstatusnotifieritem.h
  src/kstatusnotifieritemprivate_p.h

To: kmaterka, #frameworks, davidedmundson, broulik, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns