449260 - Replace rdf-driven addressbook popup menus with xbl based one

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: WIP Patch

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

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP Patch v2

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

Comment 2

[Mark Banner (:standard8)]

Attachment: The fix

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.

Comment 3

[Mark Banner (:standard8)]

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

Comment 4

[[e-mail]]

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 5

[[e-mail]]

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?

Comment 6

[Mark Banner (:standard8)]

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

Comment 7

[Mark Banner (:standard8)]

Attachment: The fix v2

Updated patch addressing Neil's comments. I've left the attributes as they are for now as we don't have anything better.

Comment 8

[[e-mail]]

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

[[e-mail]]

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?

Comment 10

[[e-mail]]

So, my next problem is that the outlook directory doesn't define a dirtype.

Comment 11

[Mark Banner (:standard8)]

(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

Comment 12

[[e-mail]]

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

[[e-mail]]

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

Comment 14

[Mark Banner (:standard8)]

Attachment: The fix v3 [Checkin: See comment 25]

Updated to address Neil's comments, carrying forward his sr.

Comment 15

[Phil Ringnalda (:philor)]

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.

Comment 16

[Phil Ringnalda (:philor)]

And apparently Step 0 is "make sure you have 'Use Mac OS X Address Book' checked."

Comment 17

[Mark Banner (:standard8)]

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

Comment 18

[[e-mail]]

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

Comment 19

[Mark Banner (:standard8)]

(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).

Comment 20

[Phil Ringnalda (:philor)]

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

Comment 21

[Mark Banner (:standard8)]

Attachment: Supplemental patch [Checkin: See comment 25]

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.

Comment 22

[[e-mail]]

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 23

[[e-mail]]

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.

Comment 24

[Phil Ringnalda (:philor)]

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.

Comment 25

[Mark Banner (:standard8)]

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.