439236 - js-driven xbl binding for selecting addressbooks

Status: RESOLVED WORKSFORME

(Thunderbird :: Address Book, defect)

Description

[Joey Minta]

Attachment: patch v1

There are a large number of rdf-driven menulists for selecting addressbooks.  We can eliminate these with something much like the folder-list binding.

The attached patch contains the binding, along with fixes for a couple of consumers.  There are more consumers out there, but these are included for illustrative/testing purposes.  (Also, patching AbSearchDialog allowed me to get rid of another global rdf service. :-) )

Comment 1

[Mark Banner (:standard8)]

So what I was really hoping for with these menus, was to fix bug 422845, and then redo them in a similar style to the folder drop down menus (e.g. Search Messages).

AFAIK the folder menu-list binding integrates a tree view into the popup, and I was therefore thinking the same code from the AB directory tree could be integrated in a binding to provide the same type of menu list integration.

Neil, should this be possible once we have bug 422845 fixed?

Sorry I haven't voiced this earlier, I just wasn't expecting this yet.

Comment 2

[[e-mail]]

As I recall there are three types of addressing menulist (although strangely only two seem to be implemented here).
a) all directories (used for searching e.g. addressing dialog, sidebar panel)
b) writable directories (used for creating cards)
c) writable directories that support mailing lists (used for mailing lists!)
However, unlike the main address book, mailing lists are currently never selectable; do you want to be able to create a card directly into a mailing list using the create card UI as opposed to the mailing list edit UI?

Comment 3

[Joey Minta]

I haven't gotten to consumers of type (b) yet, but a filter for that would be inserted at that time.  That's why only two types are implemented at this point.

Comment 4

[[e-mail]]

Oh, and of course RDF doesn't forget the selected item when you add one.

Comment 5

[[e-mail]]

Oh, and we need to select any persisted saved selection.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 325103 [details] [diff] [review]
patch v1

I'm happy that with this we'll be able to do what I want (when I get round to writing it). I think we can improve on this a little bit though (see comments below).


>@@ -89,19 +87,16 @@ function searchOnLoad()
>   // initialize a flag for phonetic name search
>   var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>                               .getService(Components.interfaces.nsIPrefService);
>   var prefBranch = prefService.getBranch(null).QueryInterface(Components.interfaces.nsIPrefBranch2);
>   gSearchPhoneticName =
>         prefBranch.getComplexValue("mail.addr_book.show_phonetic_fields", 
>                                    Components.interfaces.nsIPrefLocalizedString).data;
> 
>-  if (window.arguments && window.arguments[0])
>-    SelectDirectory(window.arguments[0].directory);
>-

This means we no longer pre-select the book based on which address book is selected - I see no reason for dropping this, please restore the functionality.


>diff -r 10403f5931b6 mail/components/addrbook/content/abContactsPanel.js
>--- a/mail/components/addrbook/content/abContactsPanel.js	Tue Jun 10 22:31:11 2008 -0400
>+++ b/mail/components/addrbook/content/abContactsPanel.js	Sat Jun 14 12:06:21 2008 -0400
>@@ -82,58 +82,47 @@ var gAddressBookPanelAbListener = {
>     // will only be called when an addressbook is deleted
>     try {
>       var directory = item.QueryInterface(Components.interfaces.nsIAbDirectory);
>       // check if the item being removed is the directory
>       // that we are showing in the addressbook sidebar
>       // if so, select the person addressbook (it can't be removed)
>       if (directory == GetAbView().directory) {
>           var abPopup = document.getElementById('addressbookList');
>-          abPopup.value = kPersonalAddressbookURI;
>-          LoadPreviouslySelectedAB();
>+          abPopup.selectAddressbook(GetDirectoryFromURI(kPersonalAddressbookURI));
>+          ChangeDirectoryByURI(abPopup.selectedItem._addrbook.URI);
>       } 


Can we not move this functionality into your binding now? Your binding listens to the same events, so surely it can correct the selection and call the oncommand handler? Otherwise we're going to have two sets of listeners per dialog.


>diff -r 10403f5931b6 mail/components/addrbook/content/abContactsPanel.xul
>--- a/mail/components/addrbook/content/abContactsPanel.xul	Tue Jun 10 22:31:11 2008 -0400
>+++ b/mail/components/addrbook/content/abContactsPanel.xul	Sat Jun 14 12:06:21 2008 -0400

>-      </menulist>
>+      <menulist id="addressbookList" type="addrbook" mode="directories" flex="1"
>+                oncommand="AddressBookMenuListChange();" persist="value"/>
>     </hbox>

The value is no longer set in your binding, so we're not going to persist it, I think I want it kept, so could you set the value to the URI?

>diff -r 10403f5931b6 mailnews/addrbook/resources/content/addrbook-bindings.xml
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/mailnews/addrbook/resources/content/addrbook-bindings.xml	Sat Jun 14 12:06:22 2008 -0400
>@@ -0,0 +1,237 @@

>+      <destructor><![CDATA[
>+          var abManager = Components.classes["@mozilla.org/abmanager;1"]
>+                                    .getService(Components.interfaces.nsIAbManager)
>+          abManager.removeAddressBookListener(this._listener);
>+      ]]></destructor>

You don't need the temporary for abManager here.

>+      <!--
>+         - Our listener to let us know when addrbooks change/appear/disappear so
>+         - we can know to rebuild ourselves.
>+        -->
>+      <field name="_listener"><![CDATA[({
>+        _menu: this,

_menu isn't used anywhere.

>+        onItemAdded: function abook_onItemAdded(aParent, aItem) {
>+          // We only care about top-level changes
>+          if (aParent.URI != "moz-abdirectory://")
>+            return;

This doesn't work, aParent is an nsISupports (ditto with onItemRemoved). We should probably fix that in a follow-up bug.

>+        onItemRemoved: function abook_onItemRemoved(aParent, aItem) {
>+          // We only care about top-level changes
>+          if (aParent.URI != "moz-abdirectory://")
>+            return;
>+          for (var i in this._popup.childNodes) {
>+            var child = this._popup.childNodes[i];
>+            if (!child || !child._addrbook)
>+              continue;

Why would we have a null child?

>+
>+            if (child._addrbook.URI == aItem.URI) {
>+              this._popup.removeChild(child);
>+              return;
>+            }
>+          }
>+        },

>+      <!--
>+         - Adds menuitems for each addrbook that passes the filter
>+        -->
>+      <method name="_build">
>+        <body><![CDATA[
>+          this._popup = this.appendChild(document.createElement("menupopup"));
>+
>+          const Cc = Components.classes;
>+          const Ci = Components.interfaces;
>+          var abManager = Cc["@mozilla.org/abmanager;1"]
>+                                    .getService(Ci.nsIAbManager)

There's probably no ideal solution here, but I think I'd prefer the . aligned with the [ on the line above.

>+          var addrEnum = abManager.directories;
>+
>+          var books = [];
>+          while (addrEnum.hasMoreElements()) {
>+            books.push(addrEnum.getNext().QueryInterface(Ci.nsIAbDirectory));
>+          }
>+
>+          var mode = this.getAttribute("mode");
>+          if (mode)
>+            books = books.filter(this._filters[mode]);
>+
>+          // Sadly, the abManager won't give us the books in a sorted order, so
>+          // we have to do our own sorting here
>+          function abSort(a, b) {

I wonder how easy this would be to do in the abManager. This function doesn't currently work, my menus are currently showing:

PAB
LDAP
some local
Mac (Dir type = 3)
some more local
CAB

It should be:

PAB
local
LDAP
Mac
CAB

I also doubt your algorithm sorts internally where you have more than one LDAP address book. I'll see if I can come up with something for this tomorrow, ping me if I don't.

>+          for each (var book in books) {
>+            var menuitem = document.createElement("menuitem");
>+            menuitem._addrbook = book;

_addrbook implies it is a private/restricted member, however you're accessing it everywhere. I think just addrbook would be better.

>+            menuitem.setAttribute("label", book.dirName);
>+            this._popup.appendChild(menuitem);
>+          }
>+
>+          this.selectedIndex = 0;
>+          
>+          abManager.addAddressBookListener(this._listener, Ci.nsIAbListener.all);

Having this in _build means we will miss-match listener addition and removal. Probably just best to put it in the constructor so that it matches with the destructor.

>+      <!--
>+         - Removes all menu-items for this popup, resets all fields, and
>+         - removes the listener.

Except it doesn't reset all fields or removes the listener. Like I said above, I don't think it needs to reset the listener.

>+        -->
>+      <method name="_teardown">
>+        <body><![CDATA[
>+          while(this._popup.hasChildNodes())
>+            this._popup.removeChild(this._popup.lastChild);
>+        ]]></body>
>+      </method>


>--- a/mailnews/jar.mn	Tue Jun 10 22:31:11 2008 -0400
>+++ b/mailnews/jar.mn	Sat Jun 14 12:06:22 2008 -0400
>@@ -32,16 +32,18 @@ messenger.jar:
>     content/messenger/addressbook/abSelectAddressesDialog.xul                  (addrbook/resources/content/abSelectAddressesDialog.xul)
>+    content/messenger/addressbook/addrbook-bindings.xml)                               (addrbook/resources/content/addrbook-bindings.xml)
>+    content/messenger/addressbook/bindings.css)                               (addrbook/resources/content/bindings.css)
>     content/messenger/addressbook/addressbook.js                               (addrbook/resources/content/addressbook.js)

Looks like spacing needs sorting out here, there are also extra brackets that shouldn't be present.

Comment 7

[Mark Banner (:standard8)]

> >+          // Sadly, the abManager won't give us the books in a sorted order, so
> >+          // we have to do our own sorting here
> >+          function abSort(a, b) {
> 
> I wonder how easy this would be to do in the abManager. This function doesn't
> currently work, my menus are currently showing:
> 
> PAB
> LDAP
> some local
> Mac (Dir type = 3)
> some more local
> CAB
> 
> It should be:
> 
> PAB
> local
> LDAP
> Mac
> CAB
> 
> I also doubt your algorithm sorts internally where you have more than one LDAP
> address book. I'll see if I can come up with something for this tomorrow, ping
> me if I don't.

So as David said on another bug, the sort code tends to belong to the view/menu. I think for now, let's keep it in the js where you have it, but fix it to match what we have currently. When we get to remove rdf from the address book completely, and redo how abManager holds its books, then we'll look at it again.

I think the best way to fix this, would be to store/generate a key like nsAbDirectoryDataSource does it at the moment, and then sort on that key:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/addrbook/src/nsDirectoryDataSource.cpp&rev=1.90&mark=536-551,560-598#529

Note, to simplify things, I think that if it is not of type local (i.e. 2 / PABDirectory), then just prepend the integer for the type onto the sort string.

Also something to watch is the sneaky "position" sort that goes onto the string before the dirType - this means the code doesn't quite tie up with the comments.

Comment 8

[Mark Banner (:standard8)]

Hmm, looks like I did this in bug 449260 and must have forgotten about this bug at the time. Marking as WFM as this was basically fixed by bug 449260.