<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/107628/">http://git.reviewboard.kde.org/r/107628/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">A few smaller things in the code that I don't quite like. But more importantly:
This still depends on manual eliding of the path (i.e. your truncatePath function). I think the truncation should be dropped.
For the size of the view you could use QAbstractItemView::sizeHintForRow and sizeHintForColumn so the view gets its preferred size. I've actually experimented a bit with that, this is my current setViewGeometry function (as you can see its not depending on a contentWidth calculated elsewhere):
const QSize centralSize = window->centralWidget()->size();
// Maximum size of the view is 3/4th of the central widget (the editor area) so the view does not overlap the
// mainwindow since that looks awkward.
const QSize viewMaxSize( centralSize.width() * 3/4, centralSize.height() * 3/4 );
// The actual view size should be as big as the columns/rows need it, but smaller than the max-size. This means
// the view will get quite high with many open files but I think thats ok. Otherwise one can easily tweak the
// max size to be only 1/2th of the central widget size
const QSize viewSize( std::min( view->sizeHintForColumn(0) + view->verticalScrollBar()->width(), viewMaxSize.width() ),
std::min( view->sizeHintForRow(0) * view->model()->rowCount(), viewMaxSize.height() ) );
// Position should be central over the editor area, so map to global from parent of central widget since
// the view is positioned in global coords
QPoint centralWidgetPos = window->mapToGlobal( window->centralWidget()->pos() );
const int xPos = std::max(0, centralWidgetPos.x() + (centralSize.width() - viewSize.width() ) / 2);
const int yPos = std::max(0, centralWidgetPos.y() + (centralSize.height() - viewSize.height() ) / 2);
view->setFixedSize(viewSize);
view->move(xPos, yPos);
In addition I added these three lines to the constructor:
view->setUniformItemSizes( true );
view->setTextElideMode( Qt::ElideMiddle );
view->setHorizontalScrollBarPolicy( Qt::ScrollBarAlwaysOff );
So the horizontal scrollbar is never displayed and the eliding happens in the middle of the items keeping the last directory part intact. The uniform item sizes is merely for optimization of rendering and layout calculating.
Can you try this out please? What do you think about the behaviour?
As you can see the code bases the maximum size on the central widget. I've tried out your initial patch with a very small mainwindow and looked quite awkward to have the popup be larger than the mainwindow. And having the popup visually inside the bounds of the central widget (the editor area) strengthens the connection it has to this area of the mainwindow.
I'm not 100% satisfied with this, with very long paths it can happen that the project name is cut out. A table or treeview might help with that, i.e. make the first column with filename + project name always resize to its contents and the last one resize to fit the view. Then set a fixed size on the view and let Qt figure out the details. It won't look like a single item anymore, but I can live with that if we get some more control over the rendering. Another option would be to actually do this stuff in a delegate, so the delegate would be fetching/calculating the filename, project name and path and ensure that only the path is being elided by qt when painting the whole thing. Thats quite a bit more complicated to implement though I think.</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://git.reviewboard.kde.org/r/107628/diff/2/?file=97530#file97530line122" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentswitcher/documentswitcherplugin.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">DocumentSwitcherPlugin::DocumentSwitcherPlugin(QObject *parent, const QVariantList &/*args*/)</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">122</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="kt">int</span> <span class="n">minViewWidth</span> <span class="o">=</span> <span class="n">std</span><span class="o">::</span><span class="n">min</span><span class="p">(</span><span class="n">contentWidth</span> <span class="o">+</span> <span class="mi">50</span><span class="cm">/*scrollBarWidth*/</span><span class="p">,</span> <span class="n">currentScreenGeometry</span><span class="p">.</span><span class="n">width</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 width of the scrollbar should be possible to find out, no need to hardcode it.</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://git.reviewboard.kde.org/r/107628/diff/2/?file=97530#file97530line165" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentswitcher/documentswitcherplugin.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">void DocumentSwitcherPlugin::walkBackward() { walk(model->rowCount()-1, 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">157</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">void</span> <span class="n">DocumentSwitcherPlugin</span><span class="o">::</span><span class="n">walkForward</span><span class="p">()</span> <span class="p">{</span> <span class="n">walk</span><span class="p">(</span><span class="mi">0</span><span class="p">,</span> <span class="n">model</span><span class="o">-></span><span class="n">rowCount</span><span class="p">()</span><span class="o">-</span><span class="mi">1</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;">I think the change that merges walkForward/Backward should be done first as a separate commit. And then on top of that adjust the presentation since it touches the walk function. That way you're not mixing refactoring with actual logic changes and people can later more easily follow why certain changes where done.</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://git.reviewboard.kde.org/r/107628/diff/2/?file=97530#file97530line173" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentswitcher/documentswitcherplugin.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">void DocumentSwitcherPlugin::walkBackward() { walk(model->rowCount()-1, 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">165</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="n">QFontMetrics</span> <span class="n">fm</span> <span class="o">=</span> <span class="n">view</span><span class="o">-></span><span class="n">fontMetrics</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 not quite the right fontmetrics, in qt's itemview the delegate has the ultimate control about rendering and also decides which font is being used. But that information is not available.</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://git.reviewboard.kde.org/r/107628/diff/2/?file=97530#file97530line176" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentswitcher/documentswitcherplugin.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">void DocumentSwitcherPlugin::walkBackward() { walk(model->rowCount()-1, 0); }</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">126</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">txt</span> <span class="o">=</span> <span class="n">v</span><span class="o">-></span><span class="n">document</span><span class="p">()</span><span class="o">-></span><span class="n">title</span><span class="p">();</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">168</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">using</span> <span class="k">namespace</span> <span class="n">KDevelop</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;">Please move the using to the top of the file or remove it completely and do a full qualification of the classes.</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://git.reviewboard.kde.org/r/107628/diff/2/?file=97530#file97530line177" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentswitcher/documentswitcherplugin.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">void DocumentSwitcherPlugin::walkBackward() { walk(model->rowCount()-1, 0); }</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">127</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KDevelop</span><span class="o">::</span><span class="n">IDocument</span><span class="o">*</span> <span class="n">doc</span> <span class="o">=</span> <span class="k">dynamic_cast</span><span class="o"><</span><span class="n">KDevelop</span><span class="o">::</span><span class="n">IDocument</span><span class="o">*></span><span class="p">(</span> <span class="n">v</span><span class="o">-></span><span class="n">document</span><span class="p">()</span> <span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">169</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Sublime</span><span class="o">::</span><span class="n">Document</span> <span class="k">const</span><span class="o">*</span> <span class="k">const</span> <span class="n">slDoc</span> <span class="o">=</span> <span class="n">v</span><span class="o">-></span><span class="n">document</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;">We usually use const T * const for const pointers to const types and not const * const.</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://git.reviewboard.kde.org/r/107628/diff/2/?file=97530#file97530line178" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentswitcher/documentswitcherplugin.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">void DocumentSwitcherPlugin::walkBackward() { walk(model->rowCount()-1, 0); }</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">128</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="hl"> </span><span class="n"><span class="hl">doc</span></span><span class="hl"> </span><span class="p">)</span> <span class="p">{</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">170</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="o"><span class="hl">!</span></span><span class="n"><span class="hl">slDoc</span></span><span class="p">)</span> <span class="p">{</span><span class="hl"> </span><span class="k"><span class="hl">continue</span></span><span class="p"><span class="hl">;</span></span><span class="hl"> </span><span class="p"><span class="hl">}</span></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 should be either a multi-line if or without the braces. I'd prefer the former but don't have a strong opinion.</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://git.reviewboard.kde.org/r/107628/diff/2/?file=97530#file97530line184" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentswitcher/documentswitcherplugin.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">void DocumentSwitcherPlugin::walkBackward() { walk(model->rowCount()-1, 0); }</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">134</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// Relative path means we've got a project in front, so remove the 'inner' parts of the path to make it short</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">176</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="kt">bool</span> <span class="n">isPartOfOpenProject</span> <span class="o">=</span> <span class="n">QDir</span><span class="o">::</span><span class="n">isRelativePath</span><span class="p">(</span><span class="n">path</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;">Same thing here, please keep the comments</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://git.reviewboard.kde.org/r/107628/diff/2/?file=97530#file97530line185" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentswitcher/documentswitcherplugin.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">void DocumentSwitcherPlugin::walkBackward() { walk(model->rowCount()-1, 0); }</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">135</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// and display in a useful way</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">177</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="n">path</span><span class="p">.</span><span class="n">endsWith</span><span class="p">(</span><span class="sc">'/'</span><span class="p">))</span> <span class="p">{</span> <span class="n">path</span><span class="p">.</span><span class="n">remove</span><span class="p">(</span><span class="n">path</span><span class="p">.</span><span class="n">length</span><span class="p">()</span> <span class="o">-</span> <span class="mi">1</span><span class="p">,</span> <span class="mi">1</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;">Please leave the comments intact when you just re-arrange code but don't change it.
Also please keep multi-line ifs, we're not trying to obfuscate our code ;)</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://git.reviewboard.kde.org/r/107628/diff/2/?file=97530#file97530line190" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentswitcher/documentswitcherplugin.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">void DocumentSwitcherPlugin::walkBackward() { walk(model->rowCount()-1, 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">182</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="n">QPair</span><span class="o"><</span><span class="n">QString</span><span class="p">,</span> <span class="n">QString</span><span class="o">></span> <span class="n">fileInProjectInfo</span> <span class="o">=</span> <span class="p">(</span><span class="n">projectNameSize</span> <span class="o"><</span> <span class="mi">0</span><span class="p">)</span> <span class="o">?</span> <span class="c1">// whole path is project name?</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;">Hmm, why not use 2 variables instead of the pair? I don't see any benefit of using a pair here since its deleted again a few lines forward and makes it less clear what the two parts are actually (hence your comment about first and second)</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://git.reviewboard.kde.org/r/107628/diff/2/?file=97530#file97530line191" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentswitcher/documentswitcherplugin.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">void DocumentSwitcherPlugin::walkBackward() { walk(model->rowCount()-1, 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">183</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qMakePair</span><span class="p">(</span><span class="n">path</span><span class="p">,</span> <span class="n">QString</span><span class="p">(</span><span class="s">"/"</span><span class="p">))</span> <span class="o">:</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 ternary operator is quite unreadable IMHO. I'd write it as:
fileInProjectInfo = projectNameSize < 0 ? qMakePair(path, QString("/"))
: qMakePair(...)
i.e. keep the if-expression on the same line and align ? and :.
I'd also either leave out the comment on this line or add it to the one before the code.</pre>
</div>
<br />
<p>- Andreas</p>
<br />
<p>On January 7th, 2013, 8:06 p.m., Jarosław Sierant 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 KDevelop.</div>
<div>By Jarosław Sierant.</div>
<p style="color: grey;"><i>Updated Jan. 7, 2013, 8:06 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;">Document switcher window width is automatically calculated base on content (auto adjust to longest item). Current screen width is upper bound of the window width.
Code duplication has been removed (methods walkForward/walkBackward).
A path to the file on items list can be truncated if it is too long (threshold=80).</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;">Only manual tests are done.</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>plugins/documentswitcher/documentswitcherplugin.h <span style="color: grey">(0924e81)</span></li>
<li>plugins/documentswitcher/documentswitcherplugin.cpp <span style="color: grey">(d457a5a)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107628/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://git.reviewboard.kde.org/r/107628/s/990/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2013/01/07/doc_switcher_QTableView_400x100.png" style="border: 1px black solid;" alt="Document Switcher window based on QTableView" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>