<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/128417/">https://git.reviewboard.kde.org/r/128417/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 10th, 2016, 6:42 a.m. UTC, <b>Eike Hein</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">New features to libtaskmanager should be implemented on Wayland first. Otherwise they increase the porting effort, which I'm not willing to do (i.e. I'm not going to take any drive-by feature contributions that leave me to figuring out how to make it work on Wayland later).</p></pre>
</blockquote>
<p>On July 10th, 2016, 11:48 a.m. UTC, <b>Xuân Baldauf</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The feature of the current patch works both under X11 and under Wayland (tested with kwin5-5.7.0 based on wayland-1.11.0). No code change (e.g. in file "waylandtasksmodel.cpp") was necessary on the Wayland side to achieve the desired behavior.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is that guaranteed in the spec, or does it work by accident? "By accident" isn't good enough - it needs to be a spec guarantee or explicit code. If it's as important as you say it is, it needs to be documented and testable behavior</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The diff also contains references to a loftier spec that creates a porting pressure vector. That needs fleshing out or removal.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And there is a UX problem: TasksModel starts out in SortAlpha and then is switched to SortManual by a call to setSortMode. Switching from a different sort mode to SortManual is supposed not to reorder tasks, but seed the map from the the existing sorting, so the user can opt into manual from the applet config dialog and then move tasks from their current location. This patch will make this experience worse by reordering window tasks by creation order. I care more about that case than plasmashell crashing, since at that point things have gone wrong anyway, and crashes are to be fixed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Further, it doesn't sort in startup tasks correctly, making things even more arbitrary, even though it's available. TasksModel is supposed to abstract over all task types correctly; doing major business logic in a source model is conceptually wrong. Sorting is also done in TasksModel, not the intentionally simple, data-providing source models. A better appoach to this would be a CreationTimestamp data role in the source models used to seed the sort map - though that by itself isn't enough to address the above UX problem.</p></pre>
<br />
<p>- Eike</p>
<br />
<p>On July 10th, 2016, 12:15 a.m. UTC, Xuân Baldauf wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma.</div>
<div>By Xuân Baldauf.</div>
<p style="color: grey;"><i>Updated July 10, 2016, 12:15 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is the first step towards not loosing the sort order of the task buttons in the task bar, even when plasmashell crashes and the user has a specific manual sort order. For now, we use the sort order imposed by the creation time (so the user can affect the sort order simply by ordering the creation of the windows).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Once this patch is successful, we can expand sort order storage by storing the manual sort order offset in the X window properties each time that offset changes. This will make plasmashell crashes less damaging to a user who prefers a certain task button sort order (for the duration of the X session).</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Testing was performed under openSUSE 42.1 against KDE Plasma 5.7</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>libtaskmanager/CMakeLists.txt <span style="color: grey">(9c3e15e)</span></li>
<li>libtaskmanager/tasksmodel.cpp <span style="color: grey">(bf37042)</span></li>
<li>libtaskmanager/xwindowtasksmodel.h <span style="color: grey">(d24a7d5)</span></li>
<li>libtaskmanager/xwindowtasksmodel.cpp <span style="color: grey">(c1e9495)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128417/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>