----------------------------------------------------------- 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: [email protected]: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 > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
