D23067: Clean up usage of m_client

[Frederik Gladhorn]

gladhorn added inline comments.

INLINE COMMENTS

> romangg wrote in useractions.cpp:137
> Ok, how about:
> 
> - Leave the assert
> - Afterwards directly do the cl.isNull() check
> - Add a comment above that you are not yet sure if the assert really holds always
> - 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.
> 
> But looking at `Workspace::slotWindowOperations`, the only place show is called from, it checks before that the argument is non-null. So we should not need a check.

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

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

REPOSITORY
  R108 KWin

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

To: gladhorn, #kwin
Cc: 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwin/attachments/20190810/fae8ff8c/attachment-0001.html>