<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/101045/">http://git.reviewboard.kde.org/r/101045/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 7th, 2011, 12:29 p.m., <b>Casper Boemann</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;">Hi it looks nicely coded. Just a few small issues listed below.
I've not actually tried to build aand use it
One question though: Don't KDE have some find toolbar already we could have resued. Okay it keeps us free of relying on KDE but on the other hand this is ui.
As far as I'm concerned it can go in, but I'm not pressing ship it since i'v not actually tried it.</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;">There are actually at least two implementations of the find bar in KDE: There's the one used in Kate, and the one in Konqueror. Kate's seems to be embedded deeply within the KatePart code. Konqueror's is embedded deeply within KHTML code, so both do not seem to be too useable from outside of those libraries.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 7th, 2011, 12:29 p.m., <b>Casper Boemann</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/101045/diff/1/?file=13894#file13894line75" style="color: black; font-weight: bold; text-decoration: underline;">libs/main/KoFindToolbar.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="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; ">KoFindToolbar::KoFindToolbar(KoFindBase* finder, KActionCollection *ac, QWidget* parent, Qt::WindowFlags f)</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">75</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QHBoxLayout</span> <span class="o">*</span><span class="n">layout</span> <span class="o">=</span> <span class="k">new</span> <span class="n">QHBoxLayout</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;">What, no ui file but hardcoding ??
oh well.
</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 did not really consider it important to use a UI file for what essentially amounts to some widgets next to each other. I can change it to make use of one, though.</pre>
<br />
<p>- Arjen</p>
<br />
<p>On April 7th, 2011, 12:11 p.m., Arjen Hiemstra 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 Calligra, Marijn Kruisselbrink, Boudewijn Rempt, Thorsten Zachmann, and Casper Boemann.</div>
<div>By Arjen Hiemstra.</div>
<p style="color: grey;"><i>Updated April 7, 2011, 12:11 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;">This is the diff of the branch libs-kofind_refactor-ahiemstra and master. It encompasses the entire refactor of KoFind to a more generic structure that can be used by different interfaces and by different document types.
I consider this to be done API-wise, though several features are currently missing. This review therefore is mostly a request for comments about the current API.
Still to be done feature-wise:
- Add support for more options (search in selection, search from cursor, regex, etc)
- Properly implement replacing, including implementing an interface for it.
- Add support for searching through multiple text shapes to KoFindText.
- Implement a searching backend for tables.</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;">I have implemented a toolbar interface for Words, which is enabled in this review. It works very well, though it does not yet feature all the options the current dialog has. Furthermore, I have implemented unit tests for KoFindMatch, the most important class in this code, though arguably at least KoFindOption/KoFindOptionSet also could do with some unit testing.</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/kotext/KoTextDocument.h <span style="color: grey">(6d755f8)</span></li>
<li>libs/kotext/KoTextDocument.cpp <span style="color: grey">(1517953)</span></li>
<li>libs/main/CMakeLists.txt <span style="color: grey">(a7f3077)</span></li>
<li>libs/main/KoFindBase.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/KoFindBase.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/KoFindMatch.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/KoFindMatch.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/KoFindOption.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/KoFindOption.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/KoFindOptionSet.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/KoFindOptionSet.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/KoFindText.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/KoFindText.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/KoFindToolbar.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/KoFindToolbar.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/tests/CMakeLists.txt <span style="color: grey">(6f13036)</span></li>
<li>libs/main/tests/testfindmatch.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>libs/main/tests/testfindmatch.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/textshape/TextShape.cpp <span style="color: grey">(1c8f2c5)</span></li>
<li>words/part/KWView.h <span style="color: grey">(a8eec16)</span></li>
<li>words/part/KWView.cpp <span style="color: grey">(539e6b1)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/101045/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>