I don't want this to get lost, so from Bug 652855 - De-RDF the address book: (In reply to comment #28) > Should I create a new bug for making the changes to the suite/ equivalent of > abCommon.js, add the patch here (already have it locally) or leave it to you > guys? I'd suggest making the suite/ port separate - there's already a lot going on here. Original patch (attachment 529815 [details] [diff] [review]) fixed only mail/.

Extending scope to include porting bug 422845; will come up with a patch eventually.

(In reply to comment #1) > Extending scope to include porting bug 422845; will come up with a patch > eventually. I was going to poke you guys about this one. We'd like to land bug 652855 reasonably soon - given where we are, I'm currently aiming that we land that bug before the 24th May, i.e. the time of the next set of merges (assuming reviews are all done etc).

Comment on attachment 530179 [details] [diff] [review] patch [Checkin: comment 3] http://hg.mozilla.org/comm-central/rev/93ef8f9a8dc5 First part done, leaving open for second part.

Change summary: * abCommon.js - dirTree -> gDirTree - dirTree.builderView/GetDirectoryFromURI -> gDirectoryTreeView (cf. abTrees.js) * abTrees.js (new file) - code from TB, haven't checked what it does in detail; questions? -> TB guys - reformatted according to our MailNews coding guideline (bracing, Cc/Ci etc.) * addressbook.js - dirTree -> gDirTree - use of MailServices and gDirectoryTreeView * addressbook.xul - JS includes - seems we never had the RDF TB replaced in that file, do we inherit? :-/

Comment on attachment 530928 [details] [diff] [review] bug 422845 port Review of attachment 530928 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following nits addressed. ::: suite/mailnews/addrbook/abCommon.js @@ +45,1 @@ > var abList = 0; gDirTree and abList are objects, hence the initial value should be |null|. ::: suite/mailnews/addrbook/abTrees.js @@ +44,5 @@ > +/** > + * Each abDirTreeItem corresponds to one row in the tree view. > + */ > +function abDirTreeItem(aDirectory) { > + this._directory = aDirectory; When you corrected the bracing style in this file, you forgot this function. @@ +48,5 @@ > + this._directory = aDirectory; > +} > + > +abDirTreeItem.prototype = { > + getText: function atv_getText() { should go onto the next line here as well.

(Sorry, splinter's context handling isn't exactly optimal.)

(In reply to comment #5) > When you corrected the bracing style in this file, you forgot this function. Actually I found some more occurrences. Don't bother, I corrected all locally.

(In reply to comment #4) > bug 654864 port You mean port of bug 422845? I made some comments there...

(In reply to comment #8) > (In reply to comment #4) > > bug 654864 port > You mean port of bug 422845? I made some comments there... One fix now being done in bug 663631

In addition to Karsten's nits, I addressed some of Neil's comments from bug 422845. For the rest, please consider follow-ups, clarify, or allow me to let someone else take over. >+ 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[]. >+ let JSON = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON); Unnecessary, JSON is now a global. >+ 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? (...) 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.

Since the landing of bug 652855, the address book is empty and the error log shows: Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXULTreeBuilder.getResourceAtIndex]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://messenger/content/addressbook/abCommon.js :: GetSelectedDirectory :: line 631" data: no] Is this a known issue?

H. Hofer: Thanks for the bug report! Yes, I think the de-RDF patch broke the Seamonkey address book - but that's what this bug is all about. For this particular problem you found, "getResourceAtIndex" is not how we go about getting the selected directory anymore. See: http://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abCommon.js#616 -Mike

(In reply to comment #12) > Yes, I think the de-RDF patch broke the Seamonkey address book - but that's > what this bug is all about. I'm tentatively duping bug 669468 to here on the basis of the above sentence. If and when this bug gets fixed, I'll check on Linux64 that the AB works again in builds containing the fix, and Rocco (bug 669468's reporter) should do the same on the Mac. If it doesn't work, then one on the other should be REOPENED.

Bumping Severity to major because the address book is supposed to be broken "until this bug gets fixed".

Neil: ping for review Note bug 671251: Drag & drop is broken since SM 2.2, and AFAIK this would fix it.

Comment on attachment 540581 [details] [diff] [review] bug 422845 port v2 >+ if (row == -1 || row > gDirTree.view.rowCount-1) { Nit: as you're touching this, please use row >= gDirTree.view.rowCount Or possibly row >= gDirectoryTreeView.rowCount would be better still, but I don't want you to search and replace gDirTree.view unless you want to. > function GetSelectedDirectory() > { > if (abList) > return abList.value; > else { >- if (dirTree.currentIndex < 0) >+ if (gDirTree.currentIndex < 0) > return null; >- var selected = dirTree.builderView.getResourceAtIndex(dirTree.currentIndex) >- return selected.Value; >+ return gDirectoryTreeView.getDirectoryAtIndex(gDirTree.currentIndex).URI; > } > } [Bah, what a horrible mixture of styles this is...] >+ this._persistOpenMap = JSON.decode(data); JSON.parse(data); >+ let data = JSON.encode(this._persistOpenMap); JSON.stringify(data); >+ getIndexOfDirectory: function dtv_getIndexOfDir(aItem) >+ { >+ for (var i in this._rowMap) >+ if (this._rowMap[i]._directory == aItem) >+ return i; >+ }, Needs a return -1; >+ if (!aValue && !bValue) >+ return 0; [Not expecting to hit this case, but should be continue;] >+ // Now select this new item >+ var index = this.getIndexOfDirectory(aItem); >+ if (index) >+ this.selection.select(index); Ideally you would reselect the previously selected item. >+ var index = this.getIndexOfDirectory(aItem); >+ if (index) Doesn't work for the first directory (index == 0) >+ MailServices.ab.addAddressBookListener(gAddressBookAbListener, >+ nsIAbListener.directoryRemoved, >+ nsIAbListener.directoryItemRemoved); >+ MailServices.ab.addAddressBookListener(gDirectoryTreeView, nsIAbListener.all); I'm concerned that this might process deletes in the wrong order. One solution would be to improve the deletion code by moving the address book listener deletion code to the tree and removing the old listener.

(In reply to comment #17) > >+ if (row == -1 || row > gDirTree.view.rowCount-1) { > Nit: as you're touching this, please use row >= gDirTree.view.rowCount Please check again, I hope I got you correctly that only the part after the || should be changed. > Or possibly row >= gDirectoryTreeView.rowCount would be better still, but I > don't want you to search and replace gDirTree.view unless you want to. Well, /I/ am always in favor of cleaning up! :-) That said, since I'm no longer using dirTree/gDirTree there, I removed the checks. I assume gDirectoryTreeView will always be present in these cases. > > function GetSelectedDirectory() > > (...) > [Bah, what a horrible mixture of styles this is...] Which we can surely get rid of, no? I hope you don't mind. > >+ this._persistOpenMap = JSON.decode(data); > JSON.parse(data); Yeah sorry for not checking. > >+ let data = JSON.encode(this._persistOpenMap); > JSON.stringify(data); ITYM JSON.stringify(this._persistOpenMap) > >+ // Now select this new item > >+ var index = this.getIndexOfDirectory(aItem); > >+ if (index) > >+ this.selection.select(index); > Ideally you would reselect the previously selected item. Why? This only triggers (AFAICS) when creating a new address book. I think it's safe to assume that you want to operate on it, no? Why reselect the previous one? > >+ MailServices.ab.addAddressBookListener(gAddressBookAbListener, > >+ nsIAbListener.directoryRemoved, > >+ nsIAbListener.directoryItemRemoved); > >+ MailServices.ab.addAddressBookListener(gDirectoryTreeView, nsIAbListener.all); > I'm concerned that this might process deletes in the wrong order. One > solution would be to improve the deletion code by moving the address book > listener deletion code to the tree and removing the old listener. Hmm, sorry, I don't feel up to this. Partly because I don't even understand the problem, and partly because it sounds like a call to modify shared code. I think this either needs someone else to work on or to go to a follow-up if you permit.

(In reply to comment #18) > (From update of attachment 546833 [details] [diff] [review]) > > >+ // Now select this new item > > >+ var index = this.getIndexOfDirectory(aItem); > > >+ if (index) > > >+ this.selection.select(index); > > Ideally you would reselect the previously selected item. > Why? This only triggers (AFAICS) when creating a new address book. I think > it's safe to assume that you want to operate on it, no? Why reselect the > previous one? Because there's nothing to say that address books are created by the address book manager window. (I did mention this in the other bug...) > Hmm, sorry, I don't feel up to this. Partly because I don't even understand > the problem, and partly because it sounds like a call to modify shared code. > I think this either needs someone else to work on or to go to a follow-up if > you permit. It's not shared code, addressbook.js is forked. It has a listener that tries to fix things up after deleting the selected address book. Sadly it only knows how to do that when the address book was deleted by the manager window - just switching the first two if statements would have significantly improved matters. This patch adds a second listener, which also tries to fix things up after deleting an address book. The good news is that it works even if the address book was deleted by another window. The bad news is that its algorithm is otherwise worse. And the worst thing is that I don't know which one wins.

Comment on attachment 546833 [details] [diff] [review] bug 422845 port v2a [Checkin: comments 22 and 27] Interdiff looks OK. I want to improve the address book deletion handling code anyway, so I guess I can fold the "duplicate" code removal in at the same time.

Hmm, I lost track of this for a moment, only seeing the sr+ but forgetting that I already had r+. Unless there are objections, I'll check part v2a in later today as-is, i.e. without addressing comment 19 (reselecting the previously selected item upon AB addition; listeners).

Comment on attachment 546833 [details] [diff] [review] bug 422845 port v2a [Checkin: comments 22 and 27] http://hg.mozilla.org/comm-central/rev/3b77cabb3de9

TODO: * check whether this needs to land on Aurora/Beta, too * check which parts need to be backported in bug 671251 (for SM 2.3, hopefully) * for the selection issue, see what the TB guys come up with in bug 669203

(In reply to comment #23) > TODO: > * check whether this needs to land on Aurora/Beta, too Looks (from bug comments) like bug 422845 landed only on -central, but on 2011-05-06, which sounds like it was uplifted to aurora on -05-24 and to beta on -07-05, so should affect both of those as well.

Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110723 Firefox/8.0a1 SeaMonkey/2.5a1 ID:20110724160038 The address book is working again on this linux64 hourly built shortly after the fix. Rocco "quicksilver" Stilo: the next nightly should be up by now. Does the Mac version have a working address book too?

Comment on attachment 546833 [details] [diff] [review] bug 422845 port v2a [Checkin: comments 22 and 27] (In reply to comment #24) > (In reply to comment #23) > > TODO: > > * check whether this needs to land on Aurora/Beta, too > > Looks (from bug comments) like bug 422845 landed only on -central, but on > 2011-05-06, which sounds like it was uplifted to aurora on -05-24 and to > beta on -07-05, so should affect both of those as well. Right: http://hg.mozilla.org/releases/comm-beta/rev/d5ac0eb39141

Comment on attachment 546833 [details] [diff] [review] bug 422845 port v2a [Checkin: comments 22 and 27] http://hg.mozilla.org/releases/comm-aurora/rev/1aaea7153818 http://hg.mozilla.org/releases/comm-beta/rev/7d8ed9d00eb1