> 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: [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
