Last Comment Bug 654864 - Suite changes from |Bug 652855 - De-RDF the address book| and |Bug 422845 - Replace rdf-driven addressbook directory tree with js one|
: Suite changes from |Bug 652855 - De-RDF the address book| and |Bug 422845 - R...
Status: RESOLVED FIXED
: useless-UI
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Address Book & Contacts (show other bugs)
: Trunk
: All All
: -- major with 1 vote (vote)
: seamonkey2.5
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
:
Mentors:
: 669468 (view as bug list)
Depends on: 422845 663631
Blocks: 652855 657607 671251 677859
  Show dependency treegraph
 
Reported: 2011-05-04 15:33 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2014-04-24 18:20 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
patch [Checkin: comment 3] (2.80 KB, patch)
2011-05-04 15:33 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review+
neil: superreview+
Details | Diff | Splinter Review
bug 422845 port (26.75 KB, patch)
2011-05-08 10:20 PDT, Jens Hatlak (:InvisibleSmiley)
mnyromyr: review+
Details | Diff | Splinter Review
bug 422845 port v2 (26.67 KB, patch)
2011-06-20 13:34 PDT, Jens Hatlak (:InvisibleSmiley)
jh: review+
Details | Diff | Splinter Review
bug 422845 port v2a [Checkin: comments 22 and 27] (26.81 KB, patch)
2011-07-19 11:05 PDT, Jens Hatlak (:InvisibleSmiley)
jh: review+
neil: superreview+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2011-05-04 15:33:19 PDT
Created attachment 530179 [details] [diff] [review]
patch [Checkin: comment 3]

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/.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2011-05-06 04:39:00 PDT
Extending scope to include porting bug 422845; will come up with a patch eventually.
Comment 2 Mark Banner (:standard8) 2011-05-06 05:06:12 PDT
(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 3 Jens Hatlak (:InvisibleSmiley) 2011-05-08 07:01:26 PDT
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.
Comment 4 Jens Hatlak (:InvisibleSmiley) 2011-05-08 10:20:41 PDT
Created attachment 530928 [details] [diff] [review]
bug 422845 port

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 5 Karsten Düsterloh 2011-05-08 13:34:45 PDT
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.
Comment 6 Karsten Düsterloh 2011-05-08 13:37:05 PDT
(Sorry, splinter's context handling isn't exactly optimal.)
Comment 7 Jens Hatlak (:InvisibleSmiley) 2011-05-08 13:44:52 PDT
(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.
Comment 8 neil@parkwaycc.co.uk 2011-05-09 06:44:24 PDT
(In reply to comment #4)
> bug 654864 port
You mean port of bug 422845? I made some comments there...
Comment 9 Ian Neal 2011-06-11 08:44:49 PDT
(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
Comment 10 Jens Hatlak (:InvisibleSmiley) 2011-06-20 13:34:18 PDT
Created attachment 540581 [details] [diff] [review]
bug 422845 port v2

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.
Comment 11 H. Hofer 2011-07-05 05:54:37 PDT
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?
Comment 12 Mike Conley (:mconley) - (needinfo me!) 2011-07-05 06:11:31 PDT
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
Comment 13 Tony Mechelynck [:tonymec] 2011-07-07 23:54:39 PDT
*** Bug 669468 has been marked as a duplicate of this bug. ***
Comment 14 Tony Mechelynck [:tonymec] 2011-07-07 23:59:55 PDT
(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.
Comment 15 Tony Mechelynck [:tonymec] 2011-07-08 00:11:41 PDT
Bumping Severity to major because the address book is supposed to be broken "until this bug gets fixed".
Comment 16 Jens Hatlak (:InvisibleSmiley) 2011-07-13 10:26:48 PDT
Neil: ping for review

Note bug 671251: Drag & drop is broken since SM 2.2, and AFAIK this would fix it.
Comment 17 neil@parkwaycc.co.uk 2011-07-19 02:34:10 PDT
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.
Comment 18 Jens Hatlak (:InvisibleSmiley) 2011-07-19 11:05:50 PDT
Created attachment 546833 [details] [diff] [review]
bug 422845 port v2a [Checkin: comments 22 and 27]

(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.
Comment 19 neil@parkwaycc.co.uk 2011-07-19 16:52:22 PDT
(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 20 neil@parkwaycc.co.uk 2011-07-19 16:57:55 PDT
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.
Comment 21 Jens Hatlak (:InvisibleSmiley) 2011-07-24 03:10:01 PDT
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 22 Jens Hatlak (:InvisibleSmiley) 2011-07-24 15:58:25 PDT
Comment on attachment 546833 [details] [diff] [review]
bug 422845 port v2a [Checkin: comments 22 and 27]

http://hg.mozilla.org/comm-central/rev/3b77cabb3de9
Comment 23 Jens Hatlak (:InvisibleSmiley) 2011-07-24 16:03:57 PDT
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
Comment 24 Robert Kaiser 2011-07-25 05:29:31 PDT
(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.
Comment 25 Tony Mechelynck [:tonymec] 2011-07-25 08:01:45 PDT
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 26 Jens Hatlak (:InvisibleSmiley) 2011-07-25 10:19:08 PDT
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

Note You need to log in before you can comment on or make changes to this bug.