<div>hein marked 7 inline comments as done.<br />
hein added inline comments.</div><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/D1722#inline-6682" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:18-23</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">do we want to have X11 conditional? Other areas of Plasma use X11 unconditionally.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Personally as maintainer I do :). From my POV X11 is a legacy windowing system for libtaskmanager and I want to support building without it. If nothing else being disciplined about that helps a bit to prevent X11 stuff from creeping over the codebase again ...</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/D1722#inline-6683" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:51</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Qt5::X11Extras</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Done.</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/D1722#inline-6692" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">tasksmodel.cpp:134</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">you need a <br />
 --d->activityInfoUsers</p>

<p style="padding: 0; margin: 8px;">otherwise:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">set sort mode activities</li>
<li class="remarkup-list-item">delete task bar</li>
<li class="remarkup-list-item">add a task bar</li>
<li class="remarkup-list-item">set sort mode activities</li>
<li class="remarkup-list-item">set sort mode something else</li>
</ul>

<p style="padding: 0; margin: 8px;">you leak an activityInfo object.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Done.</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/D1722#inline-6690" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">tasksmodel.cpp:478</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">invalidateFilter()?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">invalidateFilter() doesn't do a full resort - it only sorts in rows that may now get past the filter, leaving already-sorted rows alone. Unfortunately this was the best way I found to get QSortFilterProxyModel to guaranteed sort all rows.</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/D1722#inline-6695" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">tasksmodel.cpp:1220</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">would this be clearer and faster if it connected to groupsModel::rowsInserted ?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Quite possibly, I'll give it a try. I originally had everything in filterAcceptsRow() and the code then evolved more, and this was kind of left on the table along the way I think.</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/D1722#inline-6691" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">tasksmodel.cpp:84</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">const</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Done.</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/D1722#inline-6652" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xwindowtasksmodel.cpp:307</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">We're missing some changed roles</p>

<p style="padding: 0; margin: 8px;">IsKeepAbove<br />
IsKeepBelow<br />
SkipTaskbar</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Done.</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/D1722#inline-6653" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xwindowtasksmodel.cpp:313</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">IsVirtualDesktopChangeable</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Done.</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/D1722#inline-6655" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xwindowtasksmodel.cpp:434</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">we don't want to open and parse a config file on every new window.<br />
It'll be worth keeping the kconfig object.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I've added a FIXME to optimize this at a later time. It's not worse than the old lib (it did this too).</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/D1722#inline-6649" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xwindowtasksmodel.cpp:756</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">you can do all this in the QByteArray ctor</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Done.</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/D1722#inline-6647" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xwindowtasksmodel.cpp:861</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">what's with the Q_INVOKABLE?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Copy and paste mistake probably, fixed.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>rPLASMAWORKSPACE Plasma Workspace</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D1722" rel="noreferrer">https://phabricator.kde.org/D1722</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>hein, Plasma<br /><strong>Cc: </strong>graesslin, broulik, davidedmundson, plasma-devel, sebas<br /></div>