Review Request: Fix bug 246223 - Filter by status proxy model

[Martin Klapetek]

> On April 12, 2011, 10:55 p.m., David Edmundson wrote:
> > 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.

The problem here is that QSortFilterProxy does not support sorting by two columns (presence and name). Therefore the lessThan(..) had to be overriden and reimplemented to be able to sort by both. 

As for the "onlineness" - David's right, this should be "presence" (as that is used throughout the Telepathy as well).


> On April 12, 2011, 10:55 p.m., David Edmundson wrote:
> > account-filter-model.h, line 51
> > <http://git.reviewboard.kde.org/r/101108/diff/3/?file=14239#file14239line51>
> >
> >     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.

You probably meant sortRole() :) But that's the problem I described above - you can't do a two-columns-sorting with the sortRole(). So you either get sort-by-name XOR sort-by-presence, not both. And we (should) want to have the list sorted by name as well when sorted by presence.


> On April 12, 2011, 10:55 p.m., David Edmundson wrote:
> > account-filter-model.h, line 63
> > <http://git.reviewboard.kde.org/r/101108/diff/3/?file=14239#file14239line63>
> >
> >     Why would you have this and setSortByOnlineNess?

That's true, remove one of these.


> On April 12, 2011, 10:55 p.m., David Edmundson wrote:
> > account-filter-model.cpp, line 106
> > <http://git.reviewboard.kde.org/r/101108/diff/3/?file=14240#file14240line106>
> >
> >     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.
> >

The purpose of lessThan(..) is to compare values to do the sort and it seems like a good way to compare the uints (if they are ordered like 0-online, 1-away...7-offline, otherwise it's no use). In your proposed way it would have to compare all combinations of presences, no? Like if(leftPresence == Online && rightPresence == Away) return true; if(leftPresence == Online && rightPresence == Busy) return true;

or am I getting this wrong? :)

Also, Rémy, try to test with offline users shown.


- Martin


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


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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110413/9984a101/attachment.htm