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

[David Faure]

> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.h, line 92
> > <https://git.reviewboard.kde.org/r/124331/diff/2/?file=385017#file385017line92>
> >
> >     where is the implementation of this? (and the unit tests ?  :)

Ouch, good catch, I forgot to finish this. Done now.


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 43
> > <https://git.reviewboard.kde.org/r/124331/diff/2/?file=385018#file385018line43>
> >
> >     Are this one of the cases where mmutz and mwolff won't hunt you down for using QList ?

No, sizeof(QPersistentModelIndex) = one pointer, so it's fine. (and I got this from QIdentityProxyModel, which mmutz would have fixed by now if it was wrong ;) ).


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 78
> > <https://git.reviewboard.kde.org/r/124331/diff/2/?file=385018#file385018line78>
> >
> >     shouldn't the old source model's about to be changed signals be disconnected here, or are QAPM taking care of that ?

Good point, fixed.


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 99
> > <https://git.reviewboard.kde.org/r/124331/diff/2/?file=385018#file385018line99>
> >
> >     isn't this kind of a normal state. is noisy output warranted here?

No, this should never ever happen. Every time it does, it means something will go terribly wrong very soon, i.e. it's a bug in the proxy which doesn't reimplement some method that ends up going into that code path. Putting a breakpoint on this line has been very useful to fix all the bugs :-)


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 115
> > <https://git.reviewboard.kde.org/r/124331/diff/2/?file=385018#file385018line115>
> >
> >     I know it is widely used outside Qt but .. Q_D is actually not public Qt api. Similar for Q_Q.

At this point, it's so widely used in KDE and other libs, that we would all fight for some compatibility, if this was ever to change in Qt.
I claim this is fine, based on the amount of prior art (332 instances of Q_Q and 1739 instances of Q_D in all the frameworks).

Anyhow, they're both one-liners, we can easily replicate them locally if this is a problem one day.


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 170
> > <https://git.reviewboard.kde.org/r/124331/diff/2/?file=385018#file385018line170>
> >
> >     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.

I thought this would make things easier, especially since setExtraColumnData doesn't get a QModelIndex, so you wouldn't be able to emit dataChanged. The whole idea being that the proxy reimplementation doesn't need to know/care about how many the source model has.
But yeah, extraColumnDataChanged actually allows doing this just fine, I'll change it.


> On July 13, 2015, 7:42 p.m., Sune Vuorela wrote:
> > src/kextracolumnsproxymodel.cpp, line 194
> > <https://git.reviewboard.kde.org/r/124331/diff/2/?file=385018#file385018line194>
> >
> >     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)

In the headers?

I see two solutions: 1) a virtual headerDataForExtraColumn, 2) letting you reimplement headerData and using extraColumnForProxyColumn() from there (I'll make it protected in any case, for anything the proxy doesn't provide out of the box, like DnD support...).

What do you think?

I'm not sure we should provide a new set of virtual methods methods for everything (this reminds me of Q3ScrollView::viewportMouseMoveEvent and friends :-). Data mapping is the hard part, header data isn't.


- David


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


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/20150714/f911b236/attachment.html>