This is a feature tracking bug for having per-server properties dialogs for LDAP servers.

p4

Phil found a way to weasel out of owning this bug. Reassigning.

Not currently on the radar. M20.

[LDAP] in summary

LDAP to M30, nobody, helpwanted

Assigning this to myself

Removing helpwanted. Setting Targe milestone to 0.9

Here is the spec. http://www.mozilla.org/mailnews/specs/addressbook/LDAP4.html The current implementation is according to option 3 in this spec with one modification. And it is the port# is in the Advanced pane.

alecf can you review the patches.

adding bhuvan and me to the cc list to help with the reviews.

forgot to add chuang

List of preferences that are saved when a new directory server is added ldap_2.servers.<server_name>.description ldap_2.servers.<server_name>.uri(uri is an ldap url created using nsLDAPURL) ldap_2.servers.<server_name>.maxHits(if different from default) ldap_2.servers.<server_name>.auth.enabled To enable autocomplete ldap_2.autoComplete.useDirectory needs to be set to true. The preference that is saved when a directory server is selected from the global preferences ldap_2.autoComplete.directoryServer ldap_2.autoComplete.skipDirectoryIfLocalMatchFound (is similar to 4.x) To override the directory server that is saved in the global preferences, the per identity preference identity.overrideGlobal_Pref needs to be set to true The preference that is saved when a directory server is selected from the Mail/News Account Settings (this is saved per identity) identity.directoryServer

some suggestions for preference locking: ldapPrefsOverlay.xul + <button id="editButton" position="8" + label="&editDirectories.label;" oncommand="onEditDirectories();"/> Add the pref attributes to allow locking. Possible prefstring could be something like: pref.ldap.disable_button.edit_directories The other elements that have the pref attributes will be automatically locked. The stuff in mail/news will need some modification, but I'll have suggestions when my stuff is worked out.

oops! created a wrong attachment

One buglet I noticed in the patch: Makefile (and therefore Makefile.in) on Unix actually require tabs instead of spaces (unlike everything else in the tree).

The above patch is part1 of the patch

Need to apply the patch from bug 77916 for this code to work

With the final patch(4/25) and the patch for 77916. Everything works. I don't see js warning. r=chuang.

r=bhuvan

+ item.setAttribute("label", "none"); so "none" shows up in the UI? it should be defined in a string bundle and not hard coded. also, can you add a comment to explain "validateDescription()" is doing? I'm a little lost. +function validateDescription() +{ + var re = /[A-Za-z0-9]/g; + var str = pref_string_desc.match(re); + var temp = ""; + + for(var count = 0; count < str.length && count <= 20; count++) + { + temp = temp + str[count]; + } use substring(), something like: var len = str.length; if (len > 20) len = 20; temp = str.substring(0, len); + pref_string_desc = temp; + while(temp) { + temp = ""; + try{ temp = prefInt.CopyCharPref (prefstring+pref_string_desc+".description");} catch(e){} + if(temp) + pref_string_desc = pref_string_desc + str[0]; + } +} is that always going to stop? what happens if str is ""?

two more suggestions: I'd use gFoo instead of foo for global variables. it helps code readablilty, and seems to be convention in mozilla. set "javascript.options.strict" to true in your prefs.js. it will help you find and fix any js warnings in your code (before you check in.)

The latest patch addresses the issues Seth pointed out. Using string bundle for menuitem none. validateDescription()- Wrong choice of function name. I have added comments and also changed the function name. can't use substring because str is an array after the call to match. + var str = pref_string_desc.match(re); Also addressed the case where str might be "" Added prefix g for all the global variables. Setting the preference javascript.options.strict has been very helpful. Thanks Seth. Discovered couple of warnings where the variables have not been declared. Fixed the warnings.

+ var len = str.length; + if(len > 20) len = 20; instead of: + for(var count = 0; count < len; count++) + { + temp = temp + str[count]; + } why not something like: temp = str.substring(0, len); + gPref_string_desc = temp; + while(temp) { + temp = ""; + try{ temp = gPrefInt.CopyCharPref(gPrefstring+gPref_string_desc+".description");} catch(e){} + if(temp) + gPref_string_desc = gPref_string_desc + str[0]; + } it looks like that code is to make gPref_string_desc unique, but checking if "ldap_2.servers.<gPref_string_desc>.description" exists. if not, if str[0] is a, it will try "ldap_2.servers.<gPref_string_desc>a.description", "ldap_2.servers.<gPref_string_desc>aa.description", "ldap_2.servers.<gPref_string_desc>aaa.description", etc. again, what happens if the user enters this for the description: "..." won't str be ""? that will cause str[0] to throw an exception, right?

sr=sspitzer you can fix those minor issues now, or after you land.

If the user enters "..." then str would be null because + var re = /[A-Za-z0-9]/g; + var str = gPref_string_desc.match(re); this would delete all the characters other than alphabets and digits from the description user entered.

right, so if str is null, won't gPref_string_desc = gPref_string_desc + str[0]; throw an exception? or, is it not possible to get there if str is null?

you are right, str could be null. Adding this one line inside if(!str){ ...... str = temp; ..... } would fix it. Will post a new patch with the change.

+ gPref_string_desc = gPref_string_desc + str[0]; why not: + gPref_string_desc += str[0]; + <script language="JavaScript" src="chrome://messenger/content/addressbook/pref-directory-add.js"></script> please use; + <script type="application/x-javascript" src="chrome://messenger/content/addressbook/pref-directory-add.js"/> looks like a tab: + <button label="&findButton.label;" accesskey="&findButton.accesskey;" disabled="true"/> + <box id="okCancelButtonsRight"/> +<!ENTITY okButton.label "OK"> Generally "OK" in a dtd should be a red flag. if i'm a translator i wouldn't want to have to translate OK again (although my tools can do the work for me), and if i'm a ui designer, i've already spec'd the correct way to do ok buttons. about your xul[xml]: +<window xmlns:html="http://www.w3.org/1999/xhtml" + xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" + xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" please try to be consistent when you align stuff. Your code is a mix of 2,3,4,other indentations (it looks like you prefer 2) I'd request that you wrap near 80 cols in all code including xul/js. adding dump()'s of late has been considered bad form, will the user benefit from them? + dump("in onSave() \n"); ^not very useful; v really not very useful. + //dump("in createDirectoriesList\n"); + if((gPref_string_desc != "") && (hostname != "")) { you could probably write: + if (gPref_string_desc && hostname) { personally, and possibly for most js modules it's |if (cond)|, |for (;;)| and |while (cond)| you're inconsistent even locally: + if(gRefresh) + { + var popup = document.getElementById("directoriesListPopup"); + if ( popup ) + { + while ( popup.childNodes.length ) None of my requests are binding, but most are reasonable.

O.k the final patch addresses all the issues and sugeestions from timeless. Will checkin once my build on mac is done.

Checked in the patch

Verified with 2001052404 build.

Verified with 2001052404 build.