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

[Aurélien Gâteau]

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



plasma/declarativeimports/qtextracomponents/fullmodelaccess.h
<http://git.reviewboard.kde.org/r/106272/#comment14453>

    include should be changed to #include <QAbstractListModel>



plasma/declarativeimports/qtextracomponents/fullmodelaccess.h
<http://git.reviewboard.kde.org/r/106272/#comment14448>

    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()



plasma/declarativeimports/qtextracomponents/fullmodelaccess.h
<http://git.reviewboard.kde.org/r/106272/#comment14449>

    Properties should have a NOTIFY signal. This makes it possible to bind other properties to them.



plasma/declarativeimports/qtextracomponents/fullmodelaccess.h
<http://git.reviewboard.kde.org/r/106272/#comment14457>

    I assume you need this static method to easily init rootIndex.
    
    I think it should be renamed to something a bit more explicit, like indexForModelItem()



plasma/declarativeimports/qtextracomponents/fullmodelaccess.h
<http://git.reviewboard.kde.org/r/106272/#comment14450>

    This function should be const.



plasma/declarativeimports/qtextracomponents/fullmodelaccess.cpp
<http://git.reviewboard.kde.org/r/106272/#comment14452>

    Coding style: kdelibs coding style says brackets are mandatory for one-line if (there are other occurences of this further down).



plasma/declarativeimports/qtextracomponents/fullmodelaccess.cpp
<http://git.reviewboard.kde.org/r/106272/#comment14451>

    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.



plasma/declarativeimports/qtextracomponents/fullmodelaccess.cpp
<http://git.reviewboard.kde.org/r/106272/#comment14454>

    Message should be more explicit, and should use kWarning() so that it includes class and method name. If using kWarning() is not an option, use qWarning() << __FUNCTION__ << ...



plasma/declarativeimports/qtextracomponents/tests/fullmodelaccesstest.h
<http://git.reviewboard.kde.org/r/106272/#comment14456>

    I see you're copy'n'pasting my precious unittest code :)



plasma/declarativeimports/qtextracomponents/tests/fullmodelaccesstest.cpp
<http://git.reviewboard.kde.org/r/106272/#comment14455>

    I think it would be interesting to also run ModelTest *after* a root index and source model has been set.


- Aurélien Gâteau


On Aug. 30, 2012, 10:43 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. 30, 2012, 10:43 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/20120830/a31e2995/attachment-0001.html>