<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/103115/">http://git.reviewboard.kde.org/r/103115/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 15th, 2011, 11:53 a.m., <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;">Sorry but this is wrong:
- There's no need for a proxy model at all, simply do it in the source model
- If a proxy is used all places using itemview or selection model indices need to be adjusted to map from source to proxy and vice versa
- QProxyModel is obsolete and should not be used in new code
In addition I think this is no improvement at all. If there are two main.cpp files I'd need to hover over each to find out where in the project
they are located instead of being able to instantly see it.</pre>
</blockquote>
<p>On November 15th, 2011, 8:40 p.m., <b>Carlos Licea</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;">> - There's no need for a proxy model at all, simply do it in the source model
> - If a proxy is used all places using itemview or selection model indices need to be adjusted to map from source to proxy and vice versa
I didn't want to modify the source model for mere aesthetic reasons, hence changing all the users of the original model. I think proxy<->source mapping is unneeded as it's taken care by the proxy class because I'm merely returning a column as tooltip rather than as text, however, if I've broken something, please kindly provide the scenario and I will fix it.
>- QProxyModel is obsolete and should not be used in new code
I strongly disagree with this deprecation, for this simple case scenario I would think it'd make sense to write a proxy rather than replicate all the logic in the source model, update all the clients for the source model or write a new delegate (which is my fallback currently, if you insist I don't use a proxy model.)
>In addition I think this is no improvement at all. If there are two main.cpp files I'd need to hover over each to find out where in the project
they are located instead of being able to instantly see it.
I don't think the current scenario is much better, the path is highly likely to be much bigger than the pane hence forcing you to scroll horizontally which forces you to use your mouse, if your mouse is in the area anyway why not remove the visual clutter? My intention is to clean up the UI, there are currently way to many horizontal scrollbars.
I would argue that the patch provides a cleaner feel to the pane.</pre>
</blockquote>
<p>On November 16th, 2011, 8 a.m., <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;">Proxy <-> source mapping is _always_ needed since the Qt proxy models do not guarantee that just because the proxy does not filter or sort the indices will be the same. It might change them anyway for whatever reasons. Also the only user right now is the projectmanagerview plugin.
Regarding the deprecation: I did not talk about proxy models per se, just the class QProxyModel is deprecated in Qt. You should either use QSortFilterProxyModel or QAbstractProxyModel, especially looking forward to Qt5 where QProxyModel will not exist anymore.
You're trading having to scroll with having to hover for n seconds and that does not improve the usability at all. And usability is far more important that having some shiny UI - IMHO. If you want to improve things please come up with an easy way to show the path all the time that does not need scrolling. Maybe the buildset widget should be separated into its own view, now that toolviews can be arranged int tabs. (IIRC) That would allow moving it to the bottom area where there is much more horizontal space.</pre>
</blockquote>
<p>On November 17th, 2011, 12:17 p.m., <b>Milian Wolff</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;">Just want to chime in and say that Andreas is right. Just because your proxy<->source mapping magically works now does not mean it will continue to work in the future, if someone extends the proxy functionality. If one then has to spent time to figure out the reasons just to see that it's due to an improper implementation in the beginning is very frustrating.</pre>
</blockquote>
<p>On November 18th, 2011, 6:15 p.m., <b>Carlos Licea</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;">Yes, I perfectly understand the problem with my current implementation.
However, I'm not sure I can satisfy Andreas' request to always show the path while at the same time fixing what I wanted to fix: visual clutter.
I think I'll drop the review request.</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;">could you maybe show screenshots comparing status quo to your new implementation? maybe start a discussion on our mailing list, we could try to find a working, better solution together.</pre>
<br />
<p>- Milian</p>
<br />
<p>On November 12th, 2011, 8:14 a.m., Carlos Licea 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 Carlos Licea.</div>
<p style="color: grey;"><i>Updated Nov. 12, 2011, 8:14 a.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;">I removed some clutter by removing the path and showing it on a tooltip. I also removed the top header.</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/projectmanagerview/CMakeLists.txt <span style="color: grey">(7b58e8e)</span></li>
<li>plugins/projectmanagerview/projectbuildsetproxymodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/projectmanagerview/projectbuildsetproxymodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/projectmanagerview/projectbuildsetwidget.cpp <span style="color: grey">(585a1c8)</span></li>
<li>plugins/projectmanagerview/projectbuildsetwidget.ui <span style="color: grey">(90e74e1)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103115/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>