<table><tr><td style="">kmaterka 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-139953">View Inline</a><span style="color: #4b4d51; font-weight: bold;">anthonyfieroni</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kstatusnotifieritem.cpp:454</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">In theory that should be the correct approach, the "Qt way". But we have existing contract (also discussed in <a href="https://phabricator.kde.org/D24667" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: line-through;">D24667</a>), 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).</p></div></div><br /><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;">kstatusnotifieritem.cpp:790</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #74777d">//create a default menu, just like in KSystemtrayIcon</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">QMenu</span> <span style="color: #aa2211">*</span><span class="n">m</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QMenu</span><span class="p">(</span><span class="n">associatedWidget</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><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></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>