<table><tr><td style="">hein marked 11 inline comments as done.<br />hein 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/D1722" rel="noreferrer">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/D1722#inline-6858" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">waylandtasksmodel.cpp:115-119</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">at the point of adding a window you can be quite sure that the appId is not yet pushed. Maybe we should introduce a dedicated event in the protocol to signal that the initial known data is pushed to the client. That's e.g. how Output does it.</p>

<p style="padding: 0; margin: 8px;">Which could be nice to ensure that the task model doesn't start adding any elements before it knows what to do with it. Also for the case that the window is already unmapped.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I've added a FIXME until we figure that out.</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-6859" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">waylandtasksmodel.cpp:233</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">shouldn't you check whether the index is valid?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">If this check were necessary, something else would be broken somewhere. dataChanged() is called with window instances which are in the list, so indexOf will return something sensible. index() then can't fail because hasIndex() just does a bounds check against rowCount() (which operates on the same list) before calling createIndex() (which doesn't really do anything with data). So the index could only be invalid if dataChanged() were called with a window that's not in Private::windows, which makes no sense.</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-6860" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">waylandtasksmodel.cpp:244-246</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">WaylandTasksModel::~WaylandTasksModel() = default;</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Ok.</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-6861" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">waylandtasksmodel.cpp:256</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">any specific reason why you use an if-else structure instead of a switch?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not really.</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-6862" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">waylandtasksmodel.cpp:359</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">there is no global timestamp like on X11 on Wayland. Timestamps are only used for frame rendered callbacks and on input events. Both are defined to have an undefined base and there is nowhere specified that they have the same base.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Removed FIXME.</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-6864" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">waylandtasksmodel.cpp:490</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">why don't you use the namespace more globally?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">No particular reason. Might change as the code evolves.</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-6866" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xwindowtasksmodel.cpp:115</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">why not KSharedConfig::openConfig?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Changed.</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-6865" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xwindowtasksmodel.cpp:118</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">coding style nitpick: missing space between foreach and (</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Ok.</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-6867" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xwindowtasksmodel.cpp:168</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">you are connecting to a deprecated signal. see <a href="https://api.kde.org/frameworks/kwindowsystem/html/classKWindowSystem.html#ac8d368d83fa5e137f38e7c885d9a18ce" class="remarkup-link" target="_blank" rel="noreferrer">https://api.kde.org/frameworks/kwindowsystem/html/classKWindowSystem.html#ac8d368d83fa5e137f38e7c885d9a18ce</a></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Changed.</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-6868" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xwindowtasksmodel.cpp:198</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">coding style nitpick: missing whitespace</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Ok.</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-6869" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xwindowtasksmodel.cpp:419</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I don't understand this valid check. Why are you inserting non valid KWindowInfo into the cache in the first place?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Paranoia. I'm not inserting invalid ones into the cache, but I figured if there's a valid() it would be a good idea to call it before using it, so that block checked valid() and evicted if false. I've removed the block now.</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, graesslin<br /><strong>Cc: </strong>graesslin, broulik, davidedmundson, plasma-devel, sebas<br /></div>