<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://svn.reviewboard.kde.org/r/5671/">http://svn.reviewboard.kde.org/r/5671/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 21st, 2010, 11:19 p.m., <b>Christoph Feck</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 Sebastian, lots of good code to review :)
It would help for the API review, if you could mark private headers with the *_p.h convention.
For example, I was to suggest to make the SearchLineEdit be a simple QWidget with an QLineEdit accessor (similar to QComboBox), so that enhancements can be made (such as using a KComboBox internally later). But it looks like this class isn't exported, so this suggestions is void.
What I didn't understand from looking at the comments is what a "selection" in Facet terms means. It doesn't mean selecting the query results, does it?
If there is some general overview document somewhere, it would help understanding what the API wants to offer, and that again would help the review process.</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;">Well, your comment is still valid since SearchLineEdit is a candidate for public API as soon as I find a better name and the rest is settled. Thus, I will take it into account since it makes perfect sense to me.
I will try to improve the API docs to better reflect the idea of a selection.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 21st, 2010, 11:19 p.m., <b>Christoph Feck</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://svn.reviewboard.kde.org/r/5671/diff/1/?file=39799#file39799line91" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/nepomuk/utils/datefacet.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; ">namespace Nepomuk {</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">91</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;">Private* const d;</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;">right, fixed.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 21st, 2010, 11:19 p.m., <b>Christoph Feck</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://svn.reviewboard.kde.org/r/5671/diff/1/?file=39800#file39800line211" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/nepomuk/utils/datefacet.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; ">DateRange Nepomuk::Utils::DateFacet::Private::getCustomRange() const</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">211</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">menu</span><span class="p">.</span><span class="n">exec</span><span class="p">(</span><span class="n">QCursor</span><span class="o">::</span><span class="n">pos</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;">Hm, having reviewed bug reports in KDE for nearly a year now, I can only warn about code that uses local event loops.
In other words, if there is no way around it, you should warn the user of the class in the documentation for that function.</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;">You are right. This is a problem indeed. I have been struggling with this but could not come up with a smooth solution. Thus, it might be best to just add a hint to the API docs.</pre>
<br />
<p>- Sebastian</p>
<br />
<p>On October 20th, 2010, 4:07 p.m., Sebastian Trueg wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/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 kdelibs and Vishesh Handa.</div>
<div>By Sebastian Trueg.</div>
<p style="color: grey;"><i>Updated 2010-10-20 16:07:36</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 have been struggling with creating a good facet API for a long time now. I finally reached a point where I am happy with the result. IMHO this is essential enough to get into kdelibs/nepomuk. The first usage will obviously be Dolphin, closely followed by the file dialog. All in all this will make it so much simpler to provide Nepomuk powered search capabilities in applications.
Anyway, this review request is intended for an API review before I commit.</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>trunk/KDE/kdelibs/includes/CMakeLists.txt <span style="color: grey">(1187872)</span></li>
<li>trunk/KDE/kdelibs/includes/Nepomuk/Utils/DynamicResourceFacet <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/includes/Nepomuk/Utils/Facet <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/includes/Nepomuk/Utils/FacetWidget <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/includes/Nepomuk/Utils/ResourceModel <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/includes/Nepomuk/Utils/SearchWidget <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/includes/Nepomuk/Utils/SimpleFacet <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/includes/Nepomuk/Utils/SimpleResourceModel <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/CMakeLists.txt <span style="color: grey">(1187872)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/datefacet.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/datefacet.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/daterange.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/daterange.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/daterangeselectionwidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/daterangeselectionwidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/daterangeselectionwidget.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/dynamicresourcefacet.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/dynamicresourcefacet.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/facet.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/facet.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/facetdelegate.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/facetdelegate.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/facetfiltermodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/facetfiltermodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/facetmodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/facetmodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/facetwidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/facetwidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/resourcemodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/resourcemodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/searchlineedit.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/searchlineedit.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/searchwidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/searchwidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/searchwidget_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/simplefacet.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/simplefacet.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/simpleresourcemodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/nepomuk/utils/simpleresourcemodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5671/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>