Review Request 124156: KItemModels: new proxy KRearrangeColumnsProxyModel

[Mark Gaiser]

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



autotests/krearrangecolumnsproxymodeltest.cpp (line 180)
<https://git.reviewboard.kde.org/r/124156/#comment56017>

    coding style.
    void setup(KRearrangeColumnsProxyModel &pm)
    {
        // code...
    }



autotests/test_model_helpers.h (line 50)
<https://git.reviewboard.kde.org/r/124156/#comment56018>

    Coding style. { on new line.



src/krearrangecolumnsproxymodel.h (line 71)
<https://git.reviewboard.kde.org/r/124156/#comment56019>

    add line break



src/krearrangecolumnsproxymodel.h (line 74)
<https://git.reviewboard.kde.org/r/124156/#comment56020>

    add line break



src/krearrangecolumnsproxymodel.h (line 76)
<https://git.reviewboard.kde.org/r/124156/#comment56021>

    add line break



src/krearrangecolumnsproxymodel.h (line 81)
<https://git.reviewboard.kde.org/r/124156/#comment56022>

    add line break



src/krearrangecolumnsproxymodel.h (line 92)
<https://git.reviewboard.kde.org/r/124156/#comment56023>

    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.



src/krearrangecolumnsproxymodel.cpp (line 30)
<https://git.reviewboard.kde.org/r/124156/#comment56024>

    coding style.
        : QIdentityProxyModel(parent)
        , d_ptr(new KRearrangeColumnsProxyModelPrivate)



src/krearrangecolumnsproxymodel.cpp (line 36)
<https://git.reviewboard.kde.org/r/124156/#comment56025>

    can go if you make d_ptr a smart pointer.



src/krearrangecolumnsproxymodel.cpp (lines 66 - 71)
<https://git.reviewboard.kde.org/r/124156/#comment56029>

    A little bit more line breaks here would improve readability.



src/krearrangecolumnsproxymodel.cpp (line 96)
<https://git.reviewboard.kde.org/r/124156/#comment56026>

    if () {
        // code..
    }



src/krearrangecolumnsproxymodel.cpp (line 105)
<https://git.reviewboard.kde.org/r/124156/#comment56027>

    if () {
        // code..
    }



src/krearrangecolumnsproxymodel.cpp (line 114)
<https://git.reviewboard.kde.org/r/124156/#comment56028>

    It seems fairly simple to implement this comment. Could you...? :)
    
    A "No!" is ok as well.


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.

- Mark Gaiser


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/41c3e594/attachment-0001.html>