Closed
Bug 83091
Opened 24 years ago
Closed 23 years ago
Need to modify the addressbook to read Autocomplete entries for LDAP servers
Categories
(SeaMonkey :: MailNews: Address Book & Contacts, defect)
SeaMonkey
MailNews: Address Book & Contacts
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.9
People
(Reporter: martin.maher, Assigned: sspitzer)
References
(Blocks 1 open bug)
Details
(Whiteboard: nab-ldap)
Attachments
(2 files, 17 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
dmosedale
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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•24 years ago
|
||
What exactly do you need to change?
Comment 2•24 years ago
|
||
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•24 years ago
|
||
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.
Assignee: dmose → csaba.borbola
Comment 4•24 years ago
|
||
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•24 years ago
|
||
Comment 6•24 years ago
|
||
Doesn't look like this is getting fixed before the freeze tomorrow night.
Pushing out a milestone. Please correct if I'm mistaken.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Comment 7•24 years ago
|
||
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 9•24 years ago
|
||
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•24 years ago
|
||
Comment 11•23 years ago
|
||
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.
Updated•23 years ago
|
Keywords: nsenterprise+
Comment 12•23 years ago
|
||
Moving to 0.9.5 per PDT, and marking to nsenterprise-
Keywords: nsenterprise+ → nsenterprise-
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Comment 13•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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".
Severity: normal → major
Comment 16•23 years ago
|
||
0.9.5 is out the door. bumping up the TM one notch
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Comment 17•23 years ago
|
||
Passing the ownership to John Marmion@sun, because I was moved to another
project.
Assignee: csaba.borbola → john.marmion
Updated•23 years ago
|
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Updated•23 years ago
|
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Comment 18•23 years ago
|
||
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.
Attachment #36648 -
Attachment is obsolete: true
Attachment #46108 -
Attachment is obsolete: true
Comment 19•23 years ago
|
||
Note this patch was built against the head on 17th December 2001.
Attachment #62369 -
Attachment is obsolete: true
Comment 20•23 years ago
|
||
Update patch following major Address Book changes following bug #73868 and bug
#83023. Also, change URI scheme from "moz-abldapdirectory://" to "ldap://" .
Attachment #62524 -
Attachment is obsolete: true
Assignee | ||
Comment 21•23 years ago
|
||
john, as soon as I land my LDIF export code, I'll start taking a look at your
patch.
Comment 22•23 years ago
|
||
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.
Assignee | ||
Comment 23•23 years ago
|
||
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.
Assignee: john.marmion → sspitzer
Assignee | ||
Comment 24•23 years ago
|
||
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.
Assignee | ||
Comment 25•23 years ago
|
||
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.
Attachment #64998 -
Attachment is obsolete: true
Comment 26•23 years ago
|
||
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•23 years ago
|
||
Attachment #64143 -
Attachment is obsolete: true
Assignee | ||
Comment 28•23 years ago
|
||
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.
Attachment #65000 -
Attachment is obsolete: true
Assignee | ||
Comment 29•23 years ago
|
||
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
Status: NEW → ASSIGNED
Comment 30•23 years ago
|
||
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•23 years ago
|
||
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?
Assignee | ||
Comment 32•23 years ago
|
||
moving to 0.9.9, these aren't 0.9.8 stoppers.
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Assignee | ||
Comment 33•23 years ago
|
||
this is something we want for nsbeta1, nominating.
Keywords: nsbeta1
Assignee | ||
Comment 34•23 years ago
|
||
updated patch. this patch also skips LDAP directories during local
autocomplete.
Attachment #65374 -
Attachment is obsolete: true
Assignee | ||
Comment 35•23 years ago
|
||
this new patch (not final yet) includes a fix for #117452. this patch gets
delete working after a quick search (on local addressbooks only)
Attachment #65932 -
Attachment is obsolete: true
Comment 36•23 years ago
|
||
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.
Assignee | ||
Comment 37•23 years ago
|
||
> 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.
Assignee | ||
Comment 38•23 years ago
|
||
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.
Attachment #65938 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Whiteboard: nab-ldap
Comment 39•23 years ago
|
||
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.
Assignee | ||
Comment 40•23 years ago
|
||
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.
Assignee | ||
Comment 41•23 years ago
|
||
Attachment #66236 -
Attachment is obsolete: true
Assignee | ||
Comment 42•23 years ago
|
||
Attachment #66375 -
Attachment is obsolete: true
Assignee | ||
Comment 43•23 years ago
|
||
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.
Assignee | ||
Comment 44•23 years ago
|
||
Attachment #66377 -
Attachment is obsolete: true
Assignee | ||
Comment 45•23 years ago
|
||
Attachment #66598 -
Attachment is obsolete: true
Comment 46•23 years ago
|
||
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.
Assignee | ||
Comment 47•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
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?
Assignee | ||
Comment 50•23 years ago
|
||
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.
Assignee | ||
Comment 51•23 years ago
|
||
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.
Attachment #66599 -
Attachment is obsolete: true
Assignee | ||
Comment 52•23 years ago
|
||
hold off on testing / reviewing that patch, I'm testing it now and I need to
fix something.
Assignee | ||
Comment 53•23 years ago
|
||
false alarm, I was testing my tip tree build, instead of my tree with the
patches.
I've sent mail to dmose for review.
Assignee | ||
Comment 54•23 years ago
|
||
Attachment #67938 -
Attachment is obsolete: true
Assignee | ||
Comment 55•23 years ago
|
||
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
Assignee | ||
Comment 56•23 years ago
|
||
same as last patch, with additional changes to nsDirPrefs.cpp to prevent this
crash.
Attachment #68021 -
Attachment is obsolete: true
Comment 57•23 years ago
|
||
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•23 years ago
|
||
>
> /* 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•23 years ago
|
||
Comment on attachment 68060 [details] [diff] [review]
fix crasher when deleting directory with "abook.mab" as file
r=dmose@netscape.com
Attachment #68060 -
Flags: review+
Assignee | ||
Comment 60•23 years ago
|
||
I've changed it to be:
+ if ((server->position == 1) && (server->dirType == PABDirectory))
thanks for the review.
Comment 61•23 years ago
|
||
Comment on attachment 68060 [details] [diff] [review]
fix crasher when deleting directory with "abook.mab" as file
sr=bienvenu
Attachment #68060 -
Flags: superreview+
Assignee | ||
Comment 62•23 years ago
|
||
fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 63•23 years ago
|
||
20020225 builds on all platforms.
Verified working.
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•