<table><tr><td style="">anthonyfieroni added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D24755">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24755#inline-139955">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kmaterka</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kstatusnotifieritem.cpp:790</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">"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.<br />
To make things even worse, associatedWidget can change, so we can't compare the parent of the menu with associatedWidget during the destruction...<br />
Let's say we will change it to:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">new QMenu()</pre></div>

<p style="padding: 0; margin: 8px;">Then it will be removed, because there is no parent. It should not have any important side effects. So far so good.</p>

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

<ul class="remarkup-list">
<li class="remarkup-list-item">we don't know if no-one is doing that - most probably not and this can be ignored</li>
<li class="remarkup-list-item">the parent needs to be a QWidget type. This is serious issue, because there are cases when it is not possible.</li>
</ul>

<p style="padding: 0; margin: 8px;">The final goal is to fix KDEPlatformSystemTrayIcon and there is no QWidget to use as a parent :( Exactly:<br />
kdeplatformsystemtrayicon.cpp:339</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">m_sni->setContextMenu(ourMenu->menu());</pre></div>

<p style="padding: 0; margin: 8px;">ourMenu can live longer than KStatusNotifierItem *m_sni and can be reused for another KStatusNotifierItem instance. The situation is described in <a href="https://bugs.kde.org/show_bug.cgi?id=365105" class="remarkup-link" target="_blank" rel="noreferrer">BUG 365105</a>. In other word, in QPA, menu can and should live independently to system tray icon, which is not the case in KStatusNotifierItem.</p>

<p style="padding: 0; margin: 8px;">I really like your idea! Maybe I'm missing something obvious and is possible to set parent in KDEPlatformSystemTrayIcon...</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That's easy checkable</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">if (!qApp->closingDown() && (!d->menu->parent() || d->menu->parent() == d->associatedWidget) {
    delete d->menu;
}</pre></div>

<p style="padding: 0; margin: 8px;">We should not own the menu, that's not tight approach at least.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R289 KNotifications</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D24755">https://phabricator.kde.org/D24755</a></div></div><br /><div><strong>To: </strong>kmaterka, Frameworks, davidedmundson, broulik, nicolasfella<br /><strong>Cc: </strong>anthonyfieroni, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>