Review Request: New GroupsModel

[Dan Vrátil]

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


Just a few observations from my side.


KTp/Models/abstract-grouping-proxy-model.cpp
<http://git.reviewboard.kde.org/r/107377/#comment17371>

    Can parent() return something else then 0 or GroupNode? If this check is supposed to verify that whatever parent() returned was a GroupNode, then it won't work since static_cast never fails...



KTp/Models/abstract-grouping-proxy-model.cpp
<http://git.reviewboard.kde.org/r/107377/#comment17372>

    "{" should go on a new line :)



KTp/Models/abstract-grouping-proxy-model.cpp
<http://git.reviewboard.kde.org/r/107377/#comment17375>

    According to QAbstractItemModel docs, you should call beginResetModel() before reseting any internal data and endResetModel() afterwards.



KTp/Models/abstract-grouping-proxy-model.cpp
<http://git.reviewboard.kde.org/r/107377/#comment17374>

    Won't these two lines leak all the ProxyNodes and GroupNodes?


- Dan Vrátil


On Nov. 19, 2012, 6:10 a.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107377/
> -----------------------------------------------------------
> 
> (Updated Nov. 19, 2012, 6:10 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Initial draft of a replacement for GroupsModel.
> The key differences are:
>  - It doesn't rely on ContactModel* items, just a general model so can be used with the kpeople model as well as ours
>  - It can work on a tree (grouping by the top level items in the tree) for working with the kpeople model
>  - It can group on any property in the model. Not just GroupName. This will allow the KPeople model to still group by account.
>  - As it is generic it can also be used to provide the grouping in the LogViewer(for example), simplifying the entity model code there.
> 
> The code is pretty complex so I would like people to comment if it makes sense.
> 
> It is expected one subclasses this model for a specific purpose. i.e to group by groupNames:
> 
> QSet<QString> GroupGroupingModel::groupsForIndex(const QModelIndex &sourceIndex) const
> {
>     QStringList groups = sourceIndex.data(AccountsModel::GroupsRole).value<QStringList>();
>     if (groups.isEmpty()) {
>         groups.append("_unsorted");
>     }
> 
>     return groups.toSet();
> }
> 
> QVariant GroupGroupingModel::dataForGroup(const QString &group, int role) const
> {
>     switch (role) {
>     case Qt::DisplayRole:
>         if (group == "_unsorted") {
>             return i18n("Unsorted");
>         } else {
>             return group;
>         }
>     case AccountsModel::IdRole:
>         return group;
>     }
>     return QVariant();
> }
> 
> 
> Also if anyone can think of a better class name for something that group nodes by group name that isn't GroupGroupingModel that would be much appreciated.
> 
> 
> Diffs
> -----
> 
>   KTp/Models/abstract-grouping-proxy-model.h PRE-CREATION 
>   KTp/Models/abstract-grouping-proxy-model.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/107377/diff/
> 
> 
> Testing
> -------
> 
> See it in action at: git at git.kde.org:scratch/davidedmundson/groupingmodel
> 
> Tested with ContactsModel->FlatModelProxy->This
> Tested grouping by GroupName, by account, by client type, and grouping by presence.
> 
> In the near future ContactsModel will be a list, so this makes more sense.
> 
> No crashes, everything appeared to work.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20121129/0bf2d3b6/attachment-0001.html>