Closed Bug 954206 Opened 8 years ago Closed 8 years ago

Use a sortComparator function on groups to allow easy changing of the order of contacts

Categories

(Instantbird :: Contacts window, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: mattdentremont)

References

Details

Attachments

(1 file, 4 obsolete files)

*** Original post on bio 772 at 2011-05-04 20:12:00 UTC ***

We could use a sortComparator function that takes two contacts and returns the order of the contacts instead of directly sorting on names in group.xml on several places.
This would allow to easily replace the sortComparator by another that for example will sort on online status/availability/.. instead of alphabetical order.

Right now the observe method of groups is looking for contact name changes.. so something has to be done here as well to account for the other changes.
Blocks: 954136
Blocks: 954385
*** Original post on bio 772 at 2011-08-31 00:51:32 UTC ***

From [1]:
> 19:46:30 <flo> we could add a sortComparator method and use it in all places where we currently have a comparison on the names
> 19:46:41 <Mic> That's what I meant
> 19:47:33 <Mic> Right now it's pre-sorting on names when building the group and checking again/sorting in in addContact again (using the names again)
> 19:48:08 <flo> and checking in contact-display-name-changed too!

[1] http://log.bezut.info/instantbird/110504/#m253
*** Original post on bio 772 as attmnt 891 by mattdentremont AT gmail.com at 2011-10-14 00:42:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352634 - Flags: review?(florian)
Comment on attachment 8352634 [details] [diff] [review]
Replacing hard coded comparisons with calls to a new comparator method

*** Original change on bio 772 attmnt 891 at 2011-10-14 01:02:29 UTC ***

>+     
>+     <!-- Takes as input two contact elements. -->
Good comment, but to be more specific: what is the function doing and what it should return, -1 if contact A should be first, 1 if contact B should be first, 0 if they're the same?

>+     <method name="nameComparator">
>+      <parameter name="contactEltA"/>
>+      <parameter name="contactEltB"/>
We always start our parameters with "a" and then capitalize the next letter, so these would probably be aContactA and aContactB.

I briefly looked over the other code, but I'm not too familiar with this, just trying to save flo a review.
Attachment #8352634 - Flags: review?(florian) → review-
*** Original post on bio 772 as attmnt 892 by mattdentremont AT gmail.com at 2011-10-14 01:18:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352635 - Flags: review?(florian)
Comment on attachment 8352634 [details] [diff] [review]
Replacing hard coded comparisons with calls to a new comparator method

*** Original change on bio 772 attmnt 891 by mattdentremont AT gmail.com at 2011-10-14 01:18:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352634 - Attachment is obsolete: true
Comment on attachment 8352635 [details] [diff] [review]
Corrected comments and parameter names.

*** Original change on bio 772 attmnt 892 at 2011-10-14 10:16:57 UTC ***

>+     <method name="aNameComparator">
Mic pointed this out on IRC (I guess I'm not a great reviewer), so we only change the parameters to start with "a", your original method name (nameComparator) was fine! Sorry if I wasn't clear.
Attachment #8352635 - Flags: review?(florian) → review-
*** Original post on bio 772 at 2011-10-14 10:19:39 UTC ***

(In reply to comment #5)
> your original method name (nameComparator) was fine!

No it wasn't, but for another reason.
The point of this change is to let add-ons override this method with a function comparing based on something else than the name.
"sortComparator" would be a better name.

(By the way, don't attach a new patch yet, I have other comments to write.)
*** Original post on bio 772 at 2011-10-14 10:30:00 UTC ***

(In reply to comment #6)
> (In reply to comment #5)
> > your original method name (nameComparator) was fine!
> 
> No it wasn't, but for another reason.
> The point of this change is to let add-ons override this method with a function
> comparing based on something else than the name.
> "sortComparator" would be a better name.

So my review wasn't good either, clokep ;)


About the patch:
I find the comment of the comparator method confusing. Maybe just say that the meaning of the values is the same as in 
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/sort#Parameters
Comment on attachment 8352635 [details] [diff] [review]
Corrected comments and parameter names.

*** Original change on bio 772 attmnt 892 at 2011-10-15 22:52:42 UTC ***

>diff --git a/instantbird/content/group.xml b/instantbird/content/group.xml
>--- a/instantbird/content/group.xml
>+++ b/instantbird/content/group.xml
>@@ -88,21 +88,17 @@
>                                .getService(Components.interfaces.nsIRDFDataSource);
>         var RDF = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>                             .getService(Components.interfaces.nsIRDFService);
>         var elt = RDF.GetResource(document.location + "#" + this.id);
>         if (source.HasAssertion(elt, RDF.GetResource("closed"),
>                                 RDF.GetLiteral("true"), true))
>           this.setAttribute("closed", "true");
> 
>-        contacts.sort(function (a, b) {
>-          a = a.displayName.toLowerCase();
>-          b = b.displayName.toLowerCase();
>-          return a < b ? -1 : a > b ? 1 : 0;
>-        }).forEach(this.addContact, this);
>+        contacts.sort(this.aNameComparator).forEach(this.addContact, this);

The contacts array here contains imIContact instances, not contact.xml bindings.

>@@ -206,75 +202,87 @@
>            let index = this.contacts.indexOf(contactElt);
>            if (index == -1) {
>              // Sometimes we get a display-name-changed notification for
>              // an offline contact, if it's not in the list, just ignore it.
>              return;
>            }
>            // See if the position of the contact should be changed.
>            if (index != 0 &&
>-               name < this.contacts[index - 1].sortName ||
>+               this.aNameComparator(contactElt, this.contacts[index - 1]) == -1 ||
>                index != this.contacts.length - 1 &&
>-               name > this.contacts[index + 1].sortName) {
>+               this.aNameComparator(contactElt, this.contacts[index + 1]) == 1) {

* In order to always pass the sort comparator objects of the same type, you need to pass imIContact instances here too. The "this.contacts" array contains contact.xml instances.

To get the imIContact instance from a contact.xml elt, you just need to use elt.contact.

* Rather than checking == -1 and == 1, you should check < 0 and > 0.


* This code should be moved to a separate method, taking as a parameter a contact, checking that its position is correct, and moving it if necessary. This would for example allow an add-on sorting by availability to listen for the contact-status-changed notifications and call this method when the status of a contact changes. If we don't move this code to a separate method, an add-on doing that would need to duplicate this code.

>+     
>+     <!-- Takes as input two contact elements (defined in contact.xml) and
>+          - compares their nicknames alphabetically (case insensitive).
>+          - Returns -1 if aContactA is less than aContactB, 0 if they are
>+          - equal, or 1 if aContactA is greater than aContactB. -->

So, this method should take imIContact instances as parameter, and behave like a callback that Array.sort would accept as parameter.
That's enough for the comment, as the description at https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/array/sort#Parameters is quite good :-).

>+     <method name="aNameComparator">

This isn't a good name. The point of this patch is that an add-on could replace the comparator with a comparator comparing contacts based on something else than the name, so the name of the method should certainly not contain "name" :).

sortComparator would be good I think.

>+      <parameter name="aContactA"/>
>+      <parameter name="aContactB"/>
>+      <body>
>+      <![CDATA[
>+        let a = aContactA.displayName.toLowerCase();
>+        let b = aContactB.displayName.toLowerCase();
>+        return a < b ? -1 : a > b ? 1 : 0;

return a.localeCompare(b);

That's shorter, easier to understand, and has the nice benefit that it would work well with non-en-US characters too :-).
Attachment #8352635 - Flags: review-
*** Original post on bio 772 at 2011-10-22 14:57:53 UTC ***

Assigning this to v17al, but let us know if you can't finish this for whatever reasons and someone will. :)
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attached patch PatchV2 (obsolete) — Splinter Review
*** Original post on bio 772 as attmnt 929 by mattdentremont AT gmail.com at 2011-10-24 00:01:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352671 - Flags: review?(florian)
Comment on attachment 8352635 [details] [diff] [review]
Corrected comments and parameter names.

*** Original change on bio 772 attmnt 892 by mattdentremont AT gmail.com at 2011-10-24 00:01:52 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352635 - Attachment is obsolete: true
Comment on attachment 8352671 [details] [diff] [review]
PatchV2

*** Original change on bio 772 attmnt 929 at 2011-10-25 16:36:09 UTC ***

(In reply to comment #8)

> * Rather than checking == -1 and == 1, you should check < 0 and > 0.

I still see 2 places where you test == -1.

> So, this method should take imIContact instances as parameter, and behave like
> a callback that Array.sort would accept as parameter.
> That's enough for the comment

Any reason why your new comment includes only the type of the parameter? I said very clearly in my previous review what were the 2 things that comment needed to say.


The indentation inside your updateContactPosition method is messed up to the point that it's unreadable.
Attachment #8352671 - Flags: review?(florian) → review-
Attached patch v3 (obsolete) — Splinter Review
*** Original post on bio 772 as attmnt 942 by mattdentremont AT gmail.com at 2011-10-26 04:07:00 UTC ***

Sorry about the carelessness of that last patch, did it very quick..
Attachment #8352684 - Flags: review?(florian)
Comment on attachment 8352671 [details] [diff] [review]
PatchV2

*** Original change on bio 772 attmnt 929 by mattdentremont AT gmail.com at 2011-10-26 04:07:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352671 - Attachment is obsolete: true
Comment on attachment 8352684 [details] [diff] [review]
v3

*** Original change on bio 772 attmnt 942 at 2011-10-26 09:57:23 UTC ***

>diff --git a/instantbird/content/group.xml b/instantbird/content/group.xml

>+        // See if the position of the contact should be changed.
>+        if (index != 0 &&
>+                        this.sortComparator(contactElt.contact, this.contacts[index - 1].contact) < 0 ||
>+                        index != this.contacts.length - 1 &&
>+                        this.sortComparator(contactElt.contact, this.contacts[index + 1].contact) > 0) {

That indent is still messed up.

>+          // Check if something prevents us from moving the contact now.
>+          if (contactElt.hasAttribute("selected") ||
>+                          contactElt.hasAttribute("open"))

Here too.
Attachment #8352684 - Flags: review?(florian) → review-
Attached patch v4Splinter Review
*** Original post on bio 772 as attmnt 961 by mattdentremont AT gmail.com at 2011-10-31 01:38:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352703 - Flags: review?(florian)
Comment on attachment 8352684 [details] [diff] [review]
v3

*** Original change on bio 772 attmnt 942 by mattdentremont AT gmail.com at 2011-10-31 01:38:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352684 - Attachment is obsolete: true
Blocks: 954583
Comment on attachment 8352703 [details] [diff] [review]
v4

*** Original change on bio 772 attmnt 961 at 2011-11-10 00:43:11 UTC ***

This looks good, thanks! Sorry for the delay before getting to review this.

I think at some point someone said that we should create an add-on as part of this bug to verify that the new API is actually usable. This sounded like a good idea (+ I would really like us to have an add-on for what's requested in bug 954136 (bio 701)), but I'm not sure any more it's a good reason to delay the integration of this patch (and risking some bitrot) + this also fixes bug 954583 (bio 1150). :)
Attachment #8352703 - Flags: review?(florian) → review+
*** Original post on bio 772 at 2011-11-10 00:53:59 UTC ***

Thanks for working on this! Should allow some great add-ons. :)

Checked in as http://hg.instantbird.org/instantbird/rev/d9a296c543d9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Depends on: 954667
There was missing email mapping information for this bug during the BIO to BMO merge, manually assigning this bug.
Assignee: bugzilla → mattdentremont
You need to log in before you can comment on or make changes to this bug.