Review Request: Make it possible to use Tree models from QML

[Aurélien Gâteau]

> On Aug. 30, 2012, 12:17 p.m., Aurélien Gâteau wrote:
> > plasma/declarativeimports/qtextracomponents/fullmodelaccess.h, line 25
> > <http://git.reviewboard.kde.org/r/106272/diff/2/?file=82325#file82325line25>
> >
> >     I don't like the "FullModelAccess" name. It is a model, so to avoid confusion its name should end with "Model". It works like a proxy model, so I suggest renaming it to ColumnProxyModel.
> >     
> >     I am also wondering whether the code wouldn't be simpler if the class was inheriting from QAbstractProxyModel rather than QAbstractListModel. I think it would make it possible to remove:
> >     - data()
> >     - rowCount()
> >     - headerData()
> >     - sourceDestroyed()
> >     - sourceModel()
> 
> Marco Martin wrote:
>     +1 for ColumnProxyModel.
>     good also to put more emphasys on the columns rather than the possibility to dive in the tree nodes, that is here only because visualdatamodel can't be used in conjunction with other proxies, since doesn't inherit from qaim
> 
> Aleix Pol Gonzalez wrote:
>     I already tried that, but I didn't like it because it provides an API that suggests that mapFrom/ToSource will work.
>     
>     Also reducing those won't help much. The big part of the code is the forwarding of the signals and proxy models doesn't do that. I tried with QIdentityProxyModel, but then it tries to forward all signals and we only need some of them.
>     
>     Regarding the name, I like the addition of proxy, although I think it's misleading if it doesn't inherit QAbstractProxyModel. I don't really like to have "Column" there.
> 
> Aurélien Gâteau wrote:
>     > it provides an API that suggests that mapFrom/ToSource will work
>     
>     Isn't it the case? I would indeed expect mapFrom/ToSource to work. If they don't work then I agree the class should not inherit from QAbstractProxyModel, and should have "Proxy" in its name.
>     
>     > Regarding the name, I like the addition of proxy, although I think it's misleading if it doesn't inherit QAbstractProxyModel. I don't really like to have "Column" there.
>     
>     This model gives you access to one column of a multi-column model, which can be a tree model or a list model, that is why I think "Column" should be in the name, but maybe it will be more often used to access the first column of a subtree within a tree model. If it cannot inherit from QAbstractProxyModel I suggest ListSubModel or SubListModel.
> 
> Aleix Pol Gonzalez wrote:
>     Regarding Proxies:
>     well, in the proxy you have less items than in the source. That is all the other columns and tree branches don't have a representation so you keep returning invalid QModelIndex'es for those.
>     
>     Regarding the name:
>     I think I'll change to ColumnProxyModel, I'm lacking any inspiration whatsoever in this regard and I don't want to stop this patch because of this :).

> well, in the proxy you have less items than in the source. That is all the other columns and tree branches don't have a representation so you keep returning invalid QModelIndex'es for those.

Sure, but that is perfectly fine. Proxy models can be used to:
a) sort: in this case mapFromSource() should never return invalid QModelIndex
b) filter: in this case it is expected that mapFromSource() may return an invalid QModelIndex


- Aurélien


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106272/#review18278
-----------------------------------------------------------


On Aug. 31, 2012, 3:45 p.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106272/
> -----------------------------------------------------------
> 
> (Updated Aug. 31, 2012, 3:45 p.m.)
> 
> 
> Review request for Plasma, Aurélien Gâteau and Marco Martin.
> 
> 
> Description
> -------
> 
> This patch adds a component called ListifyModel (yeah, I hate the name too). The idea behind is to expose as a QAbstractListModel any part of a QAbstractItemModel.
> 
> This solves the problem we have in QML given the limitation that ListView only displays the first column of the root items. Here we can specify what column we want and what root index we want to have.
> 
> 
> Diffs
> -----
> 
>   plasma/declarativeimports/qtextracomponents/CMakeLists.txt 05a1195 
>   plasma/declarativeimports/qtextracomponents/fullmodelaccess.h PRE-CREATION 
>   plasma/declarativeimports/qtextracomponents/fullmodelaccess.cpp PRE-CREATION 
>   plasma/declarativeimports/qtextracomponents/qtextracomponentsplugin.cpp 429282e 
>   plasma/declarativeimports/qtextracomponents/tests/CMakeLists.txt PRE-CREATION 
>   plasma/declarativeimports/qtextracomponents/tests/fullmodelaccesstest.h PRE-CREATION 
>   plasma/declarativeimports/qtextracomponents/tests/fullmodelaccesstest.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/106272/diff/
> 
> 
> Testing
> -------
> 
> There's a passing unit test, albeit limited.
> I also tested it with a QML example I had with KPeople. If anybody is interested I can provide it too.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20120903/462dd89e/attachment.html>