<table><tr><td style="">graesslin requested changes to this revision.<br />graesslin added a reviewer: graesslin.<br />graesslin added inline comments.<br />This revision now requires changes to proceed.</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;">waylandtasksmodel.cpp:115-119</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">KService</span><span style="color: #aa2211">::</span><span class="n">Ptr</span> <span class="n">service</span> <span style="color: #aa2211">=</span> <span class="n">KService</span><span style="color: #aa2211">::</span><span class="n">serviceByStorageId</span><span class="p">(</span><span class="n">window</span><span style="color: #aa2211">-></span><span class="n">appId</span><span class="p">());</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">service</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">serviceCache</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span><span class="n">window</span><span class="p">,</span> <span class="n">service</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><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><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;">waylandtasksmodel.cpp:233</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">QModelIndex</span> <span class="n">idx</span> <span style="color: #aa2211">=</span> <span class="n">q</span><span style="color: #aa2211">-></span><span class="n">index</span><span class="p">(</span><span class="n">windows</span><span class="p">.</span><span class="n">indexOf</span><span class="p">(</span><span class="n">window</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">emit</span> <span class="n">q</span><span style="color: #aa2211">-></span><span class="n">dataChanged</span><span class="p">(</span><span class="n">idx</span><span class="p">,</span> <span class="n">idx</span><span class="p">,</span> <span class="n">QVector</span><span style="color: #aa2211"><</span><span style="color: #aa4000">int</span><span style="color: #aa2211">></span><span class="p">{</span><span class="n">role</span><span class="p">});</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">shouldn't you check whether the index is valid?</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;">waylandtasksmodel.cpp:244-246</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span class="n">WaylandTasksModel</span><span style="color: #aa2211">::~</span><span class="n">WaylandTasksModel</span><span class="p">()</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">WaylandTasksModel::~WaylandTasksModel() = default;</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;">waylandtasksmodel.cpp:256</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">role</span> <span style="color: #aa2211">==</span> <span class="n">Qt</span><span style="color: #aa2211">::</span><span class="n">DisplayRole</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #aa4000">return</span> <span class="n">window</span><span style="color: #aa2211">-></span><span class="n">title</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">any specific reason why you use an if-else structure instead of a switch?</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;">waylandtasksmodel.cpp:359</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #74777d">// FIXME Timestamp on Wayland?</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #aa4000">new</span> <span style="color: #004012">KRun</span><span class="p">(</span><span class="n">QUrl</span><span style="color: #aa2211">::</span><span class="n">fromLocalFile</span><span class="p">(</span><span class="n">service</span><span style="color: #aa2211">-></span><span class="n">entryPath</span><span class="p">()),</span> <span style="color: #601200">0</span><span class="p">,</span> <span style="color: #304a96">false</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><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><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;">waylandtasksmodel.cpp:490</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #aa4000">using</span> <span style="color: #aa4000">namespace</span> <span class="n">KWayland</span><span style="color: #aa2211">::</span><span class="n">Client</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">Surface</span> <span style="color: #aa2211">*</span><span class="n">surface</span> <span style="color: #aa2211">=</span> <span class="n">Surface</span><span style="color: #aa2211">::</span><span class="n">fromWindow</span><span class="p">(</span><span class="n">itemWindow</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">why don't you use the namespace more globally?</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;">xwindowtasksmodel.cpp:115</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">rulesConfig</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">KConfig</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"taskmanagerrulesrc"</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">configWatcher</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">KDirWatch</span><span class="p">(</span><span class="n">q</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">why not KSharedConfig::openConfig?</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;">xwindowtasksmodel.cpp:118</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">foreach</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">location</span><span class="p">,</span> <span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">standardLocations</span><span class="p">(</span><span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">ConfigLocation</span><span class="p">))</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">configWatcher</span><span style="color: #aa2211">-></span><span class="n">addFile</span><span class="p">(</span><span class="n">location</span> <span style="color: #aa2211">+</span> <span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"/taskmanagerrulesrc"</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">coding style nitpick: missing space between foreach and (</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;">xwindowtasksmodel.cpp:168</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #aa4000">void</span> <span class="p">(</span><span class="n">KWindowSystem</span><span style="color: #aa2211">::*</span><span class="n">myWindowChangeSignal</span><span class="p">)(</span><span class="n">WId</span> <span class="n">window</span><span class="p">,</span> <span style="color: #aa4000">const</span> <span style="color: #aa4000">unsigned</span> <span style="color: #aa4000">long</span> <span style="color: #aa2211">*</span><span class="n">dirty</span><span class="p">)</span> <span style="color: #aa2211">=</span> <span style="color: #aa2211">&</span><span class="n">KWindowSystem</span><span style="color: #aa2211">::</span><span class="n">windowChanged</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">QObject</span><span style="color: #aa2211">::</span><span class="n">connect</span><span class="p">(</span><span class="n">KWindowSystem</span><span style="color: #aa2211">::</span><span class="n">self</span><span class="p">(),</span> <span class="n">myWindowChangeSignal</span><span class="p">,</span> <span class="n">q</span><span class="p">,</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><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><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;">xwindowtasksmodel.cpp:198</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #74777d">// Add existing windows.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">foreach</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">WId</span> <span class="n">window</span><span class="p">,</span> <span class="n">KWindowSystem</span><span style="color: #aa2211">::</span><span class="n">windows</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">addWindow</span><span class="p">(</span><span class="n">window</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">coding style nitpick: missing whitespace</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;">xwindowtasksmodel.cpp:419</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #aa4000">return</span> <span class="n">info</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="p">}</span> <span style="color: #aa4000">else</span> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">windowInfoCache</span><span class="p">.</span><span class="n">value</span><span class="p">(</span><span class="n">window</span><span class="p">)</span><span style="color: #aa2211">-></span><span class="n">valid</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #aa4000">delete</span> <span class="n">windowInfoCache</span><span class="p">.</span><span class="n">take</span><span class="p">(</span><span class="n">window</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><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></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>