<table><tr><td style="">gladhorn 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/D23067">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/D23067#inline-130050">View Inline</a><span style="color: #4b4d51; font-weight: bold;">romangg</span> wrote in <span style="color: #4b4d51; font-weight: bold;">useractions.cpp:137</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Ok, how about:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">Leave the assert</li>
<li class="remarkup-list-item">Afterwards directly do the cl.isNull() check</li>
<li class="remarkup-list-item">Add a comment above that you are not yet sure if the assert really holds always</li>
<li class="remarkup-list-item">Remove all m_client checks afterwards anyway (since we checked cl.isNull() and I don't see how the pointer can get deleted in between. It's running in a single thread.</li>
</ul>

<p style="padding: 0; margin: 8px;">But looking at <tt style="background: #ebebeb; font-size: 13px;">Workspace::slotWindowOperations</tt>, the only place show is called from, it checks before that the argument is non-null. So we should not need a check.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I generally agree, but I wonder if the checks are there for a reason. And keep in mind, that this kind of nullptr check is very cheap, it's something that is so common that it's very optimized. I don't mind removing the first checks, as it was before, but I'd rather not cause a regression with the later ones. As soon as the popup is open, I'd assume nothing is guaranteed any more, since the actions in the menu could lead to clients being deleted (I'm not entirely sure).</p>

<p style="padding: 0; margin: 8px;">show is called by Workspace::showWindowMenu which is called by three places without checking itself as far as I can tell (Workspace::slotWindowOperations, AbstractClient::performMouseCommand and DecoratedClientImpl::requestShowWindowMenu).</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R108 KWin</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D23067">https://phabricator.kde.org/D23067</a></div></div><br /><div><strong>To: </strong>gladhorn, KWin<br /><strong>Cc: </strong>romangg, zzag, kwin, LeGast00n, sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, Pitel, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart<br /></div>