<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/124156/">https://git.reviewboard.kde.org/r/124156/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On juni 23rd, 2015, 12:26 p.m. UTC, <b>Mark Gaiser</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regarding the style. AStyle time i think ;)
There is lots of function( arg ) instead of function(arg). ?besides that, some places could use more line breaks for readability (marked above).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Some API questions.
setSourceColumns seems to be used for both rearranging and filtering with no way to change individual columns (that i see). Eg, to toogle them or show/hide columns on demand.
Would it make sense to add the following API functions as well?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">// toggle the visibility of a column
void toggleColumnVisibility(int column)
{
// pseudo code
setColumnVisible(column, !isColumnVisible(column));
}</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">// set the column visible or hidden, depending on the bool.
void setColumnVisible(int column, bool visible)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">// returns if a column is visible
bool isColumnVisible(int column)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Adding the above functionality would make it much more convenient to use in cases where you want to hide/show a column on the fly.</p></pre>
</blockquote>
<p>On juni 23rd, 2015, 2:39 p.m. UTC, <b>David Faure</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks for the reminder about astyle-kdelibs, forgot to do that. Done now.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Toggle seems overkill (even QHeaderView doesn't have that), but the other two make sense. I'll add that (with QHeaderView naming).</p></pre>
</blockquote>
<p>On juni 23rd, 2015, 2:55 p.m. UTC, <b>David Faure</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hmm, it gets a bit hairy.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If I do setSourceColumns(QVector<int>() << 2 << 1) and then setColumnHidden(2, true), I assume I'm hiding source-column-2, so the underlying vector becomes [1].
But if I now do setColumnHidden(2, false), where should column 2 reappear?
For the position to be kept, we would need (like QHeaderView) to distinguish reordering (visual<->logical mapping) and hiding (happens on top).
This makes the implementation more complex and error-prone IMHO. I think I'd rather let the program decide how it wants to handle that "vector of source columns". Maybe it's statically known (like in the first app I wrote this for), or it's replicating what a QHeaderView does at report-generation time (like in the app I'm writing this for right now, see FatCRM on github) (*), and only in a possible third case there would be a need for separating reordering and hiding...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(*) this reminds me, I wanted to add docu about how to replicate what QHeaderView does, using this proxy, but maybe this is only useful for use with KDReports...</p></pre>
</blockquote>
<p>On juni 23rd, 2015, 6:22 p.m. UTC, <b>Mark Gaiser</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's not hairy, but i see why you think it is. You're assuming that setColumnHidden hides the <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">source</em> column. That's not how i read your API.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You have setSourceColumns(QVector<int>() << 2 << 1).
Then you have setColumnHidden(2, true)* which in my view hides the second <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">proxy</em> column, not the source column. If it hides the source column the name should reflect that (eg setSourceColumnHidden).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Think of it this way, if you use the Qt class QSortFilterProxyModel and you call index(row, col) on that class, you get the QModelIndex from the <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">proxy</em>, not the <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">source</em>, right? Or i'm very wrong and need to refresh my Qt Model knowledge when a proxy is involved ;-)</p></pre>
</blockquote>
<p>On juni 23rd, 2015, 9:31 p.m. UTC, <b>David Faure</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That doesn't remove the complexity, since the problem of "what should setColumnHidden(2, false) do afterwards?" is still there, if the underlying implementation is a single QVector<int>. And if it's not, then the implementation needs to skip hidden columns, which becomes more complex.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You're right about index(row, col), but this isn't what this is about. Look at QHeaderView::hideSection, it works on logical indexes, not visual indexes.
Imagine a table with columns like "first name, last name, age, company". Then you want to programmatically use that table for a case where age doesn't matter, you do hideSection(2). This should work, even if the user reordered the columns. This is why the API works on logical indexes, they are the only thing that makes sense to the caller of the API.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Of course in the proxy here there's no such user/programmer distinction, but in order to have some consistency I still think that <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">if</em> there should be any separate API for hiding and reordering, it should probably work like QHeaderView does.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But at this point I think extra API isn't necessary.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thank you for the detailed explenation. I see where you would have that complexity issue.</p></pre>
<br />
<p>- Mark</p>
<br />
<p>On juni 23rd, 2015, 9:33 p.m. UTC, David Faure wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated jun 23, 2015, 9:33 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kitemmodels
</div>
<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;">It supports reordering and hiding columns from the source model.</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Wrote it for a customer who allowed me to keep copyright and publish it under the LGPL.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Unit-tested, and used in two real projects.</p></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>autotests/CMakeLists.txt <span style="color: grey">(4e604eeb6bd64529ec1fba983e3f58e2f8d0bd2c)</span></li>
<li>autotests/krearrangecolumnsproxymodeltest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/test_model_helpers.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(dc7d5f04c650f8d21784f16f8d7676e5c74c93e1)</span></li>
<li>src/krearrangecolumnsproxymodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/krearrangecolumnsproxymodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124156/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>