Review Request 124156: KItemModels: new proxy KRearrangeColumnsProxyModel

[David Faure]

> On June 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

Where, between methods? I like the current grouping ;)


> On June 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.

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.


> On June 23, 2015, 12:26 p.m., Mark Gaiser wrote:
> > src/krearrangecolumnsproxymodel.cpp, line 30
> > <https://git.reviewboard.kde.org/r/124156/diff/1/?file=381183#file381183line30>
> >
> >     coding style.
> >         : QIdentityProxyModel(parent)
> >         , d_ptr(new KRearrangeColumnsProxyModelPrivate)

Interesting, astyle-kdelibs didn't fix that one.


> On June 23, 2015, 12:26 p.m., Mark Gaiser wrote:
> > src/krearrangecolumnsproxymodel.cpp, line 114
> > <https://git.reviewboard.kde.org/r/124156/diff/1/?file=381183#file381183line114>
> >
> >     It seems fairly simple to implement this comment. Could you...? :)
> >     
> >     A "No!" is ok as well.

Ah, I meant to adjust that comment. Changed to "if this is too slow, ...". Better not spend more memory and CPU (in other places) unless we actually need to. In practice the number of items in this vector is around 10 (tableviews with 100 columns are not usable) so this linear search in a contiguous vector is actually pretty fast.


On June 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.

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


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


On June 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 June 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/479e891b/attachment.html>