Review Request 124331: New proxy: KExtraColumnsProxyModel, allows to add columns to an existing model.

[Sune Vuorela]

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


I like the concept, and have a whole bunch of nitpickery and a couple of more important bits.


src/kextracolumnsproxymodel.h (line 92)
<https://git.reviewboard.kde.org/r/124331/#comment56881>

    where is the implementation of this? (and the unit tests ?  :)



src/kextracolumnsproxymodel.cpp (line 43)
<https://git.reviewboard.kde.org/r/124331/#comment56877>

    Are this one of the cases where mmutz and mwolff won't hunt you down for using QList ?



src/kextracolumnsproxymodel.cpp (line 77)
<https://git.reviewboard.kde.org/r/124331/#comment56875>

    Is this assert actually needed? isn't it kind of valid to pass in a nullptr ?



src/kextracolumnsproxymodel.cpp (line 78)
<https://git.reviewboard.kde.org/r/124331/#comment56876>

    shouldn't the old source model's about to be changed signals be disconnected here, or are QAPM taking care of that ?



src/kextracolumnsproxymodel.cpp (line 99)
<https://git.reviewboard.kde.org/r/124331/#comment56880>

    isn't this kind of a normal state. is noisy output warranted here?



src/kextracolumnsproxymodel.cpp (line 115)
<https://git.reviewboard.kde.org/r/124331/#comment56878>

    I know it is widely used outside Qt but .. Q_D is actually not public Qt api. Similar for Q_Q.



src/kextracolumnsproxymodel.cpp (line 170)
<https://git.reviewboard.kde.org/r/124331/#comment56879>

    this is surprising me a bit. I'd expect that I by default would have emitted dataChanged inside my implementation of setExtraColumnData, especially if setting a piece of data influences multiple roles.



src/kextracolumnsproxymodel.cpp (line 194)
<https://git.reviewboard.kde.org/r/124331/#comment56874>

    Wouldn't it be better to redelegate this to a set of separate functions, just like the actual data so that other roles are actually supported? (In my similar, but not as thorough implementations of this, I've often needed other roles like color and/or decoration)


- Sune Vuorela


On July 13, 2015, 8:31 a.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124331/
> -----------------------------------------------------------
> 
> (Updated July 13, 2015, 8:31 a.m.)
> 
> 
> Review request for KDE Frameworks, Jesper Pedersen and Stephen Kelly.
> 
> 
> Repository: kitemmodels
> 
> 
> Description
> -------
> 
> REVIEW: 124331
> 
> 
> Diffs
> -----
> 
>   autotests/CMakeLists.txt f98e87d49b965998f31c5ede1fefb03b159a150f 
>   autotests/kextracolumnsproxymodeltest.cpp PRE-CREATION 
>   autotests/test_model_helpers.h a44db8a9cb985f69b52be96f0ffe389ebd903d6f 
>   src/CMakeLists.txt 82d776f1fcc2f7b15e74f0ed47702ed570ce9892 
>   src/kextracolumnsproxymodel.h PRE-CREATION 
>   src/kextracolumnsproxymodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124331/diff/
> 
> 
> Testing
> -------
> 
> Wrote autotests (as extensive as possible); used this in one real project, AFAIK Jesper (blackie) did as well.
> 
> 
> Thanks,
> 
> David Faure
> 
>

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