Closed Bug 449260 Opened 11 years ago Closed 11 years ago

Replace rdf-driven addressbook popup menus with xbl based one

Categories

(MailNews Core :: Address Book, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Attached patch WIP Patch (obsolete) — Splinter Review
There's various ways we're currently implementing out address book popup menus, mostly rdf, there are one or two js driven ones hanging around as well.

I want to replace this completely by a xbl based one this has various advantages, including fixing problems with rdf menus for extensions.

I have a basic implementation already in place (attaching), it doesn't quite work for TB yet, I think I've got a css problem somewhere.

I also just want to see if I can do a couple of extra tweaks so that menuitems update from the menupopup's listener (I'm not sure if this is a bit of a hack though, so we'll see).
Flags: blocking-thunderbird3?
Attached patch WIP Patch v2 (obsolete) — Splinter Review
I think this is almost there now - works for all the dialogs in TB & SM excluding the preferences windows and Account settings (these menus haven't been touched yet).
Attachment #332397 - Attachment is obsolete: true
Attached patch The fix (obsolete) — Splinter Review
Various improvements and fixes over the WIP patches.

Changes all the menu popups that currently use rdf except for those in the Account Manager and Preferences dialog (although support for those is already present in the new popup).

The fix to Thunderbird's abCommon.js for the selectedDir check is porting a fix from SeaMonkey that is also needed to stop assertions when updating during a directory deletion.

All the listener code is now within the new popup widget. This gives us the big advantage of not needing it to duplicate, and will fix updating of the menulist display which will fix bug 198137 and possibly some others.
Attachment #332533 - Attachment is obsolete: true
Attachment #332712 - Flags: superreview?(neil)
Attachment #332712 - Flags: review?(philringnalda)
Blocks: 198137
Priority: -- → P1
Neil, it looks like we'll probably want to implement a widget for listboxes (i.e. list of checkboxes) is I can make this widget into one and re-use the code within it? (If I have to have two widgets, then that's what I'll do, I just want to check first).
No, you can't reuse the code in XBL1 because your popup widget needs to extend the existing popup widget while your listbox widget needs to extend the existing listbox widget. XBL2 supports multiple inheritance though ;-)
Comment on attachment 332712 [details] [diff] [review]
The fix

>+  // Select the first address book
>+  abList.value = document.getElementById("addressbookList-menupopup")
>+                         .firstChild.value;
Am I missing something, or can you use
abList.selectedIndex = 0;

>+    <implementation>
In theory you can write <implementation implements="nsIAbListener"> and then define the listener <method>s directly, which avoids the use of "_menu".

>+            // Re-use _teardown and _build so that we can use its sort function
>+            this._menu._teardown();
>+            this._menu._build();
This will leave the menulist in an inconsistent state whereby its currently selected item refers to an element that has since been removed from the DOM. If you subsequently rename that address book then the update will not propagate correctly.

>+            for (var i = 0; i < childNodes.length; ++i) {
>+              if (childNodes[i].value == uri) {
Can you not use .getElementsByAttribute?

>+            // Only update the parent item if it is a menulist, as that's
>+            // all we know about.
Out of interest, what other parent are you considering?

>+              // Just set it to the first child.
>+              this._menu.parentNode.value = this._menu.firstChild.value;
>+
>+              // Fire a command event to update anything as necessary
>+              var handler = this._menu.parentNode.getAttribute("oncommand");
>+              if (handler) {
>+                var fn = new Function(handler);
>+                fn.apply(this._menu.parentNode);
>+              }
This bit is actually wrong, and as it happens the correct code this._menu.firstChild.doCommand(); also updates the menulist.

>+          // Empty out anything in the list
IIRC comment style is to match a capital letter with a full stop.

>+          var remoteOnly = this.getAttribute("remoteonly");
>+          var localOnly = this.getAttribute("localonly");
I'm wondering whether we should use isremote="false"/"true" instead.

>+                (!remoteOnly || (remoteOnly && ab.isRemote)) &&
>+                (!localOnly || (localOnly && !ab.isRemote)) &&
>+                (!sml || (sml && ab.supportsMailingLists)) &&
>+                (!writeable || (writeable &&
>+                                (ab.operations & nsIAbDirectory.opWrite) ==
>+                                 nsIAbDirectory.opWrite)))
A bit of Boolean algebra:
(¬p ∨ (p ∧ q)) ≡ (¬p ∨ p) ∧ (¬p ∨ q)
But (¬p ∨ p) is a tautology, so this reduces to (¬p ∨ q)
also you only test for the presence, not the value of the attribute?

>+            // Sort anything else by the dir type
>+            return a.dirType > b.dirType ? 1 : a.dirType < b.dirType ? 0 : -1;
return a.dirType - b.dirType;

>+          var menupopup = this;
That's annoying.

>+menupopup[type="addrbooks"] {
I'd prefer this used a unique class, then you could refer to it as .addrbooksPopup

>-function onChooseDirectory(event) 
>+function onChooseDirectory() 
> {
>-    var directoryURI = event.id;
>-    if (directoryURI) {
>-        SelectDirectory(directoryURI);
>-    }
>+  SelectDirectory(document.getElementById("abPopup").value);
> }
Might it be worth writing SelectDirectory(this.value); inline in the XUL?
(In reply to comment #5)
> >+          var remoteOnly = this.getAttribute("remoteonly");
> >+          var localOnly = this.getAttribute("localonly");
> I'm wondering whether we should use isremote="false"/"true" instead.

The problem I had here (and I'm open to suggestions) is that essentially we need three states:

1) All Address Books
2) Local Address Books only
3) Remove Address Books only

So we could set isremote to false or true for the last two cases, but I thought it was a bit strange with the first case - normally boolean values seem to default to false or true, not something else.
Attached patch The fix v2 (obsolete) — Splinter Review
Updated patch addressing Neil's comments. I've left the attributes as they are for now as we don't have anything better.
Attachment #332712 - Attachment is obsolete: true
Attachment #333216 - Flags: superreview?(neil)
Attachment #333216 - Flags: review?(philringnalda)
Attachment #332712 - Flags: superreview?(neil)
Attachment #332712 - Flags: review?(philringnalda)
(In reply to comment #5)
>(From update of attachment 332712 [details] [diff] [review])
>>+  // Select the first address book
>>+  abList.value = document.getElementById("addressbookList-menupopup")
>>+                         .firstChild.value;
>Am I missing something, or can you use
>abList.selectedIndex = 0;
In fact there's a separate bug because that menulist happens to persist its value so we should try to reselect the previous address book if possible.
Comment on attachment 333216 [details] [diff] [review]
The fix v2

>+      <method name="onItemPropertyChanged">
>+        <parameter name="aItem"/>
>+        <parameter name="aProperty"/>
>+        <parameter name="aOldValue"/>
>+        <parameter name="aNewValue"/>
>+        <body><![CDATA[
>+          if (aItem instanceof Components.interfaces.nsIAbDirectory) {
>+            // Find the item in the list to remove
rename, surely? (also, this and the original are missing their full stop)

>+      // Other methods
Possibly "private methods" would be more descriptive?
So, my next problem is that the outlook directory doesn't define a dirtype.
(In reply to comment #10)
> So, my next problem is that the outlook directory doesn't define a dirtype.
> 
So I bet you get assertions/warnings normally then:

http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsDirectoryDataSource.cpp#572

...and I believe it *should* be defining a dirType of 4 (MAPIDirectory) as per later on in that function and http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsDirPrefs.h#58
(In reply to comment #11)
>(In reply to comment #10)
>> So, my next problem is that the outlook directory doesn't define a dirtype.
>So I bet you get assertions/warnings normally then:
>http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsDirectoryDataSource.cpp#572
Right... I missed those because of XPCOM_DEBUG_BREAK=warn (sorry)

>...and I believe it *should* be defining a dirType of 4 (MAPIDirectory) as per
>later on in that function and
>http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsDirPrefs.h#58
Confusingly 4 is the sort order; MAPIDirectory actually has a value of 3.
Comment on attachment 333216 [details] [diff] [review]
The fix v2

Not relevant to this patch but somehow while testing I managed to clear the name of an addressbook...
Attachment #333216 - Flags: superreview?(neil) → superreview+
Depends on: 450248
Updated to address Neil's comments, carrying forward his sr.
Attachment #333216 - Attachment is obsolete: true
Attachment #333419 - Flags: superreview+
Attachment #333419 - Flags: review?(philringnalda)
Attachment #333216 - Flags: review?(philringnalda)
Attachment #333419 - Flags: review?(philringnalda) → review+
Comment on attachment 333419 [details] [diff] [review]
The fix v3
[Checkin: See comment 25]

Wait, something I don't quite get is wrong here. Steps to blow up:

1. Open AB, New List, verify that the list of possible ABs is okay.
2. Open compose, open contacts sidebar, close compose.
3. Open AB (wait through a beachball until menulist.xml throws "too much recursion), New List, note that the menu is dozens of copies of each possible AB.
Attachment #333419 - Flags: review+ → review-
And apparently Step 0 is "make sure you have 'Use Mac OS X Address Book' checked."
(In reply to comment #15)
> (From update of attachment 333419 [details] [diff] [review])
> Wait, something I don't quite get is wrong here. Steps to blow up:

I think I may have seen this once when I was playing around, however I can't reproduce it (and yes, I've got the mac address book enabled).

I've backed it out for now, but some more steps to repeat would be useful.
Didn't the old sidebar code remove the address book listener when you closed the compose window? (I'm not sure if we can do this with the new binding.)
(In reply to comment #18)
> Didn't the old sidebar code remove the address book listener when you closed
> the compose window? (I'm not sure if we can do this with the new binding.)

It only removed it when the panel was unloaded from memory. There was a separate hook for compose-window-close which would clear the search.

Additionally I would doubt that that would be the problem - Phil didn't mention adding/deleting/renaming address books (and I've just tried playing around again, and still not caused it).
Looks like maybe the missing step 0 is "create a (non-empty) mailing list in your OS X addressbook." (Creating an empty mailing list doesn't appear to trigger this, though it does seem to be a little on the crashy side.)
This is a diff against the v3 patch - I thought it would be easier to see what I'm doing.

The main cause of this is bug 439304 - the OS X directories are doing nasty things like re-adding lists to the main address book when they are already there. This is happening underneath GetDirectories which is being called from _build. onItemAdded calls _build, hence the recursion.

So, I've now attached a patch to fix that there to that bug, but I also want to protect against other cases where code may do the wrong thing (I don't think there's anywhere we do it wrong, but extensions may do it wrong). I've therefore added a protection in, worst case we'll end up with an incorrect display of a list, but I think that's better than causing recursion all the time.
Attachment #333600 - Flags: superreview?(neil)
Attachment #333600 - Flags: review?(philringnalda)
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?)
Comment on attachment 333600 [details] [diff] [review]
Supplemental patch
[Checkin: See comment 25]

If you don't want to rewrite/tweak the binding as per my previous comment then I'm happy to have a go at it after you've checked in your work so far.

>+          this._building = false;
>           this._build();
You don't need this as the first thing _build() does is to set it to true.
Attachment #333600 - Flags: superreview?(neil) → superreview+
Comment on attachment 333600 [details] [diff] [review]
Supplemental patch
[Checkin: See comment 25]

Flags like that always make me worry that we'll fail out with a relatively harmless exception and wind up never unsetting, but, guess we'll find out if we do.
Attachment #333600 - Flags: review?(philringnalda) → review+
Blocks: 450581
I've now landed both these patches, in one changeset, id: 97:bbb70f28e335

(In reply to comment #22)
> 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?)

This sounds like a good idea. I decided that we're gaining a lot from this patch, and getting it in allows us to move forward with other items, and the regression here is small. Hence I landed it and filed bug 450581.
Flags: blocking-thunderbird3?
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 467809
Attachment #333419 - Attachment description: The fix v3 → The fix v3 [Checkin: See comment 25]
Attachment #333600 - Attachment description: Supplemental patch → Supplemental patch [Checkin: See comment 25]
You need to log in before you can comment on or make changes to this bug.