Closed Bug 73868 Opened 23 years ago Closed 23 years ago

addressbook results pane, select address dialog results pane, ab sidebar should use outliner

Categories

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Product:
SeaMonkey
Component:
MailNews: Address Book & Contacts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: cathleennscp, Assigned: sspitzer)

References

(
URL
)

Details

(Keywords: perf)

Attachments

(4 files, 44 obsolete files)

screen shot
63.80 KB, image/jpeg
Details
screen shot, all columns, icons in result pane
51.69 KB, image/jpeg
Details
bloat / leak log when switching around between addressbooks (LDAP and local)
81.54 KB, text/plain
Details
updated patch, fixed conflicts.
604.28 KB, patch
dmosedale
: review+
Details | Diff | Splinter Review 
need to convert addressbook to use new RDF outliner
QA Contact: esther → fenella
Blocks: 73948
--> me
Assignee: chuang → blake
QA Contact: fenella → nbaca
hey blake (and others), if possible, please hold off on this until I land #83023
Blocks: 25554
Target Milestone: --- → mozilla0.9.7
stephend, when you post your address book perf numbers on mozilla.org, can you
add a link here?

thinking ahead about this bug, for the addressbook "elements" pane, I want to
create an addressbook view (implementing an nsIOutlinerView), instead of using
the rdfliner.   The performance improvements would be close to what we got when
we converted the mailnews thread pane.  

switching from 

XUL templates | RDF datasource | addressbook db (mork) 
to 
outliner | addressbook view | addressbook db (mork)

is quite a bit of work, but worth it.  before starting on that work, I need to
investigate what code uses the datasource that I'd be removing.

I wouldn't be getting rid of the datasource behind the addressbook "addressbook
/ mailing list" pane.  Like the folder pane, we'd switch over to use the rdfliner.

blake, feel free to give this back to me.
taking from blake.

I'm going to be working on ab perf.
Assignee: blakeross → sspitzer
No longer blocks: 107151
ok got the basics going.  see the screen shot.

I'll attach my initial patch, it's still a work in progress.
Status: NEW → ASSIGNED
No longer depends on: 83023
here's how the results pane currently works:

rdf / xul tree templates (on top of)
directory datasource (on top of)
nsIAbDirectory (mork, ldap, outlook, etc).

we learned from the thread pane that the way to make it fast and use less 
memory is to use outliner (which means now that we only have the content node 
for the outliner body, instead of content for every tree cell, tree row, etc 
for every entry in the addressbook), and to remove the datasource.

what I've got so far is:

outliner
nsAbView (implements nsIAbView)
nsIAbDirectory (mork, ldap, outlook, etc)

showing data isn't hard (that part is working).  the harder part is fixing all 
the other code in the fe that used to use the tree, and now has to use the 
outliner, including drag & drop, commands, etc.
Blocks: 83023
Summary: addressbook to use new outliner → addressbook results pane to use new outliner
Attachment #56207 - Attachment is obsolete: true
Just curious: did you try using the rdfliner for this? Did it not provide a big
win? (Were you going to get rid of the AB datasource anyway?)
Attachment #56274 - Attachment is obsolete: true
> Just curious: did you try using the rdfliner for this?

no, but I plan to for the "addressbook" pane, like we did for the folder pane.

> Did it not provide a big win? 

yes, a big win.  I haven't measured officially, but I see the difference 
between an opt build with the old way and a debug build with the new way in 
scrolling, ab loading, and startup (which loads the first ab).

the bigger the AB, the bigger the win.

it also allows to hide columns, and if we want, to add more columns easily.

> (Were you going to get rid of the AB datasource > anyway?)

there's no need to go throug the AB datasource for the results pane, as it just 
fwds on to nsIAbDirectory.
Attachment #56275 - Attachment is obsolete: true
Attachment #56279 - Attachment is obsolete: true
Attachment #56653 - Attachment is obsolete: true
loading a large ab is still slow, but I've found out why.

*every* card is a listener on the addressbook.

zoinks!

removing that will make ab loading much, much faster.

nsAddrDatabase::AddListener(nsAddrDatabase * const 0x05ce4830, 
nsIAddrDBListener * 0x086370f4) line 267 + 16 bytes
nsAddrDatabase::CreateCard(nsIMdbRow * 0x05305068, unsigned int 0x00000000, 
nsIAbCard * * 0x08e37084) line 3757
nsAddrDatabase::CreateABCard(nsIMdbRow * 0x05305068, nsIAbCard * * 0x08e37084) 
line 3775
nsAddrDBEnumerator::CurrentItem(nsAddrDBEnumerator * const 0x08e37070, 
nsISupports * * 0x0012a73c) line 3522 + 52 bytes
nsAbView::EnumerateCards(nsIAbDirectory * 0x05ce8a30) line 100 + 47 bytes
nsAbView::Init(nsAbView * const 0x08e283a0, const char * 0x08e33040) line 81 + 
17 bytes


finding more badness, but should be easy to fix.

I've added all the column the spec requires, and I'm using the same column 
token in the xul as we do in the mork ab, so that in theory, we should be able 
to pass it back from GetCellText() without having to do string compares before 
we get to mork.

unfortunately, the addressbook is horked on crack.

so even if we call getCardValue("DisplayName"), we *still* do extra work.  

I'll make sure to get to that cleanup.

now the available columns include: "Name", "Email", "Presence", "Screen 
Name", "Work Phone", "Organization", "Nick Name", "Home 
Phone", "Fax", "Pager", "Cellular", "Additional Email", "Title" 
and "Department".

I'll attach a screen shot, and update the patch.
here's one really bad thing that the ab does:

when loading a directory, we bloat (into memory) a nsIAbCard for each card in 
the database.

that would be ok, if we only built it up with the card properties that we cared 
about, and if we did it lazily.  but we don't

instead, we copy all the properties (whether we ever need them or not).

this is sucks ass with autocomplete, as we'll try to get all the attributes for 
each card, and not just the attributes we need for autocomplete.
my todo list for the next few days:

1) sorting
2) fix bloat, be lazy with nsIAbCard (help with autocomplete perf, footprint)
3) no strcmp, map right to mork.
4) get add / delete working (view will have to listen on db)
5) edit changes to work
6) double click to edit mailing list
7) get "Properties" button to work again
8) make sure ab sidebar works
9) remove the unnecessary datasource
10) sort indicators
11) menu items for sort
12) menu items for show / hide columns
13) start up, select first card.

what works now:

1) use outliner
2) selection in outliner load card pane
3) add all columns per the spec, column picker, column reordering, persistence
4) improve ab load, every card is a no longer a listener on the db
5) double click to edit card
6) icons in result pane (card and mailing list)
Attached patch updated patch, add sorting (obsolete) — — Splinter Review 

        Attachment #56781 -
        Attachment is obsolete: true

        Attachment #56801 -
        Attachment is obsolete: true

        Attachment #56809 -
        Attachment is obsolete: true

        Attachment #56811 -
        Attachment is obsolete: true

        Attachment #56846 -
        Attachment is obsolete: true

        Attachment #56951 -
        Attachment is obsolete: true

    
    

    
  
this affects perf substantially, marking as a keyword
Keywords: perf

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #57468 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated patch, edit card working (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #57470 -
        Attachment is obsolete: true

    
    

    
  
here's what's left before I can land:

0) double clicking on list in folder pane to edit
1) sort indicators
2) make sure ab sidebar works
3) restore sort selection
4) drag n drop
5) make sure db or mailing list works
6) cmd updating (delete, properties, etc)
7) changing mailing list name doesn't always update folderpane
8) get "cards in ab" status text working again
9) test ab sync
10) double check for leaks
11) makefile / .mcp changes for mac and linux
12) test ab import
13) test ldap / outlook

here's what needs to be done, after the landing:

1) fix bloat, be lazy with nsIAbCard (work on autocomplete perf, CAB perf)
2) no strcmp, map right to mork.
3) rename collecter -> collector
4) improve listener foo (too much going on)
5) default height for splitter (to save a paint)
6) more perf work, footprint work on ab.

    
    

    
  


  
updated todo list:

here's what's left before I can land:

a) fix select address dialog
b) fix ab sidebar
c) sending assertions (in addressbook)
d) updates to mailing list not reflecting in card pane until repaint
1) double clicking in folder pane to edit mailing list
2) sort indicators
3) restore sort selection
4) drag n drop
5) make sure db or mailing list works
6) cmd updating (delete, properties, etc)
7) changing mailing list name doesn't always update folderpane
8) get "cards in ab" status text working again
9) test ab sync
10) double check for leaks
11) fix duplicate access keys
12) makefile / .mcp changes for mac and linux
13) test ab import
14) test ldap / outlook
15) test how it works with no AB.
16) show name as needs work
17) look for any XXXX / dump / printf
18) remove old results tree xul

here's what needs to be done, after the landing:

1) fix bloat, be lazy with nsIAbCard (work on autocomplete perf, CAB perf)
2) no strcmp, map right to mork.
3) rename collecter -> collector
4) improve listener foo (too much going on)
5) default height for splitter (to save a paint)
6) more perf work, footprint work on ab.
Summary: addressbook results pane to use new outliner → addressbook results pane, select address dialog results pane, ab sidebar should use outliner

    
    

    
  


  

        Attachment #57747 -
        Attachment is obsolete: true

        Attachment #57946 -
        Attachment is obsolete: true

    
    

    
  
there's now a branch.

win32:
cvs co -r AB_OUTLINER_BRANCH mozilla/client.mak
cd mozilla
nmake -f client.mak

linux:
cvs co -r AB_OUTLINER_BRANCH mozilla/client.mk
cd mozilla
gmake -f client.mk

mac:
not yet

see http://www.sspitzer.org/ab.html for the todo list, and the items completed.

URL: http://www.mozilla.org/mailnews/perfo... → http://www.sspitzer.org/ab.html

    
    

    
  
I noticed the ETA on your AB page... There are going to be test builds before
you land this (and turn it on) on the trunk, right?

    
    

    
  
there's are branch bits.  
see ftp://ftp.mozilla.org/pub/mozilla/nightly/latest-AB_OUTLINER/

there's a bunch of known issues that I'm working on.

(see http://www.sspitzer.org/ab.html)

    
    

    
  
Seth, 

curious if you are doing ok with all the stuff you have on the list.. are you in
need of assistance getting this one out of the way?

    
    

    
  
also some commercial bits on sweetlou 

for example, 

ftp://sweetlou/products/client/seamonkey/windows/32bit/x86/2001-12-05-18-
AB_OUTLINER/

    
    

    
  


  
these are the changes for the branch, I still need to re-review all the
changes, clean it up, apply to the trunk, and get a trunk based diff, for
review.

        Attachment #59197 -
        Attachment is obsolete: true

    
    

    
  


  
here's a patch for the trunk.  I'm testing / self-reviewing it now.

        Attachment #60960 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch for review. (obsolete)
        — 
          — Splinter Review
      

    


  
files removed:

mailnews/addrbook/resources/content/abResultsTreeOverlay.xul
mailnews/addrbook/resources/locale/en-US/abResultsTreeOverlay.dtd
mailnews/addrbook/src/nsAbMDBRDFResource.h
mailnews/addrbook/src/nsCardDataSource.h
mailnews/addrbook/src/nsAbMDBRDFResource.cpp
mailnews/addrbook/src/nsCardDataSource.cpp

files added:
(the trunk version of them are cat'd to the end of the patch, so you can review
them)

mailnews/addrbook/resources/content/abResultsPaneOverlay.xul
mailnews/addrbook/resources/content/abResultsPane.js
mailnews/addrbook/resources/content/addressbook-panel.js
mailnews/addrbook/resources/locale/en-US/abResultsPaneOverlay.dtd
mailnews/addrbook/public/nsIAbView.idl
mailnews/addrbook/src/nsAbView.h
mailnews/addrbook/src/nsAbView.cpp
themes/classic/messenger/addressbook/mac/abResultsPane.css
themes/classic/messenger/addressbook/win/abResultsPane.css
themes/modern/messenger/addressbook/abResultsPane.css

        Attachment #61163 -
        Attachment is obsolete: true

    
    

    
  
Here are some nits with the patch:

+         * Edits an existing mail in the directory
+
 * defined by the uri, or pass in the card for the list

in this function:

+function goToggleSplitter( id, elementID )

you have some logic that relies on the element's |checked| property's value. You
should check that it works, if you are messing with checkboxes, due to blake's
recent landing to make .getAttribute("checked","false") not work (and replace it
with .hasAttribute("checked")).

I don't know if that affects you, just a heads-up for the record.

+ catch (ex) {
+   return 0;
   }

What's the point of this? That function will terminate one line thereafter anyway...

+  var treechildren;
+  for (var i=0;i<children.length; i++) {
+    if (children[i].localName == "treechildren") {
+      treechildren = children[i];
+      break;
+    }
+  }

Did you mean to fetch all the nodes with localName "treechildren" into
treechildren?  Maybe this should be |treechildren += children[i]| then?
(Otherwise treechildren will always be the last "treechildren" node...afaics)

in this file:

Index: mailnews/addrbook/resources/content/addressbook.js

You are adding an old-style NPL-license. It should be a tri-license...

in these files:

Index: mailnews/addrbook/src/nsAbDirProperty.cpp
Index: mailnews/addrbook/src/nsAbMDBDirProperty.cpp

There's no newline at end of the file. This will hork some ports, IIRC.

In these two cases:

+      if (lastName.Length())
+      else if (firstName.Length())

you should probably check for !firstName.IsEmpty(), which I think is (will be?)
cheaper to do.

Suddenly, at a certain point in this patch, it became normal |cvs diff| output
(as opposed to |cvs diff -u|)...

This is very nice cleanup, it's gonna rock to be able to use a fast addressbook!
r=hwaara

    
    

    
  
BTW, disregard the treechildren comment, I didn't notice the break in that loop.

    
    

    
  
hwaara, missed the |break;| semantics [short explanation for posterity]
javascript:for(a=0;a<6;++a)break;document.write(a)
outputs 0, not 6.

@@ -137,9 +142,13 @@
        // defined by the uri
        void addMailListToDatabase(in string uri);
 
-       // Edits an existing mail in the directory
-       // defined by the uri
-       void editMailListToDatabase(in string uri);
+        /**
+         * Edits an existing mail in the directory
+        * defined by the uri, or pass in the card for the list
whitespace ^

RCS file: 
/cvsroot/mozilla/mailnews/addrbook/resources/content/abCardViewOverlay.js,v
@@ -94,6 +94,14 @@
odd alignment, i'm assuming you should be indenting to match...

+function GetAddressesFromURI(uri)
+{
+  var addresses = "";
+
+  var editList = GetDirectoryFromURI(uri);
+  var addressList = editList.addressLists;
+  if (addressList) {
+    var total = addressList.Count();
+    if (total) {
+      for ( var i = 0;  i < total; i++ ) {
+        var current = 
addressList.GetElementAt(i).QueryInterface(Components.interfaces.nsIAbCard);
+        
+        if (i == 0)
+          addresses = current.primaryEmail;
+        else
+          addresses += "," + current.primaryEmail;
        }
-    // for seperator all other display formats
-    else {
-      separator = cvPrefs.lastFirstSeparator;
     }
   }

i think you should drop the if (total)

js> function a(){try { for (;c<d;)throw "aa";} catch (e){return 1;}return 2;}
js> function b(){try { for (;c<d+1;)throw "aa";} catch (e){return 1;}return 2;}
js> var c=d=0;print (a()+" "+b())
2 1

@@ -178,10 +200,27 @@
whitespace in +'d lines.
+    cvSetVisible(data.cvhDescription, false);
+       cvSetVisible(data.cvbDescription, false);
you're doing a lot of paired hiding, wouldn't it make more sense to use some 
sort of grouping mechanism so you only hide one thing?

+                                        <description class="CardViewLink" 
hide="true" id="cvListNameBox">
+                                               <html:p><html:a href="" 
id="cvListName"/></html:p></description>
could you move </description> to line up w/ <description...> (either vertically 
or horizontally, i'm not picky.

+  if (gAbView)
+    return gAbView;
+  else 
+    return null;
brendan would say else after return: bad.
i'll instead ask why not just return gAbView (showing my complete ignorance of 
the code).

+function ChangeDirectoryByDOMNode(dirNode)
...
+return;
+}
there's no need for this return.

+function InvalidateResultsPane()
+{
+  var outliner = GetAbResultsOutliner();
+  outliner.boxObject.invalidate();
 }

why not just GetAbResultsOutliner().boxObject.invalidate(); ? [note, rginda 
says my suggestion is ugly, so this suggestion like all others is discardable]

+function GenerateAddressFromCard(card)
+{
+  var address = "";
+  var name = card.displayName
+  if (name.length) {
+    address += "\"" + name + "\" ";
+  }
+
+  if (card.isMailList) {
+    if (name.length) {
+      address += "<" + name + ">";
+    }
+  }
+  else {
+    var email = card.primaryEmail;
+    if (email.length) {
+      address += "<" + email + ">";
+    }
+  }
+  return address;
+}
would if (foo) work as well as if (foo.length)?


<<<<<<
+function GetParentDirectoryForMailingList(abURI)
+{
+  var parentURI = null;
+
+  var abURIArr = abURI.split("/");
+  /*
+   turn turn "moz-abmdbdirectory://abook.mab/MailList6"
+   into ["moz-abmdbdirectory:","","abook.mab","MailList6"]
+   then, turn ["moz-abmdbdirectory:","","abook.mab","MailList6"]
+   into "moz-abmdbdirectory://abook.mab"
+  */
+  if (abURIArr.length == 4 && abURIArr[0] == "moz-abmdbdirectory:") {
+    parentURI = abURIArr[0] + "/" + abURIArr[1] + "/" + abURIArr[2];
+  }
+  return parentURI;
 }
======
+function GetParentDirectoryForMailingList(abURI)
+{
+  /*
+   in "moz-abmdbdirectory://abook.mab/MailList6"
+   out "moz-abmdbdirectory://abook.mab"
+  */
+  return abURI.match(RegExp("^moz-abmdbdirectory\://[^/]*/"));
 }
>>>>>>


+var abDirTreeObserver = {
+  onDragStart: function (aEvent, aXferData, aDragAction)
this function appears to want to be a consumer of 
GetParentDirectoryForMailingList but is not


<<<<<<
+        var targetArr = targetURI.split("/");
+        var srcArr = srcURI.split("/");
+        if (srcArr.length == targetArr.length && srcArr.length == 4) {
+          if (srcArr[0] == "moz-abmdbdirectory:" &&
+              srcArr[0] == targetArr[0] &&
+              srcArr[1] == targetArr[1] &&
+              srcArr[2] == targetArr[2]) {
+            needToCopyCard = false;
+          }
+        }
======
if 
(GetParentDirectoryForMailingList(targetURI)==GetParentDirectoryForMailingList 
(srcURI))
  needToCopyCard = false;
>>>>>>
[feel free to assign the ruturns of that function to local variables for 
readability]

@@ -589,22 +619,19 @@
can you change from return(false); to return false; to match what i think is 
the predominant style?

the tail of  function UpdateCardView() is the same as the function 
OnClickedCard(card)

RCS file: /cvsroot/mozilla/mailnews/addrbook/src/nsAbDirProperty.cpp,v
\ No newline at end of file

RCS file: /cvsroot/mozilla/mailnews/addrbook/src/nsAbMDBDirectory.cpp,v reeks 
of tabs (they appear to be in your + lines)

+    else {
+      if (lastName.Length())
+        *aName = nsCRT::strdup(lastName.get());
+      else if (firstName.Length())
+        *aName = nsCRT::strdup(firstName.get());
+      else
+        *aName = nsCRT::strdup(NS_LITERAL_STRING("").get());
+        *aName = nsnull;
+    }

[timeless]   jag should i use: nsCRT::strdup(NS_LITERAL_STRING("").get());
<bz> timeless: why would use strdup there??
<jag> timeless: no
<bz> timeless: that seems very wrong....  (embedded nulls and the like)
<jag> timeless: ToNewUnicode(NS_LITERAL_STRING("")) if you really must
<bz> timeless: that looks incredibly broken

if (!lastName.IsEmpty())
  *aName = ToNewUnicode(lastName);
else if (!firstName.IsEmpty())
  *aName = ToNewUnicode(firstName);
else
  *aName = nsnull;

a caller is doing:
+  if (NS_SUCCEEDED(abSession->GenerateNameFromCard(workCard, format, &aName)) 
&& (aName))

the other caller does .get() so i think we have a problem
+      rv = abSession->GenerateNameFromCard(this, format, 
getter_Copies(properties [index_DisplayName]));
+      NS_ENSURE_SUCCESS(rv,rv);
+      
         if (*properties [index_DisplayName].get() == 0) {

so i think this should be changed to:
         if (properties [index_DisplayName]) {

sorry for the long review.

    
    

    
  
So far, besides the earlier reviews comments, these are all nits and up to you
to change or not:

some of the accessors (e.g., GetIsRemote) don't ensure the arg pointer is
non-null - not sure if that's required or not currently.
There's not much reason to change a single PRBool into a PRPackedBool since
there will be no savings and could be a eensy teensy performance hit (not that
it would matter in this code).

In nsAbMDBDirProperty::GetValue*

 NS_ENSURE_SUCCESS(rv,rv);
+  return NS_OK;
+

these could all be NS_ASSERTION(NS_SUCCEEDED(rv));
return rv;

which could generate one less return statement at the cost of not looking so
pretty :-) .

I've gotten to the aboutlook stuff so far, and I'm taking a break for now. Looks
like a vast improvement so far.

    
    

    
  
In nsAddrDatabase, I think I see some tabs:
+    // we need to do this for dnd
+    PRUint32 key = 0;
+
  err = GetIntColumn(cardRow, m_RecordKeyColumnToken, &key, 0);

+  PRUint32 count;
+
addressList->Count(&count);
+
+  nsXPIDLString newEmail;
+  rv = newCard->GetPrimaryEmail(getter_Copies(newEmail));
+  NS_ENSURE_SUCCESS(rv,rv);
+
+
PRUint32 i;
+  for (i = 0; i < count; i++) {

this left-over comment is a little cryptic
//let it starts from 1

more tabs? I know sometimes cvs diff -uw shows funny things...
+  nsIMdbRow* cardRow = nsnull;
+
mdbOid rowOid;
+
rowOid.mOid_Scope = m_CardRowScopeToken;

you can use an nsCOMPtr here if you want.

+  nsIMdbRow* cardRow = nsnull;
and you might want to since I don't see you releasing this cardRow. This seems
to be in several routines in this file so you might want to look over the uses
of GetRow.






    
    

    
  
looks like tabs horked in > nsresult nsAbView::EnumerateCards()

and in nsAbView::CreateCollationKey

> // ensure restored selection is visible
> if (cardWasSelected) {
> if (mOutliner) 
> mOutliner->EnsureRowIsVisible(index);
> }
could be:
if (cardWasSelected && mOutliner)
  mOutliner->EnsureRowIsVisible(index)

    
    

    
  
moving to 0.9.8, the AB_OUTLINER_BRANCH should land in early 0.9.8

Target Milestone: mozilla0.9.7 → mozilla0.9.8

    
    

    
  
sorry for the delay.  I'll go work on the reviewers comments.  the next patch 
will also have support for exporting addressbooks (to just .csv and .tab for 
now), some code cleanup, and some performance fixes to 
nsAbCardProperty::Get/SetCardValue().

I'll attach a new patch once I address all the comments.

    
    

    
  
new patch on the way. includes changes to export directories in .csv or .tab 
format.

also some additional code cleanup (removed dead export code), and added file 
types for when doing text addressbook import.

for the reviewers comments that I didn't address in the new patch:

1)

> you have some logic that relies on the element's |checked| property's value. 
You
> should check that it works, if you are messing with checkboxes, due to blake's
> recent landing to make .getAttribute("checked","false") not work (and replace 
it
> with .hasAttribute("checked")).

The method in the patch works.  has blake landed yet, or is he going to land?

2)

>      ChangeDirectoryByDOMNode(children[i]);
>      // XXX need this? return;

I need that so I select just the first treeitem, instead of all the treeitems,
and resulting in the the last one as being selected.

3)

NS_ENSURE_SUCCESS(rv,rv);
return NS_OK;

vs

NS_ASSERTION(NS_SUCCEEDED(rv),"...");

you are right, that in a non debug mode this results in one less if () check 
and one less return.

I'm just not sure what to fill in for "...", so I've continued the pattern, 
since I get the assertion text for free.  objections to me leaving it as is?


    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated patch (obsolete)
        — 
          — Splinter Review
      

    


  
updated patch, I haven't javadoc'd the new idl files (as dmose has requested)
yet.

this patch includes more cleanup, and code to handling exporting of directories
to .csv / .tab formats.

        Attachment #61192 -
        Attachment is obsolete: true

    
    

    
  
also in the last patch are some perf changes to Set/GetCardProperty()

that code will hopefully go away, but until then, a long list of string 
compares is now some hairy switch statements that we can optimize more, by 
giving more common columns (last name, first name, primary email, phone, etc, 
stuff that commonly appears in the results pane) shorter code paths.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated patch (obsolete)
        — 
          — Splinter Review
      

    


  
localized some strings I forgot to localize, added a title to the export
addressbook dialog, made it so if you delete the "current" addressbook (in the
addressbook or the addressbook sidebar panel) we'll switch to the personal
addressbook.  I've fixed the nsIAbListener to do selective notification (like
we do for folder listeners).  we now delete the existing file on export (if the
user confirmed to overwrite).

        Attachment #61561 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated patch (obsolete)
        — 
          — Splinter Review
      

    


  
this patch fixes some problems with exporting, adds some code (but turned off
with a comment) about how our .csv format is not like Outlook Express.	this
code also fixes a problem with the import code.  the import code runs on
another thread, so we need to proxy back to access the nsAddrDataBase when
importing.

        Attachment #61607 -
        Attachment is obsolete: true

    
    

    
  


  
this patch includes a new comment about how we should be integrating with the
per server "maxHits" patch for ldap addressbooks, and has some ab / ldap
cleanup.

        Attachment #61636 -
        Attachment is obsolete: true

    
    

    
  


  
bloat / leak log when switching around between addressbooks (LDAP and local)

the local ab code doesn't seem to leak, at least not the nsAb* objects.

when using LDAP, we do leak.  dmose says that might be a known problem, but we
can investigate.

here's the nsAb* object that leak, when you use ldap in addressbook.

 108 nsAbBooleanConditionString 		    56	     56        1       
1 (    1.00 +/-     0.00)	15	  1 (	 1.69 +/-     0.66)
 109 nsAbBooleanExpression			    20	     20        1       
1 (    1.00 +/-     0.00)	18	  4 (	 2.63 +/-     1.43)
 110 nsAbCardProperty				   748	  27676      552      
37 (  249.09 +/-   144.91)     5978	  37 (	254.44 +/-   153.75)
 112 nsAbDirSearchListener			    16	     48        3       
3 (    2.00 +/-     1.00)	12	  3 (	 2.71 +/-     1.10)
 113 nsAbDirectoryQueryArguments		    28	     84        3       
3 (    2.00 +/-     1.00)      241	  3 (	 2.98 +/-     1.09)
 118 nsAbQueryLDAPMessageListener		    68	    204        3       
3 (    2.00 +/-     1.00)	27	  6 (	 4.83 +/-     2.29)

    
    

    
  
Comment on attachment 61667 [details] [diff] [review]
updated patch, includes some more clean up

r=bienvenu

        Attachment #61667 -
        Flags: review+

    
    

    
  
OK, here's my first set of comments; I'm only about 20% of the way through the
patch, so there'll be more to come.  Note that most of these are nits, and
shouldn't hold up the landing of this patch, so if they should move to another
bug, that's fine by me.

nsAbFactory.cpp
---------------
a) an extra commented out GENERIC_FACTORY_CONSTRUCTOR has been left here

nsIAbCard.idl
-------------
b) what's the point of getCardUnicharValue?

nsIAttrDatabase.idl
-------------------
c) getCardFromAttribute has "value" typed as a |string|.  But values
aren't necessarily just ASCII, right?  ie shouldn't that be an |AString|?

abCardViewOverlay.js
------------------
d) in GetAddressesFromURI, how about priming addresses with the first
email address before beginning the loop and the start the loop with
i=1?  That way you don't have to evaluate the "if (i == 0)" on every
pass through the loop.

abCommon.js
-----------
e) the license boilerplate added here is outdated; can you change it to
the latest tri-license boilerplate from <http://www.mozilla.org/MPL>?

f) GetSelectedAddresses: same as comment d

g) GetSelectedRows: the |cards| variable doesn't appear to be used at
all.

h) GetSelectedRows: what's the point of outlinerSelection?  why not
just use gAbView.selection?

i) GetSelectedRows: same as comment d

j) GetSelectedAbCards: same as comment h

k) GenerateAddressFromCard: I don't think it's necessary (and doesn't
look so good) to have quotes around the display name if there's no
unusual chars in it.  One way to avoid that would be to have this function
nsIMsgHeaderParser::MakeFullAddress under the hood?  Although that
seems to (in spite of the idl comments) use UTF8, so hmmmm.... I guess it
depends what these generated addresses are used for.

l) GetParentDirectoryFromMailingList: since the code here doesn't
actually reference the abURI param, does this function work?  And even
if so, wouldn't it be significantly lighter weight to just search from
the right end of the string for / and return a substring?

general
-------
a) any reason not to switch from addbook: to moz-addbook: URLs while
we're here?

b) for newly added methods, how about using AString instead of
wstring?  This will cut down on unnecessary mallocs().

c) I think you've missed adding yourself to the Contributor lists in some of the
license boilerplates



    
    

    
  
Patch looks good. Here are couple of things I want to point out :

1. Looks like couple of globals (like gPrefs, gAddrbookSession in
abCardViewOverlay.js) are defined in the middle of file, can be moved to the top.
2. You have already made note in the code to use string bundles (while copying
cards, alerts, etc) in couple of places. If there is a major revision coming up,
then you can fix those. Otherwise, a new bug will do.
3. Per http://www.mozilla.org/mailnews/specs/addressbook/ & section "Drag and
Drop of Card Entries from One AB to Another", we should be doing a move instead
of copy (copy when crtl key is used).   If the change is simple fix this one.
There is a open bug to track this already. I will get that one for you later.
4. looks like you forgot to remove #if 0 segment
5. if same primary addresses constitues equality, we shall get that into card's
Eqauls API.
6. All that unused code from AddressBookParser::AddLdifRowToDatabase should go away.
7. +            rv = abDataBase->GetCardFromAttribute(abDirectory,
kPriEmailColumn, emailStr.get(), PR_TRUE /* lowerCase */,
getter_AddRefs(existingCard)); 
if you care about the case here, the caseinsensitive boolean should be
PR_FALSE...right..?

r=bhuvan.

    
    

    
  
Additional comments; more to come, hopefully will finish tommorrow.

abSelectAddressesDialog.js
--------------------------
m) DropOnBucketPane:

+  var dragService =
Components.classes["@mozilla.org/widget/dragservice;1"].getService();
+  if (dragService)
+    dragService = dragService.QueryInterface(Components.interfaces.nsIDragService);
+  if (!dragService) 
+    return false;
+

Isn't it true that the only way drag service is gonna end up null will
throw an exception?  So I think try/catch is really what's needed
here instead of those if statements.  Same goes for the createInstance
of |trans|, I think.

n: DropOnBucketPane: it looks like this function returns |false| in
all cases.  Is this really what you want?


addressbook.js
--------------
o: outdated license boilerplate

p: AddPrefObservers, RemovePrefObservers: does this syntax do the
right thing?

+  var prefService = Components.classes["@mozilla.org/preferences-service;1"]
+                              .getService(Components.interfaces.nsIPrefService);

I thought the required syntax was
.getService().QueryInterface(Components.interfaces.nsIPrefService).  I
seem to remember the currently used syntax causing problems with LDAP
autocomplete.

nsAbAddressCollecter.cpp, nsAbAutoCompleteSession.cpp:
------------------------------------------------------
q: any particular reason to use the CID for the message header parser
rather than the contract ID?


nsAbAutoCompleteSession.cpp
---------------------------
r: how about using an NS_STATIC_CAST here, rather than C-style?

+  if (!aSubstrLen || (nsCRT::strlen(aString) < (PRUint32) aSubstrLen))

s: nsAbAutoCompleteSession::SearchDirectory().  any reason not to just
use the libc version of strcmp here (since those are sometimes
hand-optimized)?

nsAbLDAPDirectoryQuery.cpp
--------------------------
t: Assuming we don't mind a link-time dependency on libxpcom, you can
get the proxy object service and an object all in one shot using
NS_GetProxyForObject(), which is defined in
nsIProxyObjectEventManager.idl.

u: instead of using the deprecated n.EqualsWithConversion, how about
using n.Equals(NS_LITERAL_STRING("card:nsIAbCard"))?

nsAbMDBDirectory.cpp
--------------------
v: There are a couple of instances of the c-style (const char *) cast;
how about using NS_CONST_CAST instead?

w: GetAbDatabase:

Instead of using PL_strlen (instead of possibly hand-optimized libc
version) and old string tech:

+    nsAutoString file;
file.AssignWithConversion(&(mURI[PL_strlen(kMDBDirectoryRoot)]));

how about one of:

+    NS_ConvertASCIItoUCS2 file(&(mURI[strlen(kMDBDirectoryRoot)]));

or

+    NS_ConvertUTF8toUCS2 file(&(mURI[strlen(kMDBDirectoryRoot)]));

nsAbQueryStringToExpression.cpp
-------------------------------
x) nsISupports::AddRef is templatized in such a way that if you write this:

+    *expression = expr;
+    NS_IF_ADDREF(*expression);

as this:

+    NS_IF_ADDREF(*expression = expr);

it'll be compiled to code that is a bit faster.  There are other
instances of this in this patch, so you may wanna do a quick grep for
NS_IF_ADDREF.

nsAddrBookSession.cpp
---------------------
y) NotifyDirectoryDeleted: c-style cast

nsAddrDatabase.cpp
------------------
z) nsAddrDatabase::SetCardValue: any reason not to use 

NS_ConvertUCS2toUTF8 utf8str(value);

instead of the INTL_ConvertFromUnicode stuff?




    
    

    
  
Remaining comments:

nsAddressBook.cpp
-----------------
A) The attribPairsType stuff looks like it duplicates code in
nsAbLDAPProperties.cpp

B) ::ExportDirectory: There doesn't appear to be any error checking on
any of the calls through outputStream.  Presumably we need to notice
any errors (eg disk full) and propagate them to the UI.

addressingWidgetOverlay.js
--------------------------
C) DragOverTree, DropOnAddressingWidgetTrees: I think that .getService
and .QueryInterface will throw exceptions for anything that's gonna
make dragService or dataObj be null, so you probably need try/catch
clauses here.

fieldMapImport.js
-----------------
D) same as C, with trans and dragService

importDialog.js
---------------
E) GetStringBundle: same as D

F) ImportAddress: If it's really necessary to leave a dump() in
production code (is it?), how about using a more meaningful message so
that someone who hits this knows where to look?

nsImportAddressBooks.cpp
------------------------
G) same as t

abResultsPaneOverlay.xul
abResultsPane.js
addressbook-panel.js
abResultsPaneOverlay.dtd
abResultsPane.css (all versions)
------------------------
H) wrong license header

addressbook-panel.js
--------------------
I) gAddressBookPanelAbListener.onItemRemoved: you don't need the
"directory &&" half of the if clause.  If directory is null, the QI
will have thrown an exception by this point, which will have kicked
control to the catch clause.

nsAbView.cpp
------------
J) why do all the NS_INTERFACE_MAP stuff by hand rather than using
NS_IMPL_ISUPPORTS4()?

K) ::GetURI(): how about using ToNewCString(mURI) instead of nsCRT::strdup?

L) ::Init(): 

   * if you make GENERATED_NAME be an NS_NAMED_LITERAL_STRING
     rather than a #define, wrapping it multiple times won't be necessary.

   * it looks as though actualSortColumn can be unnecessarily copied into
     twice in the same invocation.

   * it should be possible to use .Equals() and avoid the .get()
     required for strcmp

   * I think ToNewUnicode() is preferred to nsCRT::strdup()

M) ::GetCellProperties
   
   * should the first if actually be |if (colID[0] != PRUnichar('G'))|?

N) ::GetRowProperties: 

   * why pre-initialize isMailList, since the existing value will be
     overwritten?

   * the call to properties->AppendElement() should be error-checked

O) ::GetCardValue: 

   * same as M

   * column can be declared as NS_LossyConvertUCS2toASCII column(colID);

P) ::inplaceSortCallback(): same as M

Q) ::GetSelectedCards(): NS_NewISupportsArray(selectedCards) seems to be
called twice.

As before, most of these are just nits; in general, this looks great and is
clearly a huge step forward for the addressbook code.



    
    

    
  
I'm working on a patch for these issues.  the only two I don't understand are 
two from bhuvan:

5. if same primary addresses constitues equality, we shall get that into card's
Eqauls API.

6. All that unused code from AddressBookParser::AddLdifRowToDatabase should go 
away.

bhuvan, can you elaborate?

    
    

    
  
5. if same primary addresses constitues equality, we shall get that into card's
Eqauls API.

I guess the real question is

In nsAddrDatabase.cpp, at the context @@ -1656,22 +1568,64 @@

don't we already do, 

+    if (!nsCRT::strcmp(newEmail.get(), currentEmail.get())) {

while checking equality in

+    rv = newCard->Equals(currentCard, &equals);
+    NS_ENSURE_SUCCESS(rv,rv);

If so, then I thought emailaddress check is redundant. If email is to be
considered as a valid key to check while checking the equality, I thought that
check can be placed in ::Equals() directly.

6. All that unused code from AddressBookParser::AddLdifRowToDatabase should go 
away.

There are several instances of unused like the following in that routine

-
  // else if ( -1 != colType.Find("charset") )
+
  // else if ( kNotFound != colType.Find("charset") )
 	  //   ioRow->AddColumn(ev, this->ColCharset(), yarn);
 
Are we going to use any of these commented out code in the future..? If not, we
should remove that code.

bhuvan

    
    

    
  
thanks for the solid reviews.  I've addressed all the comments (except for 
bhuvan's two, I'll go look into those.)  new patch on the way.

while I test my patch, and update my tree to the trunk, here's some feedback 
for what I didn't address:

>l) GetParentDirectoryFromMailingList: since the code here doesn't
>actually reference the abURI param, does this function work?  And even
>if so, wouldn't it be significantly lighter weight to just search from
>the right end of the string for / and return a substring?

whoops, not sure when abURI.match(RegExp()) became RegExp()

I went back to the way I original did it, using split.
've added a comment to indicate that the function returns null when the uri is 
not
a mailing list.

>7. +            rv = abDataBase->GetCardFromAttribute(abDirectory,
>kPriEmailColumn, emailStr.get(), PR_TRUE /* lowerCase */,
>getter_AddRefs(existingCard)); 
>
>if you care about the case here, the caseinsensitive boolean should be
>PR_FALSE...right..?

fixed, the comment was wrong, the code was correct

>q: any particular reason to use the CID for the message header parser
>rather than the contract ID?

fixed my usage and others, I noticed we had a mix of GetService and 
createInstance,
so I switched them all to GetService(), as it is a service has no state.

>u: instead of using the deprecated n.EqualsWithConversion, how about
>using n.Equals(NS_LITERAL_STRING("card:nsIAbCard"))?

even better that code can just be n.Equals("card:nsIAbCard").  I need to 
understand that
code better to fully criticize what's going on.  but on quick glance, it 
doesn't look kosher.

>v: There are a couple of instances of the c-style (const char *) cast;
>how about using NS_CONST_CAST instead?

did foo.get() instead of (const char *)foo

>z) nsAddrDatabase::SetCardValue: any reason not to use 
>
>NS_ConvertUCS2toUTF8 utf8str(value);

cleaned up a lot of the string foo, removed INTL_ConvertToUnicode() and 
INTL_ConvertFromUnicode() from nsDirPrefs.*

> i) GetSelectedRows: same as comment d

in this case, I'm interating over each range count, and then each value
in each range count.  so instead of just unrolling the first in a for loop
I'd be unrolling the the first of the outer and more.  I decided it wasn't 
worth it.

> a) any reason not to switch from addbook: to moz-addbook: URLs while
> we're here?

I'll spin up a separate bug for that issue, for cavin, who will be
working on printing and ab sync.

> 3. Per http://www.mozilla.org/mailnews/specs/addressbook/ & section "Drag and
> Drop of Card Entries from One AB to Another", we should be doing a move 
instead
> of copy (copy when crtl key is used).   If the change is simple fix this one.
> There is a open bug to track this already. I will get that one for you later.

yes, I've left it as a copy on purpose, until we discuss the behaviour.
move has interesting issues, like what does it mean to move a mailing list?
or move a card from directory 1 to directory 2, where the card is part of a 
mailing list
on directory 1.

> A) The attribPairsType stuff looks like it duplicates code in
> nsAbLDAPProperties.cpp.

it is.  This table of columns, if we do it right, could be shared for import 
LDIF and .csv, 
exporting LDIF and .csv, and for LDAP.  for now, I've removed the used ldif 
attributes
from the table in nsAddrDatabase.cpp.  getting all three places (import, 
export, LDAP) to
use the same table is going to be an involved task, I'll spin it off to a 
seperate bug.

> c) getCardFromAttribute has "value" typed as a |string|.  But values
> aren't necessarily just ASCII, right?  ie shouldn't that be an |AString|?

GetCardFromAttribute() and GetRowFromAttribute() require the value to be UTF8,
I've fixed the variable name and added comments.

>p: AddPrefObservers, RemovePrefObservers: does this syntax do the
>right thing?
>
>I thought the required syntax was
>.getService().QueryInterface(Components.interfaces.nsIPrefService).  I
>seem to remember the currently used syntax causing problems with LDAP
>autocomplete.

getService(Components.interfaces.nsIPrefService) works, we do it all over.
I vaguely remember that with autocomplete, I'm not sure why we had to do that.


    
    

    
  
> 5. if same primary addresses constitues equality, we shall get that into 
> card's Eqauls API.

now I understand.  no, primary email address is not part of card equality, as 
you could have two cards with the same email.  that's for comparing nsIAbCard 
instances, for cards in the local addressbook, we can use special attributes
to determine if two nsIAbCards are the same.  they way the addressbook code 
works, you could create more than on nsIAbCard for a given card in the 
addressbook.

> 6. All that unused code from AddressBookParser::AddLdifRowToDatabase 
> should go away.

I'll log a bug about cleaning up that code.  it might be getting turned on, to 
expand our ldif import capabilities.

patch coming soon...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated page (obsolete)
        — 
          — Splinter Review
      

    


  
this patch addresses the reviewers comments, and some polish:

1) existing or new profiles will only get the default columns, not all columns
in the addressbook
2) when importing, we select the first item.
3) import module names are sorted.
4) "Text Files" -> "text files (LDIF, .csv, .tab, .txt)"
5) "converting mailbox from Text" -> "converting addresses from text file"
6) header parser, all callers do getService() instead of a mix of getService()
and createInstance().
7) after import of an addressbook, we now commit.  there was a bug that if you
quit right away, you won't save your imported ab.
8) support for "Map It!"  addressbook feature.

        Attachment #61667 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 62530 [details] [diff] [review]
updated page

whoops, that patch has 2 conflicts.

        Attachment #62530 -
        Attachment is obsolete: true

    
    

    
  
I'm not sure this is the right thread, but the address book really needs an
address picker pane a la Netscape 4.76. Finding names by scrolling (with poor
response time) a big address book is a total pain.

I cannot get mailing lists to work at all. They do not import, do not display,
and you cannot send mail to them! 

I tried the 12/7 build and was unable to use it at all because once one item
(e.g., browser or mail) was opened, no others could be (like the address book).

    
    

    
  
> but the address book really needs an
> address picker pane a la Netscape 4.76. Finding names by scrolling (with poor
> response time) a big address book is a total pain.

that's AB quick search, see http://bugzilla.mozilla.org/show_bug.cgi?id=83023

> I cannot get mailing lists to work at all. They do not import, do not display,
> and you cannot send mail to them! 

can you log a bug to track this issue?


    
    

    
  
>> I cannot get mailing lists to work at all. They do not import, do not 
>> display, and you cannot send mail to them! 
> can you log a bug to track this issue?

they work for me, so if you are having problems, please log a seperate bug.

    
    

    
  
Comment on attachment 62533 [details] [diff] [review]
updated patch, fixed conflicts.

r=dmose@netscape.com

        Attachment #62533 -
        Flags: review+

    
    

    
  
this has sr=bienvenu.

landed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
Trunk build 2002-01-28-03: WinMe
Trunk build 2002-01-28-08: Linux RH 7.1
Trunk build 2002-01-28-08: Mac 10.1
Verified Fixed.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: