----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101108/#review2599 -----------------------------------------------------------
What you've done is very well written, and incredibly well documented. However, there's a big downside to the way you've done it which limits the functionality of the traditional QSortFilterProxyModel. We should be extending the mechanism there, not adding another way of sorting alongside the one QSortFilterProxy provides. Sorry if this appears negative, it's not meant as such. You clearly know your C++, but models take a bit of getting used to, to get full use out of them. P.S "onlineness" isn't a word (though this is my fault because I mentioned it in an email once. Presence is a better term. account-filter-model.h <http://git.reviewboard.kde.org/r/101108/#comment2280> Very nice commenting - I approve of that. What I don't like however is using a boolean "sort like this" when the class already inherits a method called setFilterRole() http://doc.trolltech.com/4.7/qsortfilterproxymodel.html#filterRole-prop where the filter role should be Qt::DisplayRole for sorting by name and AccountsModel::PresenceTypeRole for presence. A user of this class will be exposed to both sets of methods you don't want one to work, the other not to. account-filter-model.h <http://git.reviewboard.kde.org/r/101108/#comment2278> Why would you have this and setSortByOnlineNess? account-filter-model.cpp <http://git.reviewboard.kde.org/r/101108/#comment2281> So with my comments above I would do this as: if (sortRole() == AccountsModel::PresenceTypeRole) { leftPresence = ... rightPresence = ... return ... } else { QSortFilterProxyModel(left, right); } That way we keep as much of it working with the original sort functionality as possible - so without any extra code a client also has working "sort by status message" by setting the sort role to AccountsModel::StatusMessage (for example). Also you can use this model in a standard QTreeView and sorting would work when you click on the column headers. With your custom way it would not. account-filter-model.cpp <http://git.reviewboard.kde.org/r/101108/#comment2282> I'm really not convinced this works, if you're going to treat them like ints then there's no point overriding this method at all. I'm pretty confident (though I may be wrong) this will put offline users at the top, then online, away, busy. I think that we want to treat them as the enum values they are and need something more like: if (leftPresence == Tp::PresenceOnline) { return false; } if (rightPresence == Tp::PresenceOnline { return true; } if (leftPresence == Tp::PresenceAway) { return false; etc etc. account-filter-model.cpp <http://git.reviewboard.kde.org/r/101108/#comment2279> You would want to invalidate the filter here. However my other comments make these 3 method redundant - David On April 12, 2011, 9:05 p.m., Rémy Greinhofer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/101108/ > ----------------------------------------------------------- > > (Updated April 12, 2011, 9:05 p.m.) > > > Review request for Telepathy. > > > Summary > ------- > > Sorts the contacts by name offers the possibility to sort it also by status. > > > This addresses bug 246223. > http://bugs.kde.org/show_bug.cgi?id=246223 > > > Diffs > ----- > > account-filter-model.h d33231d > account-filter-model.cpp 7182362 > main-widget.h 18f97ee > main-widget.cpp a24ccf0 > main-widget.ui d000b9b > > Diff: http://git.reviewboard.kde.org/r/101108/diff > > > Testing > ------- > > 1. Open the contact list, the contacts are sorted by name. > 2. Click on the "Sort by onlineness button", the contacts are sorted by > onlineness and by name. > > > Thanks, > > Rémy > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
