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

[David Edmundson]

> 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.
> >
> 
> Martin Klapetek wrote:
>     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.

Your statement contains a massive "if" there.
I'm pretty sure the statuses are in this order:
http://telepathy.freedesktop.org/spec/Connection_Interface_Simple_Presence.html#Enum:Connection_Presence_Type
Which isn't a sensible order for listing.

Also I don't think we need to do every possibly combo if it's written like suggested, if either of them is "online" then that side 'wins' and you return either true or false. Otherwise you continue onto the next presence, if either of them are 'away' that way you only need each status once - though it's still pretty lengthy.

You may be right that it is easiest to turn the presence type into a rank score, which does have 0 for online, 7 for offline, then compare those. I would be happy with that. I just don't think it works as-is.


- David


-----------------------------------------------------------
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/9e8cf918/attachment.htm