<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/5794/">http://svn.reviewboard.kde.org/r/5794/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks good, just a few things I noticed, but you can commit after fixing them.</pre>
<br />
<div>
<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/5794/diff/5/?file=40894#file40894line40" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistdelegate_p.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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; ">KAboutApplicationPersonListDelegate::KAboutApplicationPersonListDelegate(</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">40</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">,</span> <span class="n">AVATAR_HEIGHT</span><span class="p">(</span> <span class="mi">50</span> <span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">These constants should just be static const int .. = ...; at the top of the file.</pre>
</div>
<br />
<div>
<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/5794/diff/5/?file=40894#file40894line45" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistdelegate_p.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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; ">KAboutApplicationPersonListDelegate::KAboutApplicationPersonListDelegate(</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">45</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">,</span> <span class="n">m_widgetSizeHint</span><span class="p">(</span> <span class="k">new</span> <span class="n">QSize</span><span class="p">()</span> <span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why is this a pointer to a QSize, rather than just a QSize member?
In fact this member (m_widgetSizeHint) doesn't seem to be used anywhere, can be removed.</pre>
</div>
<br />
<div>
<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/5794/diff/5/?file=40894#file40894line224" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistdelegate_p.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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; ">KAboutApplicationPersonListDelegate::KAboutApplicationPersonListDelegate(</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">224</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">painter</span><span class="o">-></span><span class="n">setPen</span><span class="p">(</span><span class="n">QPen</span><span class="p">(</span><span class="n">option</span><span class="p">.</span><span class="n">palette</span><span class="p">.</span><span class="n">text</span><span class="p">().</span><span class="n">color</span><span class="p">()));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why the setPen, if this method only uses drawPixmap?
This means the save + the restore can be removed too.</pre>
</div>
<br />
<div>
<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/5794/diff/5/?file=40894#file40894line254" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistdelegate_p.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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; ">KAboutApplicationPersonListDelegate::KAboutApplicationPersonListDelegate(</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">254</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QDesktopServices</span><span class="o">::</span><span class="n">openUrl</span><span class="p">(</span> <span class="n">url</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">KToolInvocation::invokeBrowser</pre>
</div>
<br />
<div>
<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/5794/diff/5/?file=40897#file40897line96" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.h</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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; ">static const QStringList KAboutApplicationPersonProfileOcsLinkAtticaTypes = ( QStringList()</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">96</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="k">const</span> <span class="n">QList</span><span class="o"><</span> <span class="n">QIcon</span> <span class="o">></span> <span class="n">KAboutApplicationPersonProfileOcsLinkIcons</span> <span class="o">=</span> <span class="p">(</span> <span class="n">QList</span><span class="o"><</span> <span class="n">QIcon</span> <span class="o">></span><span class="p">()</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is going to create all these KIcons at startup of any program that uses kdeui. Fortunately this doesn't actually load the icons, but that's still quite some "new" calls on app startup that we can do without.
My suggestion: static const char s_profileOcsLinkIcons[][16] = { { "..." }, { "..." } ... }
This makes the data "really const", which means the .rodata section in the executable, which means shared between all processes.
Same thing for the list of qstringlist above, too.</pre>
</div>
<br />
<div>
<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/5794/diff/5/?file=40897#file40897line158" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.h</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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; ">static const QStringList KAboutApplicationPersonProfileOcsLinkAtticaTypes = ( QStringList()</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">158</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="o">:</span> <span class="n">m_name</span><span class="p">(</span> <span class="n">QString</span><span class="p">()</span> <span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The QString() is not useful, that's the default value anyway. So m_name() would be enough (or nothing, but initializing members is good practice).</pre>
</div>
<br />
<div>
<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/5794/diff/5/?file=40898#file40898line24" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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">24</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#include <kdecore/io/kdebug.h></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The kdecore/subdir/ in the include surprises me, we don't do that anywhere else. I recommend not doing it, since people copying that in header files will break stuff outside kdelibs.</pre>
</div>
<br />
<div>
<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/5794/diff/5/?file=40898#file40898line140" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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">140</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">for</span><span class="p">(</span> <span class="kt">int</span> <span class="n">i</span> <span class="o">=</span> <span class="mi">2</span><span class="p">;</span> <span class="n">i</span> <span class="o"><=</span> <span class="mi">10</span><span class="p">;</span> <span class="o">++</span><span class="n">i</span> <span class="p">)</span> <span class="p">{</span> <span class="c1">//OCS supports 10 total homepages as of 2/oct/2009</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why does this start at 2?</pre>
</div>
<br />
<div>
<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/5794/diff/5/?file=40898#file40898line222" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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">222</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QByteArray</span> <span class="n">data</span> <span class="o">=</span> <span class="n">QByteArray</span><span class="p">(</span> <span class="n">reply</span><span class="o">-></span><span class="n">readAll</span><span class="p">()</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">the QByteArray() ctor call is not needed, readAll returns a QByteArray already.</pre>
</div>
<br />
<div>
<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/5794/diff/5/?file=40898#file40898line223" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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">223</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QPixmap</span> <span class="n">pixmap</span> <span class="o">=</span> <span class="n">QPixmap</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The = QPixmap() is not needed, "QPixmap pixmap;" would already create an empty pixmap :)</pre>
</div>
<br />
<div>
<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/5794/diff/5/?file=40898#file40898line235" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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">235</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">emit</span> <span class="n">layoutChanged</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why the emit layoutChanged()? When a cell changes, dataChanged() is enough.</pre>
</div>
<br />
<div>
<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/5794/diff/5/?file=40898#file40898line250" style="color: black; font-weight: bold; text-decoration: underline;">trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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">250</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">//This can't be static like the other lists because of i18n</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">(well technically it could be, using I18N_NOOP).
But anyway, a better solution then would be to use a switch, rather than creating the list every time just to extract one element. But I guess I'm mostly nitpicking, performance is definitely not an issue here.
For maintainance reasons, all these lists should really be merged into a single list of structs, in fact. It would then be much easier to add or remove entries. I can do that change after you commit, if you don't see what I mean.</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On November 9th, 2010, 9:22 a.m., Teo Mrnjavac 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.</div>
<div>By Teo Mrnjavac.</div>
<p style="color: grey;"><i>Updated 2010-11-09 09:22:50</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;">The present review request contains the implementation of a Social About Dialog feature for KDE apps, and is a rewrite of Amarok's Social About Dialog (See http://teom.wordpress.com/2009/08/23/social-desktop-integration-in-kaboutdialog/ ). For those who don't have time to read through that link, the Social About Dialog is an evolution of our old KAboutDialog which adds optional Open Collaboration Services features to complete the contributors' data.
The only changes that concern the API are in kaboutdata.{h,cpp}, and I've tried to keep it binary compatible to the best of my abilities by following the instructions on our Techbase, I hope I did it right.
The only new dependency is the Attica library, except if we are building with KDE_PLATFORM_FEATURE_BINARY_COMPATIBLE_FEATURE_REDUCTION, in that case all social features are #ifdef'ed out and the dialog behaves very much like the old KAboutDialog.
The dialog itself uses model/view/delegate, not very different from KNewStuff3, and most of the action happens in the model (jobs to get data from Attica/OCS) and in the delegate.
As we're getting dangerously close to the string freeze, I've decided to put this up for review even though there are still a couple TODOs in the code. These are:
* The icons are placeholders, we are going to need some real icons for links and social networks. We already have some of those for Amarok's about dialog but this needs to be cleanly solved in a way that would not make us liable for IP infringement in any way. IANAL here so a discussion as to how to proceed is welcome.
* In the optional translators tab there's a short but not very short text about the translation team. I've added it as a label but it tends to take a bit more room than I'd like, so I'm requesting for comments on what to do with it. Also, I don't know how translators' names are added. If translators can't add their OCS usernames anyway then I'll easily revert to the old about view for them and the problem of the tall translation team text is automatically solved.
* Since kdeui does not depend on KIO, I couldn't use KIO::get and had to use QNAM instead. For the same reason I had to use QDesktopServices instead of KRun::runUrl() to launch URLs. As it is now, stuff works, but to make it nicer the plan could be to add an Attica::PixmapFromUrlJob (which I've been told would satisfy other unrelated needs too) to Attica to remove the single instance where QNAM is used instead of KIO.
Except maybe for the translators tab issues, none of the TODOs require string changes.
To test the feature you only need to compile KDElibs with the patch and run any KDE app which has an about dialog, however you won't be able to try the social features unless you also add OCS usernames (by default the provider is openDesktop.org) to one or more contributors in the KAboutData of a KDE app of your choice.
For example: aboutData.addAuthor( ki18n("Random Dude") , ki18n("Random stuff"), "randomdude@kde.org", "http://example.com", "random-dude's-oD.o-username");
</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;">Testing during development, both with Attica enabled and disabled.</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/kdecore/kernel/kaboutdata.h <span style="color: grey">(1194020)</span></li>
<li>trunk/KDE/kdelibs/kdecore/kernel/kaboutdata.cpp <span style="color: grey">(1194020)</span></li>
<li>trunk/KDE/kdelibs/kdeui/CMakeLists.txt <span style="color: grey">(1194020)</span></li>
<li>trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationconfigattica_p.h.cmake <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationdialog.h <span style="color: grey">(1194020)</span></li>
<li>trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationdialog.cpp <span style="color: grey">(1194020)</span></li>
<li>trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistdelegate_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistdelegate_p.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistview_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistview_p.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>trunk/KDE/kdelibs/kdeui/dialogs/thumb_frame.png <span style="color: grey">(UNKNOWN)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5794/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://svn.reviewboard.kde.org/r/5794/s/552/"><img src="http://svn.reviewboard.kde.org/media/uploaded/images/2010/11/08/snapshot2_1_400x100.png" style="border: 1px black solid;" alt="The social about dialog as it appears when OCS usernames for openDesktop.org are provided in addAuthor()" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>