<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/104803/">http://git.reviewboard.kde.org/r/104803/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 1st, 2012, 2:39 p.m., <b>David Edmundson</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;">I'm very impressed! Especially with documentation and testing, this isn't the Lasath I know!
Few comments, though generally I'm happy with it.
However, do _NOT_ merge this into master until 0.4 is branched. I will talk to mck182 about this.
(so a suspended ship-it! from me)</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;">My lecturer has got it into our heads (via his favourite method of teaching - pain and suffering) that when crunched for time, you can always scale back features, but crappy code is still crappy code. So you're better of having less features well written and tested than having a whole bunch of haxx that's hard to maintain and/or are broken.
Thus, the tests and documentation.
And yeah, this wasn't meant to be merged to master - it's already on my scratch repo but quickgit.kde.org hadn't realized it existed at the time.
http://quickgit.kde.org/index.php?p=clones%2Fktp-text-ui%2Ffernando%2Fgsoc.git&a=shortlog&h=refs/heads/first_attempt</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 1st, 2012, 2:39 p.m., <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/104803/diff/1/?file=59836#file59836line35" style="color: black; font-weight: bold; text-decoration: underline;">filters/emoticons/emoticon-filter.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">private:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">35</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Private</span> <span class="o">*</span><span class="n">d</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You don't /need/ to use d-pointers in the filters.
If you prefer to use them, go ahead - but you don't need to maintain binary compatibility as it's not a library people will link against.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It's actually a habit now - so I'm probably going to keep them there or else it'll feel weird to me :S</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 1st, 2012, 2:39 p.m., <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/104803/diff/1/?file=59850#file59850line1" style="color: black; font-weight: bold; text-decoration: underline;">lib/ktptxtui_message_filter.desktop</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">[Desktop Entry]</pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What is this file for?
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I assumed I needed this to let KDE know that 'KTpTextUi/MessageFilter' is a valid ServiceType.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 1st, 2012, 2:39 p.m., <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/104803/diff/1/?file=59852#file59852line95" style="color: black; font-weight: bold; text-decoration: underline;">lib/message-processor.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">Message MessageProcessor::processOutgoingMessage(const Tp::Message &sentMessage)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">93</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_filters</span><span class="p">.</span><span class="n">append</span><span class="p">(</span><span class="n">filter</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is just in any random order.
Needs fixing long term. If you write code you know needs fixing add a "//FIXME" or "//TODO" so it's not forgotten.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Oh yeah, I was actually going to ask you during our weekly meeting if I can have my own component in bugzilla. That way I could use it to keep track of everything that needs to be done. (And eventually for bugs after it's been released)</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 1st, 2012, 2:39 p.m., <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/104803/diff/1/?file=59853#file59853line37" style="color: black; font-weight: bold; text-decoration: underline;">lib/message.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">37</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * All methods in this class are thread safe, though some may cause an</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Don't say it's thread safe unless it actually is.
Which I'm not sure this is.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It //will// be :P
As soon as I get around to building the async version (and therefore accept that there is such a thing as concurrency in this world).</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 1st, 2012, 2:39 p.m., <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/104803/diff/1/?file=59856#file59856line31" style="color: black; font-weight: bold; text-decoration: underline;">tests/message-processor-basic-tests.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">private:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">31</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">SyncProcessor</span> <span class="n">s</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">try and call member variables m_something.
it will be make it clearer.
also don't write "this->" everywhere.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Okay, it's now called actualProcessor, but it's in a d->pointer.
I have to have one or the other now, or I feel my head will explode.
PS: I blame Java</pre>
<br />
<p>- Lasath</p>
<br />
<p>On May 1st, 2012, 8:43 a.m., Lasath Fernando 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 Telepathy and David Edmundson.</div>
<div>By Lasath Fernando.</div>
<p style="color: grey;"><i>Updated May 1, 2012, 8:43 a.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;">I'm starting do make some headway on my project (despite being swamped by Uni work at the moment), so I thought I may as well review what I've done so far.
Firstly, I gave MessageProcessor a KPluginLoader. I then cleaned it up, removed unneeded #includes, gave AbstractMessageFilter a camel case header etc.
Before I started porting the three existing filters to be KPlugins, I decided I should write unit tests for them. I'm not really sure on what the conventions are for tests because, well I haven't seen any on this project. So I made a few QTests and stuck them in a directory called tests.
I've ported EscapeFilter and EmoticonFilter. I just wrote tests for UrlFilter, but haven't got to port it yet.
I also started documenting my work, in the hope that it'll make things easier to maintain. Currently, the Message class is more or less documented.
If at all possible, I'd like someone who isn't familiar with how these work internally to read it and tell me if that documentation is clear.</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;">THERE ARE UNIT TESTS!! :D</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>CMakeLists.txt <span style="color: grey">(e5dc102)</span></li>
<li>filters/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/emoticons/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/emoticons/emoticon-filter.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/emoticons/emoticon-filter.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/emoticons/ktptextui_message_filter_emoticons.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/escape/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/escape/escape-filter.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/escape/escape-filter.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>filters/escape/ktptextui_message_filter_escape.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>lib/CMakeLists.txt <span style="color: grey">(e94a432)</span></li>
<li>lib/KTp/AbstractMessageFilter <span style="color: grey">(PRE-CREATION)</span></li>
<li>lib/abstract-message-filter.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>lib/abstract-message-filter.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>lib/emoticon-filter.cpp <span style="color: grey">(0e37aab)</span></li>
<li>lib/escape-filter.cpp <span style="color: grey">(c366410)</span></li>
<li>lib/filters.h <span style="color: grey">(6059ea2)</span></li>
<li>lib/ktptxtui_message_filter.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>lib/message-processor.h <span style="color: grey">(d6228b5)</span></li>
<li>lib/message-processor.cpp <span style="color: grey">(a9b409e)</span></li>
<li>lib/message.h <span style="color: grey">(c9d4340)</span></li>
<li>lib/message.cpp <span style="color: grey">(ae947d2)</span></li>
<li>tests/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/message-processor-basic-tests.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/message-processor-basic-tests.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/sync-processor.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/sync-processor.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104803/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>