83091 - Need to modify the addressbook to read Autocomplete entries for LDAP servers

Status: VERIFIED FIXED

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

Description

[Cuchulainn]

There are new LDAP entries in prefs.js created as a result of dmose work on
autocompletion. The URI scheme used is not compatible with our existing LDAP
scheme so we will have modify the existing LDAP factory implementation and the
preferences related code to use these entries in the addressbook.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

What exactly do you need to change?

Comment 2

[Csaba Borbola]

We have to translate the ldap scheme into our abldapdirectory scheme in LDAP
directory factory. 
In order to be able to pick up the LDAP autocompletion settings for Address
Books, I need to to modify a bit the nsDirPrefs.cpp file too.
I have to switch from our SUNLDAPDIRECTORY directory type to LDAPDIRECTORY type,
which needs a few changes in the nsAbBSDirectory.cpp file as well.
These modification would depend on the new search UI stuff (bug 83023) and the
our patch II (bug 78933) as well.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

New URI schemes will need to start with "moz-" as per bug 69513.  Perhaps this
is a good time to fix that both here and in 78933 (patch II).  It'll need to be
fixed in patch II before that can be checked in; I'll note that in 78933 itself
as part of my review.

Since it sounds like you'll be doing the coding on the this patch, I'm
reassigning the bug to you; hope that's ok.

Comment 4

[Csaba Borbola]

Thanks you for the honor, that you assigned me this bug. Of course, it's not a
problem at all.
I've read your comments in bug 78933 as well and I'll change the URI scheme as
it is written in bug 69513.
I'm still waiting for the patch of bug 83023, because I'm depending on it. As
soon as I get it, I'll submit my patch too. Basicly I'm ready with these
changes, but I can't make my tests without that patch.

Comment 5

[Csaba Borbola]

Attachment: Proposed patch. I haven't managed to test yet. It is only for first review.

Comment 6

[Blake Ross]

Doesn't look like this is getting fixed before the freeze tomorrow night.
Pushing out a milestone.  Please correct if I'm mistaken.

Comment 7

[Csaba Borbola]

You were right, Blake.
Now it seems to be, that the #78933 will land soon, so I will do an update on
this patch and I'll post it to here.

Comment 8

[fenella]

LDAP is Yulian's area.

Comment 9

[Csaba Borbola]

Sorry, but I can't work on this bug anymore.

I would like to summarize the status of this bug.
There are 4 cases to finish this bug:

1. Submit the current state of the implementation, which has a lot of TODOs yet.
This patch does the followings: picks up the URI entries at address book loading
time from the preference file (prefs.js). It picks up those entries, which were
added to the Autocompletion Directory Server list at the addressing preferences.
The addressbook contains a preference Observer for the ldap_2.server" preference
entry changes. Via this the addressbook gets notified about adding a new entry
and modifying an existing one, but not about a deletion. 
There are some addressbook interface changes and preference filter
implementations, which takes only the "ldap://" pref changes into the BootStrap
directory implementation, which is the top level address book directory entry.


2. Submit the Notification/Observer implementation only, which would take care
about the LDAP entriesadded, modified and deleted by the autocompletion
preference module and submit a separate UI bug for the necessary UI changes.
This still needs the update of the preference Observer interface, which does not
notifiy about deletion.

3. Rewrite the Directory Server List handling under control of addressbook
session. This needs some UI changes as well. 
This solution looks the best and the most expensive one, but I think this is the
one, which is the best in terms of structure. All the Directroy Server
preferences would be under control of the AB and the autocomplete session should
pick up one for using with a combo box, like it is doing now.
The advantages of this solution is that this would eliminate the LDAP specific
Addressbook interface changes, which is needed for the other solutions. The
deletions would work as well, since there is no need for notification (we could
not delete the one, which is selected for autocompletion).

There is now a pressing need to have a separate UI for Address Book attributes.
Up to recently Mozilla only supported one type of Address Book viz the Personal
Address Book. Now Mozilla can support Outllok, Outlook Express and LDAP. We
should be able to right-click on the Address Books and see and set the
preferences.  


4. Submit a simple solution of this bug. This would be able to pick up those URI
ldap entries, which already exist at loading time of the addressbook. All of
those would appear on the left pane of the address book window. There is not any
notification implementation. If you add a new Directory Server entry at the
autocompletion preference settings, you have to restart Mozilla to pick up the
new entry.


Considering the limited time I can spend more on this bug, I offer the 4th
solution and will submit the patch as soon as it is tested.

Comment 10

[Csaba Borbola]

Attachment: proposed patch for the simple solution. NOTE: dependent on bug83023

Comment 11

[John Marmion]

On 24th August I proposed a new solution to #83100. This solution would involve 
amending the File->New->Address Books to include both LDAP and Outlook/Outlook
Express now that 78933 has landed. I have just posted a new gzipped tarball of
the screen shots to 83100. They will aslo be available on our
http://www.abzilla.mozdev.org 
site tomorrow.

Comment 12

[Jaime Rodriguez, Jr.]

Moving to 0.9.5 per PDT, and marking to nsenterprise-

Comment 13

[Jussi-Pekka Mantere]

Without this fix, Bug 83023 is practically useless (by extension, Search LDAP 
Directories in Address Book.) We need to keep just one set of LDAP preferences 
around; a single LDAP server entry needs to work for both Typedown Addressing and 
Address Book Search.

Bugs 83091 and 83023 need to be both nsenterprise- or both nsenterprise+, not a 
mix of the two.

Comment 14

[Csaba Borbola]

It isn't really true, that the #83023 is useless without this fix, because you
can perform search on any kind of addressbooks, for example the native Mork
database based local addressbooks. Of course, this fix would give the main
meaning of #83023.

Comment 15

[Jussi-Pekka Mantere]

True, not useless, but a very bad end-user experience. Getting this fixed should
be as high a priority as checking in Bug 83023. Setting severity to "major".

Comment 16

[Asa Dotzler [:asa]]

0.9.5 is out the door. bumping up the TM one notch

Comment 17

[Csaba Borbola]

Passing the ownership to John Marmion@sun, because I was moved to another
project.

Comment 18

[John Marmion]

Attachment: Update Patch

Patch to create a unified integrated approach of adding LDAP Directory Servers
through the Preferences and through the Address Book UI. Thus
Additions/Deletions in either need to be reflected in both. I need the LDAP
Autocomplete people to look at this. There are still some issues to be sorted:

1. support for maxHits preference value in the Address Book
2. What to do if an attempt is made to delete the Selected AutoComplete Server
from the Address Book.
3. This patch will need to be updated following bug 73868 and bug 83023.

I have chosen to write away the URI in the preferences as "ldap://" but to
translate this to existing Address Book URI scheme "moz-abldapdirectory://"
when accessed as an Address Book. This patch is a 'must' for bug 83023.

Comment 19

[John Marmion]

Attachment: Small update to previous patch

Note this patch was built against the head on 17th December 2001.

Comment 20

[John Marmion]

Attachment: Update patch against the head

Update patch following major Address Book changes following bug #73868 and bug
#83023. Also, change URI scheme from "moz-abldapdirectory://" to "ldap://" .

Comment 21

[(not reading, please use [e-mail] instead)]

john, as soon as I land my LDIF export code, I'll start taking a look at your 
patch.

Comment 22

[John Marmion]

I will keep the patch current against the head in the meantime. This patch still
has a number of issues outstanding but its real purpose is to get agreement on
how to unify the addition of LDAP Address Books. To recap outstanding issues :

1. support for the 'maxHits' preference value 
2. Add/Delete work from the Preferences but what to do about Edit? 
3. What to do (if anything) if an attempt is made to delete the Selected
Directory Server from the AB.

Comment 23

[(not reading, please use [e-mail] instead)]

I'm leaning towards #3 from http://bugzilla.mozilla.org/show_bug.cgi?id=83091#c9

For local addressbooks, LDAP directories, LDAP autocomplete directories, and
other addressbooks types (OE, MAPI, etc) I'd like to see us have something
similar to the account manager and the account manager datasource

something else we should keep in mind is alecf's isp datasource.  this allows
ISPs and IT departments to pre-flight accounts.  if we did something similar for
LDAP, that might prove really useful.   that is probably beyond the scope of
this bug, but something to keep in mind.

I'm going to review the latest patch and see what it is doing, and then report back.

taking from john, as I'm working on this full time now.

when I check in, john and the original author will get credit.

Comment 24

[(not reading, please use [e-mail] instead)]

Attachment: proof-of-concept patch

this small patch makes one of my LDAP autocomplete directories show up in the
addressbook.

I need to figure out how to convert a ldap:// url into "moz-abldapdirectory://
uri.

before I get too deep, I'll discuss this idea with dmose and others.

what's after that would be getting add / delete and edit to work.

Comment 25

[(not reading, please use [e-mail] instead)]

Attachment: patch, with comment

this proof of concept patch makes it so my two ldap autocomplete directories
(one with a query) shows up.

editing is going to be interesting, as hostname, port and query are part of the
URI.   I'll think more about that tomorrow.

Comment 26

[John Marmion]

Seth, my initial patch before xmas was to store the "ldap://..." in the prefs
and transform to "moz-abldapdirectory://.." before the Directory Factory
instantiation in the code. But I later changed that in my latest patch to use
"ldap://" only. I thought it was a much cleaner solution. It sounds like you are
going through similar soul searching.

Comment 27

[John Marmion]

Attachment: Keep the patch current against the head

Comment 28

[(not reading, please use [e-mail] instead)]

Attachment: backend patch, almost there

here's a back end patch.  I need to fix whitespace and review my string usage.

this patch adds the level of indirection that we are going to need between
moz-abldapdirectory:// uri and ldap:// url.

the way it works is that we take the ldap url, create a unique (and
non-changing moz-abldapdirectory:// uri for it. 

user_pref("ldap_2.servers.nscpphonebook.uri",
"ldap://nsdirectory.mcom.com:389/ou=People,dc=netscape,dc=com??one");

->

moz-abldapdirectory://ldap_2.servers.nscpphonebook

before, we were use the resource URI as the LDAP URL. (expect instead of
ldap:// we were using moz-abldapdirectory:// as the scheme)

now, we take the URI (moz-abldapdirectory://ldap_2.servers.nscpphonebook) and
turn that into a pref (ldap_2.servers.nscpphonebook.uri) and then get the LDAP
url when we need it (to establish a LDAP connection).

now, if someone tweaks the the hostname, port, base dn, etc on the LDAP server
(via the autocomplete prefs), it won't be a problem.  the RDF URI stays the
same and the next time we do an LDAP search, we'll get the pref and the pref
will have been updated.

Comment 29

[(not reading, please use [e-mail] instead)]

here's a todo list, but I think only #1 needs to be fixed before some sort of 
backend fix can land.  #1 is important because it will affect autocomplete 
performance.

1)
skip moz-abldapdirectories when doing local autocomplete (I've got a bug on 
this already)

2)
directory ds needs to observer or get notified with autocomplete added or 
deleted.

3)
don't show moz-abldapdirectories in sidebar

4)
fix UI to disable when read only (like LDAP)  [this was part of an another SUN 
patch once]

5)
if your autocomplete prefs have a search filter, that doesn't affect LDAP in 
addressbook (bug, or is this desired)

5)

note, it looks like we got a lot of code just to get around the fact that 
DIR_Server 
is not scriptable (we use hash tables and some helper classes to turn the hash 
table into a XPCOM cstring array.)

I've extended the createDirectory() method on the nsIAbDirFactory interface to 
pass 
in the optional prefName string (which is server->prefName).  
I think there's a lot of code cleanup we can do here.

6)


        <menulist id="directoriesList" flex="1"
                  preftype="string" 
                  prefstring="ldap_2.autoComplete.directoryServer">
          <menupopup id="directoriesListPopup" 
            onpopupshowing="createDirectoriesList(true);">
          </menupopup>
       </menulist>

should be build from the directory datasource, type = "autocomplete"

+	mURI	0x057ec730 "moz-abdirectory://"

rooted like 

  <tree id="dirTree" 
      class="abDirectory" 
      ref="moz-abdirectory://"
      datasources="rdf:addressdirectory"

in abDirTreeOverlay.xul

7) add (or expose) UI to add, edit, delete, view properties of LDAP directory 
from addressbook

Comment 30

[John Marmion]

I agree, #1 would allow us to get this started with the minimum amount of code
changes but give us the ability to add LDAP Address Books. 

You can also add the maxHits to the todo list as this can be entered but the
Address Book continues to ignore it and always returns a maximum of 100.

>5)if your autocomplete prefs have a search filter, that doesn't affect LDAP in 
>addressbook (bug, or is this desired)

With the implementation of the quick search, I believe this is a bug and we need
to strip it out before instantiating the LDAP Address Book. I included this in
the nsAbLDAPDirFactory.cpp patch.

>7) add (or expose) UI to add, edit, delete, view properties of LDAP directory 
>from addressbook

This will open up other issues - like whether you can delete the AutoComplete
Server from here etc. but lets worry about that when we come to it.

Comment 31

[John Marmion]

Seth, one other issue, I noticed that you are transforming between uri schemes
ldap:// and moz-abldapdirectory://. As I stated in an earlier contribution, that
was how I originally implemented it but my final patch then settled for using
only ldap://.

I was interested in why you wish to implement it using both?

Comment 32

[(not reading, please use [e-mail] instead)]

moving to 0.9.9, these aren't 0.9.8 stoppers.

Comment 33

[(not reading, please use [e-mail] instead)]

this is something we want for nsbeta1, nominating.

Comment 34

[(not reading, please use [e-mail] instead)]

Attachment: updated patch, includes fix for #117540

updated patch.	this patch also skips LDAP directories during local
autocomplete.

Comment 35

[(not reading, please use [e-mail] instead)]

Attachment: updated patch, includes fixes for #117540, and #117452

this new patch (not final yet) includes a fix for #117452.  this patch gets
delete working after a quick search (on local addressbooks only)

Comment 36

[John Marmion]

I downloaded and built this patch today. I appreciate that this is still a work
in progress but I thought you might be interested in some feedback. Apologies if
I am telling you nothing new. 

Adding a Directory Server through the Preferences instantiates an LDAP Address
Book in the AB left-pane. The quick search feature works in this patch. Also,
adding a filter does not cause any problem with the quick search feature in the
LDAP Address Book. The filter is ignored. The only issue I see is the "position"
attribute determines where in the left-pane the AB appears. As the Directory
Server entry is written with no "position" value, then it can appear first,
before the traditional Personal Address Book. A second entry appears between the
PAB and the Collected Addresses. The delete for the query results now works in
MAB but this feature is now exposed for read-only LDAP Address Books.

Comment 37

[(not reading, please use [e-mail] instead)]

> I thought you might be interested in some feedback.

definitely, I appreciate the feedback.

> The only issue I see is the "position" attribute

I completely forgot about position.  I'll work on that.

> The delete for the query results now works in
> MAB but this feature is now exposed for read-only LDAP Address Books.

I'll fix that, too.  There's a bunch of UI that needs to disable based on the
current nsIAbDirectory operations attribute.

Comment 38

[(not reading, please use [e-mail] instead)]

Attachment: updated patch, not final yet

this patch includes the removal of some hash table bloat, and some helper
classes to deal with it.  PropertyPtrArraysToHashtable and
HashtableToPropertyPtrArrays (from nsAbUtils.*) are gone, replaced with a
simple scriptable interface (nsIAbDirectoryProperties) that I use to pass
around
the wstring and two strings we need.  (it can always be extended if we ever
need more.)

I'm still testing, and I still need to solve the position problem that John
pointed out.

Comment 39

[John Marmion]

The patch is coming along nicely. The creation of the DirectoryProperties has
simplified the code. On the position, it might simply be enough to set this to
some known large value for now. To do this correctly, you probably would need to
unify the creation of Address Books and Directory Servers. You will be returning
to this in (7) of comment #29.

Comment 40

[(not reading, please use [e-mail] instead)]

ugh.  the addressbook is an onion.  there's always more to peel, and it makes 
you cry when you cut into it.

the way position appears to work is that when we call DIR_GetDirectories() the 
first time, we'll generate the list of servers.  there appears to be code in 
nsDirPrefs.cpp to ensure that the PAB is first.

I'm considering letting it stay broken until I can fix it, and how we sort 
addressbooks in the directory pane (and other UI elements).  there should be a 
primary sort type (forcing PAB and CAB first), and then doing the rest by type 
(local, then ldap, then outlook, etc.).  within each type, we should be sorting 
alphabetically.  [there is a bug to make it so when you add new addressbooks, 
they should show up in the right sorted order, not in the order they were 
added.]

I'm going to spin off this mess into another bug.

Comment 41

[(not reading, please use [e-mail] instead)]

Attachment: more cleanup, includes fix for #121709 as well.

Comment 42

[(not reading, please use [e-mail] instead)]

Attachment: more cleanup, includes fix for #121709 as well.

Comment 43

[(not reading, please use [e-mail] instead)]

I'll attach a new patch soon.  I've landed a few of the fixes that were part of 
that patch already, so I'll need a new patch to start the review process.

Comment 44

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 45

[(not reading, please use [e-mail] instead)]

Attachment: patch, removed the non-addrbook parts.

Comment 46

[John Marmion]

Built this patch today. The LDAP AB appears to be working fine. I have a couple
of minor observations which are not pressing but can be looked at as this
progresses.

I have mentioned the deletion of LDAP entries causing some grief but the other
related issue is that the LDAP Address Book itself can be deleted from within
the AB. This can cause Mozilla to crash. It is not a consistent crash. The other
minor observation related to this is that the nsDirPrefs code deletes the
preferences entries for the deleted Directory Server but sets the position
attribute to zero. Any subsequent attempt to create and instantiate an identical
Directory Server at a later stage will fail. This can be looked at when we
revisit the position attribute.   

Comment 47

[(not reading, please use [e-mail] instead)]

John, thanks for going the extra mile and testing my patch.  It is greatly 
appreciated.

> I have mentioned the deletion of LDAP entries causing some grief but the other
> related issue is that the LDAP Address Book itself can be deleted from within
> the AB. This can cause Mozilla to crash. It is not a consistent crash.

I see if I can track that down before I check in.
 
> The other
> minor observation related to this is that the nsDirPrefs code deletes the
> preferences entries for the deleted Directory Server but sets the position
> attribute to zero. Any subsequent attempt to create and instantiate an 
identical
> Directory Server at a later stage will fail. This can be looked at when we
> revisit the position attribute.

I think this will be fixed when we fix http://bugzilla.mozilla.org/show_bug.cgi?
id=116449

since that can cause failure, I'll move it in from mozilla 1.2 (to 0.9.9) and 
fix it.  should be easy (famous last words)   

Comment 48

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 66599 [details] [diff] [review]
patch, removed the non-addrbook parts.

Looks good; almost all of these comments are quite minor...

nsIAbDirectory.idl
------------------
a)  how about using AString for description instead of wstring?

nsAbBSDirectory.cpp
-------------------
b) Couldn't this really be strcmp() instead of strstr()?

+      // check: this is a 4.x file, remove when conversion is done
+      PRUint32 fileNameLen = strlen(server->fileName);
+      if ((fileNameLen > kABFileName_PreviousSuffixLen) && 
+	   strstr(server->fileName + fileNameLen -
kABFileName_PreviousSuffixLen, kABFileName_PreviousSuffix))

c)
-				URIUTF8 = kMDBDirectoryRoot;
-				URIUTF8.Append (server->fileName);
+	 URI = kMDBDirectoryRoot;
+	 URI.Append (server->fileName);

It may save a bit of time to do this:

+	 URI = kMDBDirectoryRoot + server->fileName;

d)

+      if ((URI.Length() > kABFileName_PreviousSuffixLen) && 
+	 strstr(URI.get() + URI.Length() - kABFileName_PreviousSuffixLen,
kABFileName_PreviousSuffix) &&
+	 strstr(URI.get(), kMDBDirectoryRoot)) {

The following seems like it would be cleaner and would avoid the
unnecessary strstr() calls as well:

if ( Substring(URI, URI.Length() - kABFileName_PreviousSuffixLen,
	       kABFileName_PreviousSuffixLen).
					  Equals(kABFileName_PreviousSuffix) {

e) why is is_migrating to DIR_AddNewAddressBook set to PR_FALSE in the
new code but not the old?

f) Instead of:

+	nsCAutoString URI;
+	URI = kMDBDirectoryRoot;
+	URI += server->fileName;

How about:

nsCAutoString URI(kMDBDirectoryRoot + server->fileName);

g) strstr() seems like overkill here too; shouldn't Substring and .Equals be
   sufficient?

+	if (strstr(uri, kMDBDirectoryRoot)) // for moz-abmdbdirectory://
+		fileName = &(uri[kMDBDirectoryRootLen]);

nsAbDirProperty.cpp
---------------------
h) why do this?

-/* readonly attribute long operations; */
 NS_IMETHODIMP nsAbDirProperty::GetOperations(PRInt32 *aOperations)
 {

i) I think NS_INIT_ISUPPORTS() is prefered to NS_INIT_REFCNT() these days:

+nsAbDirectoryProperties::nsAbDirectoryProperties(void)
+{
+	NS_INIT_REFCNT();
+}

nsAbLDAPDirFactory.cpp
----------------------
j) see point f re this code:

+      nsCAutoString bridgeURI;
+      bridgeURI = "moz-abldapdirectory://";
+      bridgeURI += prefName.get();

YYY k) "bridgeURI"?  more comments?

nsAbLDAPDirectory.cpp
--------------------- 
l) This strlen() info could be calculated at compile-time instead of
run-time, I think.  Using NS{_NAMED}_LITERAL_CSTRING would be one way
to do this.  You could also then do this as a concatenation in the
constructor param list rather than as separate statements.

+    nsCAutoString prefName;
+    prefName = mURINoQuery.get() + strlen("moz-abldapdirectory://");
+    prefName += ".uri";

nsAbMDBDirFactory.cpp
---------------------
m) In the following code:

+      nsCAutoString fileName;
+      nsCAutoString uriStr;
+      uriStr = uri;
+      
+      if (uriStr.Find(kMDBDirectoryRoot) == 0) // for moz-abmdbdirectory://
+	   uriStr.Right(fileName, uriStr.Length() - kMDBDirectoryRootLen);

if uriString is made to be an nsDependentCString, a copy is avoided.
Additionally, it looks like Substring().Equals() could be used instead
of .Find(), which would make the failure case faster.

nsAddressBook.cpp
-----------------
n) couldn't strstr() actually be strcmp()?

+		 /* check: this is a 4.x file, remove when conversion is done
*/
+		 PRUint32 fileNameLen = strlen(server->fileName);
+		 if ((fileNameLen > kABFileName_PreviousSuffixLen) && 
+		      strstr(server->fileName + fileNameLen -
kABFileName_PreviousSuffixLen, kABFileName_PreviousSuffix))

nsDirPrefs.cpp
--------------
o) see previous comment re:

+	 // determine if server->fileName ends with ".na2"
+	 PRUint32 fileNameLen = strlen(server->fileName);
+	 if ((fileNameLen > kABFileName_PreviousSuffixLen) && 
+	     strstr(server->fileName + fileNameLen -
kABFileName_PreviousSuffixLen, kABFileName_PreviousSuffix))

p) Instead of .Cut and .Append, how about using .Replace()?  Also, if you
make kABFileName_Prefix an NS_NAMED_LITERAL_CSTRING(), then you'll
have a compile-time calculated .Length() rather than a run-time
calculated strlen() call: 

+					name.Cut(pos,
PL_strlen(kABFileName_PreviousSuffix));
+					name.Append(kABFileName_CurrentSuffix);

q) Why do we need two separate types here?

-	   server->dirType == FixedQueryLDAPDirectory)	
+	   server->dirType == FixedQueryLDAPDirectory ||  // this one might go
away
+	   server->dirType == LDAPDirectory)

Comment 49

[Dan Mosedale (:dmosedale, :dmose)]

Comment k was intended to say something along the lines of: it's not likely to
be clear to the naive coder what the "bridge" here is referring to; can we come
up with a better name for this and/or add some comments talking about what's
going on here?

Comment 50

[(not reading, please use [e-mail] instead)]

thanks for the feedback.  I'll be out for a week, but when I get back, I'll
address your comments and work to land this into the trunk.

I estimate it will be landed by Feb 8th, and will be part of 0.9.9

when I land, I'll start a list of known issues as spin off bugs.

Comment 51

[(not reading, please use [e-mail] instead)]

Attachment: patch which addresses dmose's coments

thanks for the review, dmose.  I've addressed most of your comments.  here are
my replies to the ones I didn't address:

> e) why is is_migrating to DIR_AddNewAddressBook set to PR_FALSE in the
> new code but not the old?

none of the callers to that method was adding ("migration","true") to the hash
table,
so I hard coded it to PR_FALSE.  note, there are other direct callers to
DIR_AddNewAddressBook()
that pass in other values for is_migrating.


> why do this?
> -/* readonly attribute long operations; */
> NS_IMETHODIMP nsAbDirProperty::GetOperations(PRInt32 *aOperations)
> {

I realize that those comments come from what the .idl generates, but since our
interfaces are not
frozen, I've seen those comments bit rot, so I removed it.  In this particular
instance,
I was planning on adding a typedef for operation type, I just never got around
to it.


> p) Instead of .Cut and .Append, how about using .Replace()?  Also, if you
> make kABFileName_Prefix an NS_NAMED_LITERAL_CSTRING(), then you'll
> have a compile-time calculated .Length() rather than a run-time
> calculated strlen() call: 

that code wasn't compiled, so I removed it.


> q) Why do we need two separate types here?
> 
> -	   server->dirType == FixedQueryLDAPDirectory)	
> +	   server->dirType == FixedQueryLDAPDirectory ||  // this one might go
> away
> +	   server->dirType == LDAPDirectory)  

right now, we'll still support anyone who has manually added prefs
moz-abldapdirectory://

that would be something that only shows up in addressbook, but not in the
autocomplete prefs.
if we decide we never want that, I can remove that line.

Comment 52

[(not reading, please use [e-mail] instead)]

hold off on testing / reviewing that patch, I'm testing it now and I need to 
fix something.

Comment 53

[(not reading, please use [e-mail] instead)]

false alarm, I was testing my tip tree build, instead of my tree with the 
patches.

I've sent mail to dmose for review.

Comment 54

[(not reading, please use [e-mail] instead)]

Attachment: extended the fix to also handle ldaps:// urls

Comment 55

[(not reading, please use [e-mail] instead)]

with some debugging help from bienvenu, I've fixed the crash that john reported:

>This can cause Mozilla to crash. It is not a consistent crash. 

I'll attach a new patch.  the related changes are in nsDirPrefs.cpp

I've also fixed the "setting position to zero" problem:

> The other minor observation related to this is that the nsDirPrefs code 
> deletes the preferences entries for the deleted Directory Server but sets the 
> position attribute to zero. 

see bug #116449

Comment 56

[(not reading, please use [e-mail] instead)]

Attachment: fix crasher when deleting directory with "abook.mab" as file

same as last patch, with additional changes to nsDirPrefs.cpp to prevent this
crash.

Comment 57

[John Marmion]

Built Mozilla today with this latest patch and spent some time testing it. It
appears to be working very well. The remaining outstanding issues have been
documented. I also notice that there exists a bug #83114 which might allow you
transfer some/all of those remaining issues including the "position" and
notification issues to it. 

Comment 58

[Dan Mosedale (:dmosedale, :dmose)]

>
>  		/* set default personal address book file name*/
> -		if (server->position == 1)
> +		if ((server->position == 1) && (server->dirType != LDAPDirectory))

This should probably be changed to either include all acceptible tables, or
exclude all non-acceptible types.  

Fix that, and you've got r=dmose.

Comment 59

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 68060 [details] [diff] [review]
fix crasher when deleting directory with "abook.mab" as file

r=[e-mail]

Comment 60

[(not reading, please use [e-mail] instead)]

I've changed it to be:

+ if ((server->position == 1) && (server->dirType == PABDirectory))

thanks for the review.

Comment 61

[David :Bienvenu]

Comment on attachment 68060 [details] [diff] [review]
fix crasher when deleting directory with "abook.mab" as file

sr=bienvenu

Comment 62

[(not reading, please use [e-mail] instead)]

fixed.

Comment 63

[yulian chang]

20020225 builds on all platforms.
Verified working.