<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/D27088">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/D27088#inline-155247">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">sortedsystemtraymodel.cpp:47</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I think calling QSortFilterProxyModel::lessThan(left, right); would do the same</p>

<p style="padding: 0; margin: 8px;">then you don't need compareDisplayAlphabetically</p>

<p style="padding: 0; margin: 8px;">your implementation looks fine though, so do whichever</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The question of being implicit or explicit. I like your idea more, will change that.</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/D27088#inline-155245">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">sortedsystemtraymodel.h:35</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">We tend not to write virtual at the start now we have the clearer override keyword</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Another embarrassing lack of C++ knowledge... Especially this "new" (hehe, C++11) feature :)</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/D27088#inline-155249">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">systemtraymodel.cpp:115</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">The applet is still alive after removeApplet is called. "this" is still alive</p>

<p style="padding: 0; margin: 8px;">dataItem is not.</p>

<p style="padding: 0; margin: 8px;">If an applet is added, removed   and then (potentially during applet teardown) it changes its title, this would crash.</p>

<p style="padding: 0; margin: 8px;">I don't know if that's a realistic scenario or not,  but I would still maybe disconnect all signals from applet -> this on removeApplet?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">During lifetime of the SystemTray, dataItems are never removed, they are just marked as not "renderable". Item is still alive, it is needed in configuration page (you can enable applet again in the configuration).</p>

<p style="padding: 0; margin: 8px;">Anyway, it is a good idea to disconnect signals, for the sake of consisteny. In addition, dataItems are deleted eventually which may (?) lead to a crash on shutdown.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R120 Plasma Workspace</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D27088">https://phabricator.kde.org/D27088</a></div></div><br /><div><strong>To: </strong>kmaterka, Plasma: Workspaces, Plasma, davidedmundson, ngraham, broulik<br /><strong>Cc: </strong>mart, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra<br /></div>