<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/103007/">http://git.reviewboard.kde.org/r/103007/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 4th, 2011, 11:29 a.m., <b>Aaron J. Seigo</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;">lots of coding style issues, though substance-wise it looks ok. a big concern i'm left with right now is that we end up with all of these patches in libtaskmanager and while it has more features, the overall quality of the code goes down due to the massive nature of the patches going in with very little testing combined with a decrease in readability due to less consistent coding style.
it also seems that in this patch we lose the ability to associate an executable with a launcher. that is a regression that needs addressing.
i also need to look at the config UI still, as well ...</pre>
</blockquote>
<p>On November 4th, 2011, 11:59 a.m., <b>Craig Drummond</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;">Sorry for the coding issues - I thought I had run 'astyle', but perhaps I am mistaken. I *promise* to run 'astyle' when all the changes are checked in, and go through the files by hand to check the spacing. I agree that consistent style is a good idea.
As to not associating an executable with the launcher, this *was* mentioned even before I submitted the first diff. But anyway, yes this needs to be fixed. In fact the issue is already resolved, I just have not updated the patch - I dont have access to my git checkout at the moment.
I dont agree with the "very little testing", these changes are used in IconTasks - and that has been out for about couple of months.</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;">the code added has been tested a fair amount, but nowhere near the testing that it will get now that it is in a mainline Plasma Workspaces release. your code is about to be exposed by orders (plural) of magnitude more people. but more importantly: it has not been tested in the form it is in git right now, which is merged with the upstread libtaskmanager. we're making changes to the patches and it will be used with more than just the icon tasks widget now.
so, yes, testing is currently an issue. not a fatal one, and one that will repidly be addressed once this is merged and the first betas go out ;) i just want us to be aware of this and ready to address problems that may arise that we're unaware of right now. nothing we (e.g. you and i) can't or won't handle, but awareness == readiness.</pre>
<br />
<p>- Aaron J.</p>
<br />
<p>On November 3rd, 2011, 9:21 p.m., Craig Drummond wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma.</div>
<div>By Craig Drummond.</div>
<p style="color: grey;"><i>Updated Nov. 3, 2011, 9:21 p.m.</i></p>
<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;">1. If fail to automatically find launcher, then prompt user to select from installed applications.
2. Add a config page, so that manualy set launchers may be adjusted.
(Part of IconTasks' taskmanager changes)</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>libs/taskmanager/CMakeLists.txt <span style="color: grey">(6ae36dc)</span></li>
<li>libs/taskmanager/groupmanager.h <span style="color: grey">(acaa142)</span></li>
<li>libs/taskmanager/groupmanager.cpp <span style="color: grey">(6e7ffa7)</span></li>
<li>libs/taskmanager/launcherconfig.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/taskmanager/launcherconfig.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/taskmanager/launcherconfig.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/taskmanager/launcherproperties.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/taskmanager/launcherproperties.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/taskmanager/launcherproperties.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/taskmanager/taskactions.cpp <span style="color: grey">(011c565)</span></li>
<li>libs/taskmanager/taskactions_p.h <span style="color: grey">(407b2c9)</span></li>
<li>libs/taskmanager/taskitem.h <span style="color: grey">(3c84678)</span></li>
<li>libs/taskmanager/taskitem.cpp <span style="color: grey">(e2c07d8)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103007/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>