422845 - Replace rdf-driven addressbook directory tree with js one

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Joey Minta]

Attachment: patch (no updates)

Standard8 asked me to look at replacing the directory pane with a js-driven tree.  It's pretty straightforward, as this patch shows.  This patch depends on the PROTO_TREE_VIEW that's floating around bugzilla.  At the moment, I've chosen not to make this patch depend on iteratorUtils.jsm, though it could also benefit from that.

The remaining question I'll need someone to help me with is where I hook in a listener/observer to be notified when someone changes/adds/deletes a directory.

I barely resisted the urge to trash a bunch of the global vars here.  They're on notice though.

Comment 1

[Joey Minta]

Attachment: patch v1

I found nsIAbListener, so this should now update when changes are made.  Looks good to go, and this is as good of a place as any to introduce PROTO_TREE_VIEW.

Comment 2

[Mark Banner (:standard8)]

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

Please put the new directoryTree items in a separate new file in mailnews/addrbook/r.../c..., I am hoping to use it in replacing some of the drop down menus, so addressbook.js isn't the best place for it.

There appears to be no sort functionality, or if there is, then I think its how nsDirPrefs is reading/sorting it. I'd prefer it to behave like the existing for the time being:

http://mxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsDirectoryDataSource.cpp#527

Mailing lists don't get displayed - there's a '-' next to the ab, but no items below it, and I get the following error when trying to toggle it:

[Exception... "'[JavaScript Error: "setting a property that has only a getter" {file: "chrome://messenger/content/jsTreeView.js" line: 158}]' when calling method: [nsITreeView::toggleOpenState]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/bindings/tree.xml :: changeOpenState :: line 222"  data: yes]

We seem to have lost the different icons for remote (ldap) address books, all the ABs have the standard local address book icon.

Attempting to drag a card onto an AB and drop gives this exception:

[Exception... "'[JavaScript Error: "dirTree.builderView.getResourceAtIndex is not a function" {file: "chrome://messenger/content/addressbook/abDragDrop.js" line: 122}]' when calling method: [nsITreeView::canDrop]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]

+    var selected = directoryTreeView.getDirectoryForIndex(dirTree.currentIndex);
+    return selected.URI;

nit: no point in having the temporary variable here.

-  var dirTree = GetDirTree();
-  dirTree.addEventListener("click",DirPaneClick,true);
+  abManager.addAddressBookListener(directoryTreeView, nsIAbListener.all);

The directoryTreeView should register itself with the abManager. There should also be a separate remove call when it goes away.

+  _level: 0,
+  get level() {
+    return this._level;
+  },

I don't understand this, level is always 0, but we have mailing lists, shouldn't they be level 1?

+      var abDirectory = Components.interfaces.nsIAbDirectory;
...
+    var Ci = Components.interfaces;
+      this._rowMap.push(new abDirTreeItem(dirEnum.getNext().QueryInterface(Ci.nsIAbDirectory)));

I'd prefer these to be consistent.

+  onItemAdded: function dtv_onItemAdded(aParent, aItem) {
+  onItemRemoved: function dtv_onItemAdded(aParent, aItem) {
+  onItemPropertyChanged: function dtv_onItemAdded(aItem, aProp, aOld, aNew) {

Copy and paste error I think...

\ No newline at end of file

nit: please fix this.

Comment 3

[[e-mail]]

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

Note: I've not actually read the bug, or tested the code, or anything!

>   //workaround - add setTimeout to make sure dynamic overlays get loaded first
>-  setTimeout('OnLoadDirTree()', 0);
>+  setTimeout(SelectFirstAddressBook, 0);
If the timeout really was needed that must have been a really old bug.

>-  var dirTree           = GetDirTree();
>+  var dirTree           = document.getElementById("dirTree");
Out of interest, what does this buy you?

>+  _open: true,
>+  get open() {
>+    return this._open;
>+  },
I don't see any way to set the open state (called by toggleOpenState).
For a field as simple as this, consider writing it as a field, instead of wrapping it.

>+  _level: 0,
>+  get level() {
>+    return this._level;
>+  },
Isn't the level supposed to be set somewhere?

>+    aProps.AppendElement(atomSvc.getAtom("DirCol"));
This is uncessary; the column id is automatically added to the properties.

>+  _rebuild: function dtv__rebuild() {
>+    var oldCount = this._rowMap.length;
...
>+    if (this._tree)
>+      this._tree.rowCountChanged(0, this._rowMap.length - oldCount);
This doesn't really handle the selection correctly.

>+  onItemPropertyChanged: function dtv_onItemAdded(aItem, aProp, aOld, aNew) {
>+    if (!(aItem instanceof Components.interfaces.nsIAbDirectory))
>+      return;
>+
>+    for (var i in this._rowMap)  {
>+      if (this._rowMap[i].id == aItem.URI)
You should be able to compare its _directory against aItem.
(It's a pity you can't use indexOf here though.)

>+        this._tree.invalidateRow(i);
break; ?

>+            onclick="DirPaneClick(event);"
Weird, I wonder what this is for - updating the quick search if the selection changes? (In which case it really ought to be done off the select event...)

>+  getCellProperties: function ftv_getCellProperties(aRow, aCol, aProps) {
>+    var atomSvc = Components.classes["@mozilla.org/atom-service;1"]
>+                            .getService(Components.interfaces.nsIAtomService);
Unused variable!

>+  /**
>+   * This is easy since the ftv items assigned the _parent property when making
>+   * the child lists
>+   */
>+  getParentIndex: function ftv_getParentIndex(aIndex) {
>+    for (let i = 0; i < this._rowMap.length; i++) {
>+      if (this._rowMap[i] == this._rowMap[aIndex]._parent)
>+        return i;
>+    }
>+    return -1;
return this._rowMap.indexOf(this._rowMap[aIndex]._parent);

>+  /**
>+   * If the next item in our list has the same level as us, it's a sibling
>+   */
>+  hasNextSibling: function ftv_hasNextSibling(aIndex, aNextIndex) {
>+    return this._rowMap[aIndex].level == this._rowMap[aNextIndex].level;
>+  },
This is wrong, but you probably get away with it in addressbook because you only have two levels.

>+  /**
>+   * If we have a child-list with at least one element, we are a containe.  This
>+   * means that we don't display empty containers as containers.
>+   */
We already have code so that we don't display empty containers as containers.

>+        this._tree.rowCountChanged(aIndex + 1, (-1) * count);
I hope you've heard of the unary minus operator.

>+    } else {
>+      // We're opening the container.  Add the children to our map
Might want to double-check that the container isn't empty?

>+          for (let [i, child] in Iterator(tree._rowMap[aNewIndex].children)) {
Where does Iterator come from?

>+            let index = Number(aNewIndex) + Number(i) + 1;
Aren't these numbers already?

>+      // Notify the tree of changes
>+      if (this._tree)
>+        this._tree.rowCountChanged(aIndex, this._rowMap.length - oldCount);
This is incorrect, because the new rows are being inserted after the current row (ironically you got it right for removing rows).

>+  _restoreOpenStates: function ftv__persistOpenStates() {
>+    if (!(this.mode in this._persistOpenMap))
>+      return;
>+
>+    let curLevel = 0;
>+    let tree = this;
>+    function openLevel() {
>+      var goOn = false;
>+      for (let [i, row] in Iterator(tree._rowMap)) {
>+        if (row.level != curLevel)
>+          continue;
>+
>+        let map = tree._persistOpenMap[tree.mode];
>+        if (map && map.indexOf(row.id) != -1) {
>+          tree.toggleOpenState(i);
>+          goOn = true;
>+        }
>+      }
>+
>+      // If we opened up any new kids, we need to check their level as well.
>+      curLevel++;
>+      if (goOn)
>+          openLevel();
>+    }
>+    openLevel();
>+  },
I assume here your Iterator solution is inefficient because it only iterates over the items that existed before you started opening. If instead you simply start from 0 to length (note that length may of course increase as you go) then you can't avoid opening all the items.

>+  _rebuild: function ftw__rebuild() {
>+    throw "You must override the _rebuild method of a PROTO_TREE_VIEW!";
>+  }
Hardly seems worth defining - after all you never actually call it from here. It would be different if this file expected a definition.

>\ No newline at end of file
D'oh!

Should this file be a .jsm?

Comment 4

[Joey Minta]

Attachment: patch v2

This patch addresses most of the review comments by Mark and Neil.  We're currently now running into a problem where mailing-list URIs are not initialized in time, which Mark is working on.  A couple responses to Neil's comments:

(In reply to comment #3)
> >-  var dirTree           = GetDirTree();
> >+  var dirTree           = document.getElementById("dirTree");
> Out of interest, what does this buy you?
2 less items in the global scope.

> 
> >+  _open: true,
> >+  get open() {
> >+    return this._open;
> >+  },
> I don't see any way to set the open state (called by toggleOpenState).
> For a field as simple as this, consider writing it as a field, instead of
> wrapping it.
My concern here was that callers might think that toggling this value was enough to make the tree row open itself.  I've changed the call in toggleOpenState to reach in and set the _open variable instead.


> 
> >+  _rebuild: function dtv__rebuild() {
> >+    var oldCount = this._rowMap.length;
> ...
> >+    if (this._tree)
> >+      this._tree.rowCountChanged(0, this._rowMap.length - oldCount);
> This doesn't really handle the selection correctly.
I've punted this responsibility to callers at the moment.

> >+  /**
> >+   * If the next item in our list has the same level as us, it's a sibling
> >+   */
> >+  hasNextSibling: function ftv_hasNextSibling(aIndex, aNextIndex) {
> >+    return this._rowMap[aIndex].level == this._rowMap[aNextIndex].level;
> >+  },
> This is wrong, but you probably get away with it in addressbook because you
> only have two levels.
I don't understand what this function is supposed to return, then. FWIW, this pattern worked for a folder-tree with multiple levels, though I'm not sure if the function itself was ever actually called.

> Should this file be a .jsm?
> 
I thought about that, and originally filed bug 421913 to do just that.  On further reflection though, I don't anticipate anyone who's not chrome:// needing to use PROTO_TREE_VIEW, so having it in a jsm seems superfluous, when people can just use <script/>

Comment 5

[Joey Minta]

Attachment: patch v3

Now that the mailing-list stuff is sorted out, this should be good to go.  This patch fixes the hasNextSibling bit that Neil explained to me, and switches to using the now available nsIAbManager::directories.

Comment 6

[[e-mail]]

Comment on attachment 314684 [details] [diff] [review]
patch v3

Despite all these followup comments, I still feel I've overlooked something!

>+var dirTree;

>-var gDirTree;
So, um, what's the difference between dirTree and gDirTree?

>+    var targetURI = directoryTreeView._rowMap[index]._directory.URI;
This looks ugly. Why not use .getDirectoryAtIndex?

>+  this._initialized = false;
>+  this._open = false;
These are already false in the prototype.

>+abDirTreeItem.prototype = {
>+  getText: function atv_getText() {
>+    return this._directory.dirName;
>+  },
>+  get id() {
>+    return this._directory.URI;
>+  },
Blank line before get id() ?

>+  _initialized: false,
>+  _children: [],
As you may have noticed, setting things to arrays in prototypes doesn't work very well. As it happens, you can avoid the problem by setting _children to null to indicate uninitialised and then set it to an array in the getter.

>+      const Ci = Components.interfaces;
>+      var myEnum = this._directory.childNodes;
>+      while (myEnum.hasMoreElements()) {
>+        this._children.push(new abDirTreeItem(myEnum.getNext().QueryInterface(Ci.nsIAbDirectory)));
>+        this._children[this._children.length - 1]._level = this._level + 1;
>+        this._children[this._children.length - 1]._parent = this;
So, you went to the trouble of abbreviating Components.interfaces as Ci but you couldn't put the new abDirTreeItem in its own local?

>+        return a._directory.dirName.toLowerCase() > b._directory.dirName.toLowerCase();
IIRC .localeCompare is better for this, it's both case and accent insensitive.

>+  getDirectoryForIndex: function dtv_getDirForIndex(aIndex) {
IIRC At is more popular than For when indexing.

>+    var Cc = Components.classes;
>+    var Ci = Components.interfaces;
These were const before.

>+      return a._directory.dirName > b._directory.dirName;
You insensitive fool!

>+    // Now select this parent item
This only works for mailing lists, not address books.

>+  getCellText: function ftv_getCellText(aRow, aCol) {
>+      return this._rowMap[aRow].getText(aCol.id);
Wrong indent.

>+  /**
>+   * If an item in our list has the same level and parent as us, it's a sibling
>+   */
>+  hasNextSibling: function ftv_hasNextSibling(aIndex, aNextIndex) {
>+    var targetLevel = this._rowMap[aIndex].level;
>+    for (var i = aIndex + 1; i < this._rowMap.length; i++) {
Start at aNextIndex + 1; it's guaranteed to be a descendant of aIndex.

>+    this._rowMap[aIndex]._open = !this._rowMap[aIndex].open;
>+
>+    if (!this._rowMap[aIndex].open) {
This looks odd. Maybe toggling afterwards instead would be more readable?

>+      let count = 0;
>+      let i = 2;
>+      let row = this._rowMap[aIndex + 1];
Confusingly inconsistent use of let and var.

>+      while (row && row.level > this._rowMap[aIndex].level) {
>+        count++;
>+        row = this._rowMap[aIndex + i++];
This will generate a JS strict warning if you walk off the end of the array.
How about:
var level = this._rowMap[aIndex].level;
var row = aIndex + 1;
while (row < this._rowMap.length && this._rowMap[row].level > level)
  row++;
var count = row - aIndex - 1;

>+            aNewIndex += recursivelyAddToMap(child, index);
Except this only returns the count of immediate children, not of all the descendants (of course in address book you don't have any grandchildren).

It seems to me that adding the children in individually is inefficient but I guess that's a limitation of this implementation.

>+  _persistOpenMap: [],
Except you null-check this, so it should default to null, no?

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 314684 [details] [diff] [review]
patch v3

Neil's comments may address these, but anyway, I've just tried running this and I'm getting errors like:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this._rowMap[aIndex] is undefined" {file: "chrome://messenger/content/jsTreeView.js" line: 84}]' when calling method: [nsITreeView::getLevel]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************
WARNING: row count changed unexpectedly: 'mRowCount == rowCount', file /Users/moztest/mozilla/main/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp, line 2798

When selecting different ABs.

+      // Collected addressbook goes next

Actually it goes last, but I'm just about to post to m.d.a.thunderbird about that.

Looking better though.

Comment 8

[Joey Minta]

Attachment: patch v4

Comment 9

[Joey Minta]

Comment on attachment 322528 [details] [diff] [review]
patch v4

This is an updated patch which should address neil's comments.  I'm hoping that the toggling tweaks will fix standard8's errors, since I don't have clear steps to reproduce.

To answer the question about dirTree and gDirTree, they're exactly the same thing, I've just eliminated one of them. See http://mxr.mozilla.org/mailnews/source/mail/components/addrbook/content/abCommon.js#325
http://mxr.mozilla.org/mailnews/source/mail/components/addrbook/content/addressbook.js#829

Also, I've kept the open-state toggling as the initial line in that function, because other parts of the logic rely on that already being set.  Trying to refactor seemed like a lot of work, especially given the comments explaining the blocks. I hope that's ok.

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #9)
> (From update of attachment 322528 [details] [diff] [review])
> This is an updated patch which should address neil's comments.  I'm hoping that
> the toggling tweaks will fix standard8's errors, since I don't have clear steps
> to reproduce.

This seems to be improved, however I've got some problems still:

1) Have several address books created with mailing lists
2) Expand the directory pane so that mailing lists are visible.
3) Create a addressbook
-> Mailing lists collapse, errors like:
JavaScript Error: "this._toggleOpenState is not a function" {file: "chrome://messenger/content/jsTreeView.js" line: 253}]
'[JavaScript Error: "this._rowMap[aIndex] is undefined" {file: "chrome://messenger/content/jsTreeView.js

Similar thing happens for deleting address books, and creating and deleting mailing lists. Once its in this state it isn't happy.

Also the state of the items being collapsed/expanded isn't saved, but we probably know about that.

This patch also seems to break drag and drop. I can't drag an address book card from the results pane onto an address book in the tree.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 322528 [details] [diff] [review]
patch v4

+    // Sort our addressbooks now
+    function abSort(a, b) {

So I don't think I'm in a position to change this just yet, so I think this function should support what we actually do currently:

PAB
Other Mork address books in name order
LDAP address books in name order
MAPI & any other of that type in name order
CAB

+var directoryTreeView = new directoryTree();

as this is a global, lets at least give people an idea that it is a global and call it gDirectoryTreeView.

Comment 12

[Mark Banner (:standard8)]

I also forgot to mention that (if it was working) this patch would break drag and drop on SeaMonkey. Either we need to work around that, do a SeaMonkey port or get a SeaMonkey port done for when this patch goes into the code base.

Comment 13

[[e-mail]]

Comment on attachment 322528 [details] [diff] [review]
patch v4

>+    var targetURI = directoryTreeView._rowMap[row]._directory.URI;
Missed one!

>+  this._children = [];
No point setting this here, since get children() overwrites it. This also allows you to replace if (this._initialized) with if (!this._children)

>+        var dir = myEnum.getNext().QueryInterface(Ci.nsIAbDirectory);
>+        this._children.push(new abDirTreeItem(dir));
>+        this._children[this._children.length - 1]._level = this._level + 1;
>+        this._children[this._children.length - 1]._parent = this;
When I said "the new abDirTreeItem" I did mean that, and not the dir.

>+        return a._directory.dirName.localeCompare(b._directory.dirName.toLowerCase());
Don't need .toLowerCase with localeCompare

>+      // Collected addressbook goes next
last?

>+      // Then MAPI
Or Windows or OSX!

>+    // Now select this new item
What happens if someone else adds (deletes) an address book?

>+      if (this._rowMap[i].id == aItem.URI) {
Why not just compare _directory with aItem?

>+     return this._rowMap[aRow].getText(aCol.id);
Only 50% better ;-)

>+            aNewIndex += recursivelyAddToMap(child, index);
Maybe recursively adding them in reverse order would be more efficient (or you could add all the children at once and expand them in reverse order)

>+   * This is a javaascript map of which folders we had open, so that we can
Typo!

(In reply to comment #9)
>Also, I've kept the open-state toggling as the initial line in that function,
>because other parts of the logic rely on that already being set.
I don't see where.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: patch v5

The major issues I dealt with here were:

1)  Updated the patch for the 2011 state of the tree
2)  Drag and drop has been restored
3)  A buggy state on address book creation/deletion has been resolved
4)  Address books are now sorted by PAB, Mork, LDAP, MAPI and other, CAB - and then alphabetically within those groups.

All feedback welcome.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: patch v6

I forgot to clean up a few things, test-wise.

Regarding tests - I spent a bunch of time trying to write nice Mozmill tests to check on the behaviour of the new tree view.  Unfortunately, Mozmill didn't seem to like working with the address book tree - Mozmill recorder didn't produce anything useful when clicking on various tree items, etc.

So I ended up writing a few functions in test-address-book-helpers.js that I didn't end up using - but I think they might still be useful, so I left them.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 521805 [details] [diff] [review]
patch v6

When I tested the patch I noticed that it looks like all displayed
list items have the same icon - LDAP and mailing lists are both wrong.

When trying to drag drop a card to a mailing list I see:

JavaScript strict warning: chrome://global/content/bindings/tree.xml, line 1020: reference to undefined property b.view
--DOMWINDOW == 23 (0x11b124da8) [serial = 36] [outer = 0x0] [url = chrome://messenger/content/addressbook/abEditCardDialog.xul]
JavaScript strict warning: chrome://messenger/content/jsTreeView.js, line 96: reference to undefined property this._rowMap[aIndex]._parent

Dragging and dropping across address books seems to work, but there's
also various warnings displayed.

Creating a list in a new address book seems to default to being
closed, and doesn't select the new address book.

Deleting an address book with mailing lists doesn't seem to select any
address books, and what was in the contact pane remains.

diff -r ef50f461a021 -r 6a809e5fc075 .hgpatchinfo/rdf_ab_clean.dep
diff -r ef50f461a021 -r 6a809e5fc075 .hgpatchinfo/rdf_ab_clean_tests.dep

I assume these are part of your patch tree and won't get checked in.

diff -r ef50f461a021 -r 6a809e5fc075 mail/base/content/jsTreeView.js

Ok, so most of this got used in folderPane.js. I'm currently a bit
torn getting that to re-use this now or later. I'm also thinking that
the folderPane.js one may have had some enhancements already.

+  _restoreOpenStates: function ftv__persistOpenStates() {

In any case, I'm not sure this version should keep the 'ftv' prefixes,
as it isn't the folder tree view.

diff -r ef50f461a021 -r 6a809e5fc075 mail/components/addrbook/content/abTrees.js
+    var abMgr = Cc["@mozilla.org/abmanager;1"].getService(Ci.nsIAbManager);
+    var dirEnum = abMgr.directories;

Nit: We can do away with the abMgr intermediate variable here.

+    function getDirectoryValue(aDir, aKey) {
...
+        if (aDir._directory.URI == "moz-abmdbdirectory://abook.mab")

nit: use kPersonalAddressBookURI from abCommon.js

+        if (aDir._directory.URI == "moz-abmdbdirectory://history.mab")

nit: use kCollectedAddressbookURI from abCommon.js

+        if (aDir._directory.dirType == 2)

So, iirc I was starting to prefer doing:

aDir._directory instanceof Ci.nsIAbMDBDirectory

because that meant we might be able to get rid of the dirType things (and similar
for LDAP). Although the real solution is probably to fix address book
up properly so that we don't need books to sort ;-)

+  onItemRemoved: function dtv_onItemRemoved(aParent, aItem) {
+  onItemPropertyChanged: function dtv_onItemProp(aItem, aProp, aOld, aNew) {

nit: blank lines before these functions please.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: patch v7

> When I tested the patch I noticed that it looks like all displayed
> list items have the same icon - LDAP and mailing lists are both wrong.

Yep - a major oversight on my part.  Fixed.

> JavaScript strict warning: chrome://global/content/bindings/tree.xml, line
> 1020: reference to undefined property b.view
> --DOMWINDOW == 23 (0x11b124da8) [serial = 36] [outer = 0x0] [url =
> chrome://messenger/content/addressbook/abEditCardDialog.xul]
> JavaScript strict warning: chrome://messenger/content/jsTreeView.js, line 96:
> reference to undefined property this._rowMap[aIndex]._parent

> Dragging and dropping across address books seems to work, but there's
> also various warnings displayed.

I wasn't able to reproduce these warnings / errors on my machine.  Can anyone else confirm?

> Deleting an address book with mailing lists doesn't seem to select any
> address books, and what was in the contact pane remains.

Hm - I also can't reproduce this one.  For me, after deleting an address book with a mailing list, the address book with the next lower index is selected, the first contact in that address book is selected, and the contact page updates.  Can anyone else confirm?

> I assume these are part of your patch tree and won't get checked in.

Yep - that's freaky pbranch stuff.  I've cleaned it out of this revision.

> Ok, so most of this got used in folderPane.js. I'm currently a bit
> torn getting that to re-use this now or later. I'm also thinking that
> the folderPane.js one may have had some enhancements already.

Perhaps we should refactor jsTreeView.js and folderPane.js, and update jsTreeView.js with folderPane's goodness.  Probably out of scope for this bug.

> In any case, I'm not sure this version should keep the 'ftv' prefixes,
> as it isn't the folder tree view.

Agreed - the prefixes have been updated.

And the other nits have been (hopefully) taken care of.

Let me know if you need anything further.

Comment 18

[Mark Banner (:standard8)]

Attachment: Patch v7a

Small update to the patch - this copies the changes that folderPane.js has for the getParentIndex function and as a result fixes one of the warnings I was seeing.

The rest of the warnings appear to be a result of two things: a) Nothing is telling the js tree view what the actual tree element is, b) Nothing then tells the tree element what the tree view is (e.g. look for _tree and/or setTree in your patch and then compare with how folderPane.js works.

Additionally, I've also noticed we're not persisting the open/collapsed values like the folderPane does. You'll probably need to copy the load/unload code from folderPane.js for this.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

> The rest of the warnings appear to be a result of two things: 
> a) Nothing is telling the js tree view what the actual tree element is
> b) Nothing then tells the tree element what the tree view is 
> (e.g. look for _tree and/or setTree in your patch and then compare with how 
> folderPane.js works.

Unless I'm mistaken, I think both (a) and (b) are taken care of with line 193 of addressbook.js:

> document.getElementById("dirTree").view = gDirectoryTreeView;

Using Chromebug, I've noticed that this statement seems to do a lot automatically - including calling setTree on gDirectoryTreeView.

This seems to be similar to what happens in folderDisplay.js (see the comments here:  http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js#1606)

Or have I misunderstood something here?

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v8

Thanks for the review, Mark!

We now persist collapse/expanded address book states.  I've also added a few tests for persisting state, as well as ordering for address books.

Comment 21

[David :Bienvenu]

Comment on attachment 528131 [details] [diff] [review]
Patch v8

this patch seems to be at least partially a diff against the prev patch, not against the trunk. I'm hoping to apply this so I can test the de-rdf backend patch...

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v9

Whoops - you're absolutely right.  This new patch should be properly based off of trunk.

Comment 23

[David :Bienvenu]

I'm not going to review this in full until there's a new patch for bug 652855 so I can test them together, but one quick comment - there are comments that refer to folders and ftvItems in jsFolderTree.js that should be changed...

Comment 24

[Mike Conley (:mconley) (:⚙️)]

To make things easier, would you prefer that I integrate both patches into bug 652855 as a single diff, and close this one?

Comment 25

[David :Bienvenu]

(In reply to comment #24)
> To make things easier, would you prefer that I integrate both patches into bug
> 652855 as a single diff, and close this one?

No, I think it's OK to have two bugs, especially since this patch doesn't require the other bug.

Comment 26

[Mark Banner (:standard8)]

I'm quite happy to review as-is (especially as I'd already done some of it). I just hadn't finished it off last week because the patch didn't apply and I was afk for most of it.

Comment 27

[Mark Banner (:standard8)]

Comment on attachment 529464 [details] [diff] [review]
Patch v9

Review of attachment 529464 [details] [diff] [review]:

I've not reviewed the tests yet, but here's some comments on the code. There's the warning I mentioned on irc to fix as well.

::: mail/base/content/jsTreeView.js
@@ +1,1 @@
+/* ***** BEGIN LICENSE BLOCK *****

I think this is a generic enough file that we can just put it in mailnews/base/content to begin with.

@@ +78,5 @@
+    return this._rowMap[aRow].getText(aCol.id);
+  },
+
+  /**
+   * The ftvItems take care of assigning this when building children lists

Reference to ftv isn't quite right here.

@@ +85,5 @@
+    return this._rowMap[aIndex].level;
+  },
+
+  /**
+   * This is easy since the ftv items assigned the _parent property when making

ftv again

@@ +97,5 @@
+    return -1;
+  },
+
+  /**
+   * This is duplicative for our normal ftv views, but custom data-displays may

ftv -> jstv

@@ +131,5 @@
+    return !this._rowMap[aIndex].children.length;
+  },
+
+  /**
+   * Just look at the ftvItem here

ftvItem referenced again

@@ +211,5 @@
+        return tree._rowMap.length - currentCount;
+      }
+      recursivelyAddToMap(this._rowMap[aIndex], aIndex);
+
+      // Add this folder to the persist map

Should probably reference item or something here (Rather than folder)

::: mail/components/addrbook/content/abCommon.js
@@ +25,4 @@
 #   Seth Spitzer <[e-mail]>
 #   Mark Banner <[e-mail]>
 #   Simon Wilkinson <[e-mail]>
+#   Mike Conley <[e-mail]>

You might want to update this now!

::: mail/components/addrbook/content/abTrees.js
@@ +39,5 @@
+ * depends on jsTreeView.js being loaded before this script is loaded.
+ */
+
+let Cc = Components.classes;
+let Ci = Components.interfaces;

These cause issues with extensions and other things because they aren't globally defined (there's a separate bug on this). Additionally you'll find through the file that they are defined where needed. So I think I'd rather you leave these out at the moment.

@@ +191,5 @@
+    const Cc = Components.classes;
+    const Ci = Components.interfaces;
+
+    var dirEnum = Cc["@mozilla.org/abmanager;1"]
+                  .getService(Ci.nsIAbManager).directories;

Can probably be replaced by a call to MailServices now

@@ +212,5 @@
+          return "mork";
+        if (aDir._directory instanceof Ci.nsIAbLDAPDirectory)
+          return "ldap";
+        else
+          return "mapi+other";

No else needed after the return.

::: mail/components/addrbook/content/addressbook.js
@@ +127,4 @@
 function OnUnloadAddressBook()
 {
+  var abManager = Components.classes["@mozilla.org/abmanager;1"]
+                 .getService(Components.interfaces.nsIAbManager);

Let's use MailServices.ab here.

@@ +209,5 @@
   // directory item is/are removed. In the case of directory items, we are
   // only really interested in mailing list changes and not cards but we have
   // to have both.
+  var abManager = Components.classes["@mozilla.org/abmanager;1"]
+                  .getService(Components.interfaces.nsIAbManager);

and MailServices.ab here.

Comment 28

[David :Bienvenu]

Comment on attachment 529464 [details] [diff] [review]
Patch v9

I think Standard8 wants to review this

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 529464 [details] [diff] [review]
Patch v9

Review of attachment 529464 [details] [diff] [review]:

Ok, I've taken a look at the test and there's a few comments I've made. I've not actually run the tests yet.

Regarding the treeview errors - I think we're actually getting those with the 3-pane window as well, so I'm guessing there's something wrong or regressed in the code somewhere. Therefore I'm not going to block landing on those, but please can you make sure there's a bug covering both.

Apart from that, I think you just need to update the patch for the this set of comments and the previous set, and then that should be more or less it.

::: mail/test/mozmill/addrbook/test-address-book.js
@@ +13,5 @@
+ *
+ * The Original Code is Thunderbird Mail Client.
+ *
+ * The Initial Developer of the Original Code is
+ * the Mozilla Messaging, Inc.

Should be "the Mozilla Foundation."

@@ +38,5 @@
+/*
+ * Tests for the address book.
+ */
+
+let Cu = Components.utils;

Isn't this already defined in the mozmill stuff? In any case, I'm not sure you're actually using it.

@@ +85,5 @@
+}
+
+function setupTest(test)
+{
+}

Remove the empty function?

::: mail/test/mozmill/shared-modules/test-address-book-helpers.js
@@ +35,4 @@
  *
  * ***** END LICENSE BLOCK ***** */
 
+let Ci = Components.interfaces;

General comment: we normally use var for global preferences because let doesn't actually have any effect (over var) at global level.

@@ +57,2 @@
   // Ensure all the directories are initialised.
+  MailServices.ab.directories

Missing semi-colon.

@@ +58,5 @@
+  MailServices.ab.directories
+  collectedAddresses = MailServices.ab
+                       .getDirectory("moz-abmdbdirectory://history.mab");
+  prefs = Cc["@mozilla.org/preferences-service;1"]
+          .getService(Ci.nsIPrefService);

We should be able to use Services.prefs here (or rather, just replace the global var with Services.prefs).

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v10 [Checked in: Comment 32+33]

Thanks for the review, Standard8.  I've implemented your suggestions, and the address book tests still pass.

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 530149 [details] [diff] [review]
Patch v10
[Checked in: Comment 32+33]

Review of attachment 530149 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, so apart from forgetting to add the new test directory to mozmilltests.list, this looks fine (and the test does run). I'll do the addition on checkin.

Comment 32

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/d5ac0eb39141

Comment 33

[Mark Banner (:standard8)]

Checked in a small MozMill bustage fix as well:

http://hg.mozilla.org/comm-central/rev/6e2ec369d646

Comment 34

[[e-mail]]

Comment on attachment 530149 [details] [diff] [review]
Patch v10
[Checked in: Comment 32+33]

>+        this._children.push(abItem);
>+        this._children[this._children.length - 1]._level = this._level + 1;
>+        this._children[this._children.length - 1]._parent = this;
You should set _level and _parent directly on abItem rather than fiddling around with _children[].

>+      // Parse our persistent-open-state json file
I'm not particularly keen on the idea of saving state in a JSON file but that's how session store does it and its code is no nicer...

>+        let JSON = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON);
Unnecessary, JSON is now a global.

>+    // Sort our addressbooks now
I'm not sure what this code is doing but maybe the code in addrbookWidgets.xml could be at least copied...

>+    // Now select this new item
I take it we're assuming here that if the address book is open then it's the cause of all address book creations and deletions?

>+    for (var [i, row] in Iterator(this._rowMap)) {
>+      if (row.id == aItem.URI) {
Might it be worth having a helper method to find the index of a directory?

>+    for (var i in this._rowMap)  {
>+      if (this._rowMap[i]._directory == aItem) {
That would at least avoid having slightly different code for no reason ;-)

>+      // Note that we can't simply splice out children.length, because some of
>+      // them might have children too.  Find out how many items we're actually
>+      // going to splice
Keeping the open grandchildren as members of the children as well would make toggling both ways easier, not that it affects the address book.

Comment 35

[[e-mail]]

(In reply to comment #34)
> Keeping the open grandchildren as members of the children as well would make
> toggling both ways easier, not that it affects the address book.
Although now that I think about it, it would make opening harder.

Comment 36

[[e-mail]]

(In reply to comment #16)
> So, iirc I was starting to prefer doing:
> 
> aDir._directory instanceof Ci.nsIAbMDBDirectory
> 
> because that meant we might be able to get rid of the dirType things
OK, so that explains why, although it does make the code much uglier :-(

Comment 37

[[e-mail]]

(In reply to comment #17)
> Perhaps we should refactor jsTreeView.js and folderPane.js, and update
> jsTreeView.js with folderPane's goodness.
From what I saw, jsTreeView.js had more goodness than folderPane.js ;-)

Comment 38

[[e-mail]]

Just wanted to highlight some of my earlier comments...

(In reply to comment #3)
> >+    for (var i in this._rowMap)  {
> >+      if (this._rowMap[i].id == aItem.URI)
> You should be able to compare its _directory against aItem.
> (It's a pity you can't use indexOf here though.)

> >+    for (let i = 0; i < this._rowMap.length; i++) {
> >+      if (this._rowMap[i] == this._rowMap[aIndex]._parent)
> >+        return i;
> >+    }
> >+    return -1;
> return this._rowMap.indexOf(this._rowMap[aIndex]._parent);

(In reply to comment #13)
> >+    // Now select this new item
> What happens if someone else adds (deletes) an address book?

> >+      if (this._rowMap[i].id == aItem.URI) {
> Why not just compare _directory with aItem?

Comment 39

[Ian Neal]

(In reply to comment #38)
> Just wanted to highlight some of my earlier comments...
> 
> (In reply to comment #3)
> > >+    for (var i in this._rowMap)  {
> > >+      if (this._rowMap[i].id == aItem.URI)
> > You should be able to compare its _directory against aItem.
> > (It's a pity you can't use indexOf here though.)
This was fixed (in patch v5).
> 
> > >+    for (let i = 0; i < this._rowMap.length; i++) {
> > >+      if (this._rowMap[i] == this._rowMap[aIndex]._parent)
> > >+        return i;
> > >+    }
> > >+    return -1;
> > return this._rowMap.indexOf(this._rowMap[aIndex]._parent);
See bug 663631
> 
> (In reply to comment #13)
> > >+    // Now select this new item
> > What happens if someone else adds (deletes) an address book?
Mike?

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Hey - whoops, didn't even realize a conversation was still happening in here.  I don't think I was getting Bugzilla mail on it, which is strange.

> What happens if someone else adds (deletes) an address book?

Hm - I think I need some clarification here.  Could you give me an example of "someone else"?  I need some help seeing this scenario that you're asking about.

Comment 41

[[e-mail]]

(In reply to comment #16)
>>+        if (aDir._directory.dirType == 2)
> 
>So, iirc I was starting to prefer doing:
> 
>aDir._directory instanceof Ci.nsIAbMDBDirectory
> 
>because that meant we might be able to get rid of the dirType things (and
>similar for LDAP). Although the real solution is probably to fix address book
>up properly so that we don't need books to sort ;-)
Would you accept a bug to add the equivalent of the compareSortKeys method?

Comment 42

[[e-mail]]

(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #3)
> > > >+    for (var i in this._rowMap)  {
> > > >+      if (this._rowMap[i].id == aItem.URI)
> > > You should be able to compare its _directory against aItem.
> > > (It's a pity you can't use indexOf here though.)
> This was fixed (in patch v5).
Although only the one cae that I commented on; there are two other cases of .id == .URI that weren't fixed.

It occurs to me that almost the same code was repeated three times, and this really belongs in a helper method called e.g. getIndexOfDirectory.

Comment 43

[[e-mail]]

(In reply to comment #40)
> > What happens if someone else adds (deletes) an address book?
> Hm - I think I need some clarification here.  Could you give me an example
> of "someone else"?  I need some help seeing this scenario that you're asking
> about.
LDAP address books can be added and removed from the Edit Directories dialog, which (at least in SeaMonkey) can be opened from several windows, such as the Preferences window and the Account Settings window. In this case jminta's code would cause the address book window to change its selection depending on the actions in the apparently unrelated window. Users might find this surprising, especially if this causes the addressbook to prompt for an LDAP password.

Comment 44

[Mike Conley (:mconley) (:⚙️)]

> LDAP address books can be added and removed from the Edit Directories dialog,
> which (at least in SeaMonkey) can be opened from several windows, such as the  > Preferences window and the Account Settings window. In this case jminta's code > would cause the address book window to change its selection depending on the
> actions in the apparently unrelated window. Users might find this surprising,  > especially if this causes the addressbook to prompt for an LDAP password.

Ok, I gotcha.  Yep, that's an issue.

I've filed it as bug 669203.  Thanks for the heads up!