Review Request 124156: KItemModels: new proxy KRearrangeColumnsProxyModel

Tue Jun 23 18:22:23 UTC 2015

> On jun 23, 2015, 12:26 p.m., Mark Gaiser wrote:
> > src/krearrangecolumnsproxymodel.h, line 71
> > <https://git.reviewboard.kde.org/r/124156/diff/1/?file=381182#file381182line71>
> >
> >     add line break
> 
> David Faure wrote:
>     Where, between methods? I like the current grouping ;)

My preference: a line break after a function defenition if the defenition contained (doxygen) documentation above it's definition.

> On jun 23, 2015, 12:26 p.m., Mark Gaiser wrote:
> > src/krearrangecolumnsproxymodel.h, line 92
> > <https://git.reviewboard.kde.org/r/124156/diff/1/?file=381182#file381182line92>
> >
> >     can you make a smart pointer of this one? I prefer std::unique_ptr<...> but the Qt versions are obviously OK as well :)
> >     
> >     Also removes the need for "delete d_ptr" in the destructor.
> 
> David Faure wrote:
>     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.

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

On jun 23, 2015, 12:26 p.m., David Faure wrote:
> > 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).
> > 
> > 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?
> > 
> > // toggle the visibility of a column
> > void toggleColumnVisibility(int column)
> > {
> >     // pseudo code
> >     setColumnVisible(column, !isColumnVisible(column));
> > }
> > 
> > // set the column visible or hidden, depending on the bool.
> > void setColumnVisible(int column, bool visible)
> > 
> > // returns if a column is visible
> > bool isColumnVisible(int column)
> > 
> > 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.
> 
> David Faure wrote:
>     Thanks for the reminder about astyle-kdelibs, forgot to do that. Done now.
>     
>     Toggle seems overkill (even QHeaderView doesn't have that), but the other two make sense. I'll add that (with QHeaderView naming).
> 
> David Faure wrote:
>     Hmm, it gets a bit hairy.
>     
>     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...
>     
>     (*) 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...

It's not hairy, but i see why you think it is. You're assuming that setColumnHidden hides the _source_ column. That's not how i read your API.

You have setSourceColumns(QVector<int>() << 2 << 1).
Then you have setColumnHidden(2, true)* which in my view hides the second _proxy_ column, not the source column. If it hides the source column the name should reflect that (eg setSourceColumnHidden).

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 _proxy_, not the _source_, right? Or i'm very wrong and need to refresh my Qt Model knowledge when a proxy is involved ;-)

- Mark

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124156/#review81702
-----------------------------------------------------------

On jun 23, 2015, 11:33 a.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124156/
> -----------------------------------------------------------
> 
> (Updated jun 23, 2015, 11:33 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> -------
> 
> It supports reordering and hiding columns from the source model.
> 
> 
> Diffs
> -----
> 
>   autotests/CMakeLists.txt 4e604eeb6bd64529ec1fba983e3f58e2f8d0bd2c 
>   autotests/krearrangecolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h PRE-CREATION 
>   src/CMakeLists.txt dc7d5f04c650f8d21784f16f8d7676e5c74c93e1 
>   src/krearrangecolumnsproxymodel.h PRE-CREATION 
>   src/krearrangecolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124156/diff/
> 
> 
> Testing
> -------
> 
> Wrote it for a customer who allowed me to keep copyright and publish it under the LGPL.
> 
> Unit-tested, and used in two real projects.
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150623/7b9ff46c/attachment-0001.html>