Review Request: New GroupsModel

[David Edmundson]

> On Nov. 29, 2012, 12:38 a.m., Dan Vrátil wrote:
> > KTp/Models/abstract-grouping-proxy-model.cpp, line 233
> > <http://git.reviewboard.kde.org/r/107377/diff/1/?file=95310#file95310line233>
> >
> >     Won't these two lines leak all the ProxyNodes and GroupNodes?

No. The clear() above will delete all the nodes. My maps are now full of dangling pointers, so we clear them.


> On Nov. 29, 2012, 12:38 a.m., Dan Vrátil wrote:
> > KTp/Models/abstract-grouping-proxy-model.cpp, line 69
> > <http://git.reviewboard.kde.org/r/107377/diff/1/?file=95310#file95310line69>
> >
> >     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...

It's a problem. In theory parent could be another ProxyNode. - but we'll never call this method when parent isn't a GroupNode (in theory!) 
I'll switch to dynamic_cast and try to stress test it.


> On Nov. 29, 2012, 12:38 a.m., Dan Vrátil wrote:
> > KTp/Models/abstract-grouping-proxy-model.cpp, line 231
> > <http://git.reviewboard.kde.org/r/107377/diff/1/?file=95310#file95310line231>
> >
> >     According to QAbstractItemModel docs, you should call beginResetModel() before reseting any internal data and endResetModel() afterwards.

but this is a QStandardItemModel and that would be happening internally.


- David


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


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