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

[Aleix Pol Gonzalez]

> 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

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.


> On Aug. 30, 2012, 12:17 p.m., Aurélien Gâteau wrote:
> > plasma/declarativeimports/qtextracomponents/fullmodelaccess.cpp, line 38
> > <http://git.reviewboard.kde.org/r/106272/diff/2/?file=82326#file82326line38>
> >
> >     Maybe the content of this if() can be replaced with:
> >     
> >     disconnect(m_sourceModel, 0, this, 0);
> >     
> >     Given the number of connections, it would avoid future problems if one adds a new signal but forgets to add a disconnect() call there.

If we disconnect everything the user might have done weird things and they'll stop working.

On the other hand, like this way we just clean up what we did, and I think it's fine.


> On Aug. 30, 2012, 12:17 p.m., Aurélien Gâteau wrote:
> > plasma/declarativeimports/qtextracomponents/tests/fullmodelaccesstest.h, line 32
> > <http://git.reviewboard.kde.org/r/106272/diff/2/?file=82329#file82329line32>
> >
> >     I see you're copy'n'pasting my precious unittest code :)

;)


- Aleix


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


On Aug. 31, 2012, 7:53 a.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, 7:53 a.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/20120831/9d7e086f/attachment.html>