<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 June 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;">
<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="https://git.reviewboard.kde.org/r/124156/diff/1/?file=381182#file381182line92" style="color: black; font-weight: bold; text-decoration: underline;">src/krearrangecolumnsproxymodel.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">92</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KRearrangeColumnsProxyModelPrivate</span> <span class="o">*</span><span class="k">const</span> <span class="n">d_ptr</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">can you make a smart pointer of this one? I prefer std::unique_ptr<...> but the Qt versions are obviously OK as well :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also removes the need for "delete d_ptr" in the destructor.</p></pre>
</blockquote>
<p>On June 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;">I was wondering, but this is still not current practice in any Qt/KDE code I'm looking at.
It requires more #include in the header, which is why I'm not sure about it.</p></pre>
</blockquote>
<p>On June 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;">Some - very few! - do:
http://lxr.kde.org/source/frameworks/kactivities/src/common/database/Database.h
http://lxr.kde.org/source/frameworks/kactivities/src/service/Activities.h
http://lxr.kde.org/source/frameworks/kcompletion/src/kcompletionmatches.h
http://lxr.kde.org/source/frameworks/plasma-framework/src/declarativeimports/platformcomponents/utils/d_ptr.h *
In general: some classes use it, the vast majority doesn't. In this modern C++11 world where we have smart pointers, we should use them. It's way to easy to forget a delete statement.
It might not be the current practice to use them, but it certainly is time (with C++14 available these days!) to start using it.
* ok, this is a custom D_PTR class. More info on that can be found here: http://comments.gmane.org/gmane.comp.kde.devel.frameworks/4644</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">OK, used QScopedPointer; we have to start somewhere, don't we.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 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 June 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 June 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 June 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>
</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;">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>
<br />
<p>- David</p>
<br />
<p>On June 23rd, 2015, 11:33 a.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 June 23, 2015, 11:33 a.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>