<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/109986/">http://git.reviewboard.kde.org/r/109986/</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 13th, 2013, 10:47 a.m. UTC, <b>Andreas Pakulat</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/109986/diff/1/?file=138480#file138480line41" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentview/kdevdocumentviewdelegate.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; ">KDevDocumentViewDelegate::~KDevDocumentViewDelegate()</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">41</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">bool</span> <span class="n">useExpandIndicator</span> <span class="o">=</span> <span class="nb">true</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;">Whats the reason for this?</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've been trying folder icons instead of the expand indicator there, it works OK, but looks a bit more crowded. I'll remove this. (Also will save a KIcon construction elsewhere).</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 13th, 2013, 10:47 a.m. UTC, <b>Andreas Pakulat</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/109986/diff/1/?file=138480#file138480line77" style="color: black; font-weight: bold; text-decoration: underline;">plugins/documentview/kdevdocumentviewdelegate.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; ">void KDevDocumentViewDelegate::paint( QPainter *painter, const QStyleOptionViewItem &option, const QModelIndex &index ) const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">73</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_view</span><span class="o">-></span><span class="n">style</span><span class="p">()</span> <span class="o">-></span><span class="n">drawItemText</span><span class="p">(</span> <span class="n">painter</span><span class="p">,</span> <span class="n">textrect</span><span class="p">,</span> <span class="n">Qt</span><span class="o">::</span><span class="n">AlignCenter</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">76</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_view</span><span class="o">-></span><span class="n">style</span><span class="p">()</span> <span class="o">-></span><span class="n">drawItemText</span><span class="p">(</span> <span class="n">painter</span><span class="p">,</span> <span class="n">textrect</span><span class="p">,</span> <span class="n">Qt</span><span class="o">::</span><span class="n">Align<span class="hl">Left</span></span><span class="hl"> </span><span class="o"><span class="hl">|</span></span><span class="hl"> </span><span class="n"><span class="hl">Qt</span></span><span class="o"><span class="hl">::</span></span><span class="n"><span class="hl">AlignV</span>Center</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;">Hmm, this seems to still elide the text - according to your screenshot. So I guess the elidedText function call was never needed?</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 *want* to elide the text (the full path is shown in the tooltip). In the original, the label for categories (mimetypes there) is centered, that doesn't look good for paths and makes it harder to read. So we align left, and elide, if the user needs to see the whole path, she can either make the view wider, or hover with the mouse to see the tooltip. I've tried eliding on different sides (left, middle, right), all but elide right feels weird.
IMO, this should stay as it is.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 13th, 2013, 10:47 a.m. UTC, <b>Andreas Pakulat</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;">In general I think the change is great.</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;">Cool, I'll address the issues you pointed out, and will submit an updated version. (Not sure when I get to it, but I'll make sure it won't drop off the table.)
Also, thanks for the quick review.</pre>
<br />
<p>- Sebastian</p>
<br />
<p>On April 12th, 2013, 11:50 p.m. UTC, Sebastian Kügler wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Sebastian Kügler.</div>
<p style="color: grey;"><i>Updated April 12, 2013, 11:50 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 patch changes the categorization in KDevelop's Documents sidebar to categorizing by path, not by mimetype anymore.
The problems with sorting by mimetypes are:
- files that are related to each other are often far away in the UI
- there are often files with the same name right underneath each other, making picking the right one almost impossible
With this patch, the sidebar is organized per path, in alphabetical order. This makes the location of a file in the UI more predictable, groups file in a more meaningful way.
I've asked a few people on IRC about this, nobody was particularly attached to the current design, so I changed the current one, instead of adding a new plugin. (It makes sense to me personally as well, I don't see how categorization by mimetype there is useful, but my use case is rather limited).
Please give it a try, tell me what you think of it. You can find the code in the sebas/sidebar branch in kdevplatform.</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;">Tested with my actual projects, behaviour feels much better, I now (kind of ;)) enjoy the sidebar, rather than outright hating it. :-)
I didn't notice any particular misbehaviour or breakage, performance also doesn't seem to be a problem.</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/documentview/kdevdocumentviewdelegate.cpp <span style="color: grey">(b172ac0d5b2c1a60cbd45d26e30b72ef725d8678)</span></li>
<li>plugins/documentview/kdevdocumentview.desktop.cmake <span style="color: grey">(a89e79e18a9b79becd669813ead2d2e31f588a4d)</span></li>
<li>plugins/documentview/kdevdocumentview.cpp <span style="color: grey">(b8c4a02ad31fb8aaf29691d7d7d991c4b699360e)</span></li>
<li>plugins/documentview/kdevdocumentview.h <span style="color: grey">(e237f1050aee6c4ef383c809d929b524f94cb938)</span></li>
<li>plugins/documentview/kdevdocumentmodel.cpp <span style="color: grey">(b411a815680ee92d8eaf81b19266f24c6b4a4f13)</span></li>
<li>plugins/documentview/kdevdocumentmodel.h <span style="color: grey">(05cff452c403156cd53e9caaf029d6bbcd394862)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109986/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/04/12/kdevelop-sidebar-original.png">original sidebar</a></li>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/04/12/kdevelop-sidebar-new.png">new sidebar</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>