Address Book lists in dialogs may become unsorted

RESOLVED FIXED in Thunderbird 3.0a3

Status

P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: standard8, Assigned: neil)

Tracking

Trunk
Thunderbird 3.0a3
Dependency tree / graph
Bug Flags:
wanted-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
In Bug 449260 Neil suggested:

> Ideally you'd keep the array of directories as a member and then you can a) use
> it to validate the notifications and b) use it to sort a new entry into the
> correct position (speaking of which, shouldn't renaming an entry re-sort it?)

I agree this would be an improvement. Neil said he'd be happy to do it, so assigning to him.
Flags: blocking-thunderbird3+
(Reporter)

Comment 1

10 years ago
Not blocking, but definitely wanted.
Flags: blocking-thunderbird3+ → wanted-thunderbird3+
Priority: -- → P2
Summary: In addrbookWidgets.xml store the directories in an array and use for validation and re-sorting → Address Book lists in dialogs may become unsorted
(Reporter)

Comment 2

10 years ago
Created attachment 334085 [details] [diff] [review]
WIP Patch

Stealing: I decided that I needed the array implementation for being able to provide an alternative value for the menuitems (i.e. dirPrefId as opposed to URI).

This patch changes over to the array implementation and fixes some bugs in the onItemAdded routine (as well as avoiding teardown and rebuilding).

I think the only other thing I need to fix now is re-sorting the list in onItemPropertyChanged.
Assignee: neil → bugzilla
Status: NEW → ASSIGNED
(Assignee)

Comment 3

10 years ago
Comment on attachment 334085 [details] [diff] [review]
WIP Patch

Isn't the node for this._books[i] always going to be this.childNodes[i]?
(Assignee)

Comment 4

10 years ago
Created attachment 334173 [details] [diff] [review]
untested

Also the code may be unreadable but I hope you get the idea ;-)
(Reporter)

Comment 5

10 years ago
(In reply to comment #4)
> Created an attachment (id=334173) [details]
> untested
> 
> Also the code may be unreadable but I hope you get the idea ;-)
> 
This looks better than my version. When I tried it just now, it didn't seem to work too well. Initially renames and deletes weren't working. I then restarted and trying adding an ab (and that worked), but then renaming, partially worked but was throwing errors.
Assignee: bugzilla → neil
Status: ASSIGNED → NEW
(Assignee)

Comment 6

10 years ago
Created attachment 334259 [details] [diff] [review]
tested

OK, so I had to switch from indexOf to a for loop because indexOf requires strict equality which we don't get for xpconnect objects. (This isn't a problem when deleting/renaming a new directory because we already have a strictly equal xpconnect object to compare against.) I also fixed a typo which prevented renames from working (passing _compare() instead of _compare to sort).
Attachment #334173 - Attachment is obsolete: true
(Reporter)

Updated

10 years ago
Attachment #334259 - Flags: review?(bugzilla)
(Reporter)

Updated

10 years ago
Attachment #334085 - Attachment is obsolete: true
(Reporter)

Updated

10 years ago
Attachment #334259 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

10 years ago
Attachment #334259 - Flags: superreview?(dmose)
(Reporter)

Updated

10 years ago
Blocks: 408613
(Reporter)

Updated

10 years ago
Blocks: 451381

Comment 7

10 years ago
Keeping wanted‑thunderbird3+.
Whiteboard: [needs sr:dmose]
Target Milestone: --- → mozilla1.9.1a2

Updated

10 years ago
Target Milestone: mozilla1.9.1a2 → Thunderbird 3.0b1
(Assignee)

Comment 8

10 years ago
Created attachment 335034 [details] [diff] [review]
Added none="&noLDAPServer.label;" attribute

I added an extra entry in the array to track the menuitem as that seems to be simpler than adjusting the offsets depending on whether the menuitem exists.
Attachment #334259 - Attachment is obsolete: true
Attachment #335034 - Flags: superreview?(dmose)
Attachment #335034 - Flags: review?(bugzilla)
Attachment #334259 - Flags: superreview?(dmose)
(Reporter)

Updated

10 years ago
Attachment #335034 - Flags: review?(bugzilla) → review+

Comment 9

10 years ago
Comment on attachment 335034 [details] [diff] [review]
Added none="&noLDAPServer.label;" attribute

There are enough things here that are under-documented that this is turning out to be significantly more work to review than it should be, and this also means that the code is going to be hard to maintain for future patchers.

>diff -r 5c1aae64a225 mailnews/addrbook/resources/content/addrbookWidgets.xml
>--- a/mailnews/addrbook/resources/content/addrbookWidgets.xml	Thu Aug 21 23:36:13 2008 +0200
>+++ b/mailnews/addrbook/resources/content/addrbookWidgets.xml	Fri Aug 22 13:24:32 2008 +0100
>@@ -44,27 +44,57 @@
>   <binding id="addrbooks-menupopup"
>            extends="chrome://global/content/bindings/popup.xml#popup">
>     <implementation implements="nsIAbListener">
>+      <field name="_directories">[]</field>
>+
>+      <field name="_value">this.getAttribute("value") || "URI"</field>

It's not clear to me what the point of "URI" is above.  Can you add a comment explaining?

>+          if (this.hasAttribute("none")) {
>+            this._directories.unshift(null);
>+            menulist.insertItemAt(0, this.getAttribute("none"), "");
>+          }

Please document how the "none" attribute works, who sets it, etc.

>+            if (index != -1)
>+              if (this.parentNode.selectedItem ==
>+                  this.removeChild(this.childNodes[index]))
>+                if (this.hasChildNodes())
>+                  this.firstChild.doCommand();
>+                else
>+                  this.parentNode.selectedItem = null;

The above code could use at least a comment, and might make sense to break up for readability.

>+      <method name="_contains">
>+        <parameter name="ab"/>

This method needs some commentary explaining the exact semantics of what it's doing.  I'm not totally convinced that _contains is the right name: it's used in building up the array, when it's got nothing to do with what the widget actually contains at that time, but rather what it should contain, inferred from the various attributes on the widget.  I'm not really sure what the best name would be, _shouldContain might be a little better, but that's not great either.

>         <body><![CDATA[
>+          const nsIAbDirectory = Components.interfaces.nsIAbDirectory;
>+          return (ab.isRemote ? this.getAttribute("localonly") != "true"
>+                              : this.getAttribute("remoteonly") != "true") &&
>+                 (ab.supportsMailingLists ||
>+                  this.getAttribute("supportsmaillists") != "true") &&
>+                 ((ab.operations & nsIAbDirectory.opWrite) ||
>+                  this.getAttribute("writeable") != "true");

I know this code was just moved from what was here previously, but it's really a pain to read, which makes it hard to review as well as fragile to maintain.  I suspect this wants to be split up into multiple statements with local variables.  The various different attributes also want to be documented, I think.

One thing that was nice about the old code is that most non-trivial clauses had comments explaining what they were trying to do.  This made it much easier to follow the code flow as well as meant that there was an english expression describing what the code was supposed to be doing that the reviewer/reader can audit the actual code behavior against.  Please give the code a once-over and do this, if you would; then I'll take another pass.
Attachment #335034 - Flags: superreview?(dmose) → superreview-
(Assignee)

Comment 10

10 years ago
Created attachment 337279 [details] [diff] [review]
Updated for review comments (!)

I changed "_contains" to "_matches" to indicate what it does.
I also gave the method alternative body which you might prefer.
I hope I added sufficient comments for you to understand the code.
Attachment #335034 - Attachment is obsolete: true
Attachment #337279 - Flags: superreview?(dmose)
Attachment #337279 - Flags: review+

Updated

10 years ago
Attachment #337279 - Flags: superreview?(dmose) → superreview+
Comment on attachment 337279 [details] [diff] [review]
Updated for review comments (!)

Code is definitely more approachable & maintainable; thank you!

>+      <!-- Represents the nsIAbDirectory attribute used as the value of the
>+           parent menulist. Defaults to URI but can be e.g. dirPrefId -->
>+      <field name="_value">this.getAttribute("value") || "URI"</field>

Clever.

>+          // Attempt to select the persisted or otherwise first directory.
>+          menulist.value = menulist.value;

Why write to .value rather than .selectedItem?

>+          /* This code was vetoed by dmose, but I can't think why...

Heh.  You don't really _have_ to keep this comment in there, but you're welcome to if you wish.  :-)

Don't forget to add yourself to the license boilerplate.

sr=dmose
(Assignee)

Comment 12

10 years ago
(In reply to comment #11)
> >+          // Attempt to select the persisted or otherwise first directory.
> >+          menulist.value = menulist.value;
> Why write to .value rather than .selectedItem?
The .value setter simply finds the .selectedItem for us, if there is one, based on the preferred nsIAbDirectory attribute that was persisted for us. The alternative would have been for us to keep track of which nsIAbDirectory has the attribute corresponding to the value as we build up the list.
(Assignee)

Comment 13

10 years ago
Pushed changeset 3776ea557ca4 to comm-central.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Updated

10 years ago
Whiteboard: [needs sr:dmose]
(In reply to comment #12)
> (In reply to comment #11)
> > >+          // Attempt to select the persisted or otherwise first directory.
> > >+          menulist.value = menulist.value;
> > Why write to .value rather than .selectedItem?
> The .value setter simply finds the .selectedItem for us, if there is one, based
> on the preferred nsIAbDirectory attribute that was persisted for us. The
> alternative would have been for us to keep track of which nsIAbDirectory has
> the attribute corresponding to the value as we build up the list.

That's what the getter half of the statement does, not the setter half.  But why assign it the returned value to .value rather than assigning it to .selectedItem?
(Assignee)

Comment 15

10 years ago
No, the value getter simply reads the menulist's value attribute.

menulists have three properties identifying the current selection:
1. selectedItem - the actual node in the current DOM
2. selectedIndex - the (numeric) index in childNodes of selectedItem
3. value - a (string) abstract representation stored as an attribute
Setting any one of the properties will update the other two.

So when you write menulist.value = menulist.value; you're taking the persisted attribute and asking the menulist to convert it to a DOM node and child index.
(Reporter)

Updated

10 years ago
Depends on: 459358
You need to log in before you can comment on or make changes to this bug.