Closed Bug 124007 Opened 18 years ago Closed 17 years ago

LDAP addressbooks appear above personal addressbook / collected addressbook in the directory pane

Categories

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

x86
All
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.4alpha

People

(Reporter: sspitzer, Assigned: cavin)

References

Details

(Whiteboard: [adt2], nab-ldap [ue1])

Attachments

(1 file, 7 obsolete files)

LDAP addressbooks appear above personal addressbook / collected addressbook in
the directory pane

they need to appear after all the local addressbooks.
Whiteboard: nab-ldap
I think the position pref needs to go away. 

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.]
*** Bug 124201 has been marked as a duplicate of this bug. ***
Local Address Books always first.
    PAB - always at top
    CAB - #2 position
    Any user created Local ABs in Alphabetical order

LDAP Directories follow all LAB's
    Added in alphabetical order
taking
Assignee: sspitzer → shliang
Attached patch patch (obsolete) — Splinter Review
rewrote DIR_SortServers
UI doesn't match spec, not having addressbooks correctly ordered makes it hard 
to locate the one you are looking for.
Keywords: nsbeta1
Whiteboard: nab-ldap → [adt2 rtm], nab-ldap
instead of this approach, I recommend doing what we do in the folder pane.

rely on the xul (or outliner builder?) sort service. 

The trick to making things show up in the desired order is to use a prefix, like:

0Personal Address Book
1Collected Address Book
2MAB1
2MAB2
 LIST1
 LIST2
3LDAP1
3LDAP2

The existing way can be scrapped.  The good thing about doing it this way, is
we'll get other bugs ("mailing lists should appear sorted, not in order they are
created") for free.

keep in mind, addressbook pretty names are PRUnichar*, like foldernames, so
you'll need to do the same thing we did there.

let me know if you have questions.
Attached patch patch (obsolete) — Splinter Review
Attachment #82505 - Attachment is obsolete: true
Attached patch additional patch (obsolete) — Splinter Review
sort all the menulists with ab directories in them as well (new card dialog,
new list dialog, search, etc.)
Comment on attachment 84344 [details] [diff] [review]
patch

nice job on the first patch.  just a few minor comments:

1)

@@ -1172,7 +988,7 @@
	 * needs to be re-sorted.
	 */
	if (resort)
-		resort = DIR_SortServersByPosition(wholeList);
+		//resort = DIR_SortServersByPosition(wholeList);

this won't cause the next line after the if (resort) to be considered part of
the if block, will it?
I'd just say remove the if (resort) as well, and probably the comment above it.


2)
	/* Make sure our position changes get saved back to prefs
	 */
@@ -3424,7 +3240,7 @@
		DIR_DeleteServerList(obsoleteList);

	/* Sort the list to make sure it is in order */
-	DIR_SortServersByPosition(*list);
+	//DIR_SortServersByPosition(*list);

same thing. unless there is a reason why you commented out instead of removing
it,
just remove this and the comment above it.

3)

+  char *uri;
+  resource->GetValue(&uri);

You're leaking uri.  instead of GetValue. do GetValueConst(), like this:

   const char *uri = nsnull;
   rv = resource->GetValueConst(&uri);
   NS_ENSURE_SUCCESS(rv,rv);

then, you don't have to free uri.

4)

instead of PL_strcmp(), use strcmp()
and instead of these strings, use the #defines defined in nsDirPrefs.h

nsDirPrefs.h:#define kPersonalAddressbookUri   
"moz-abmdbdirectory://abook.mab"
nsDirPrefs.h:#define kCollectedAddressbookUri  
"moz-abmdbdirectory://history.mab"

+  if (PL_strcmp(uri, "moz-abmdbdirectory://abook.mab") == 0)
+    sortString.AppendInt(0);
+  else if (PL_strcmp(uri, "moz-abmdbdirectory://history.mab") == 0)
+    sortString.AppendInt(1);

5)

instead of PL_strnstr, I suggest strncmp() and use these #defines from
nsDirPrefs.h

#define kMDBDirectoryRoot	   "moz-abmdbdirectory://"
#define kMDBDirectoryRootLen	   21

#define kLDAPDirectoryRoot	   "moz-abldapdirectory://"
#define kLDAPDirectoryRootLen	   22


+  else if (PL_strnstr(uri, "moz-abmdb", 9) == uri)
+    sortString.AppendInt(2);  
+  else if (PL_strnstr(uri, "moz-abldap", 10) == uri)
+    sortString.AppendInt(3);


6)

+
+  rv = createNode(ToNewUnicode(sortString + name), target);
+

createNode() takes a const PRUnichar *.  so, this will leak since ToNewUnicode
will allocate.

One way around this would be do to this:

+ sortString += name;
+ rv = createNode(sortString.get(), target);
+ NS_ENSURE_SUCCESS(rv,rv);


7)

can you add a comment, about the if () block, about how using 0 for PAB, 1 for
CAB, 2 for all other local abs and 3 for ldap abs gives us the desired sort? 
Something, like this:

0Personal Address Book
1Collected Address Book
2MAB1
 2MAB1LIST1
 2MAB1LIST2
2MAB2
 2MAB2LIST1
 2MAB2LIST2
3LDAP1
3LDAP2
Attachment #84344 - Flags: needs-work+
Comment on attachment 85359 [details] [diff] [review]
additional patch

r=sspitzer.  nice work.
Attachment #85359 - Flags: review+
Attached patch revised patch (obsolete) — Splinter Review
Attachment #84344 - Attachment is obsolete: true
Attachment #85359 - Attachment is obsolete: true
Comment on attachment 86476 [details] [diff] [review]
revised patch

r=sspitzer

looks good.

this will improve the addressbook UI, for those who use LDAP.
Attachment #86476 - Flags: review+
something I forgot to ask in my review:

right now, we don't support renaming of addressbooks (but we will as soon as 
#17230 is fixed.)

with this sorting code, what happens when I create a new local addressbook, 
import a new addressbook, create a new ldap addressbook, add a mailing list, or 
rename a mailing list?  Does everything stay properly sorted?
Whiteboard: [adt2 rtm], nab-ldap → [adt2 rtm], nab-ldap [ue2]
Blocks: 156960
Whiteboard: [adt2 rtm], nab-ldap [ue2] → [adt2 rtm], nab-ldap [ue1]
Note, this applies to the Select Addresses dialog as well.
QA Contact: nbaca → gchan
Attached patch patch (obsolete) — Splinter Review
this patch combines work for this bug and bug 17230 (renaming addressbooks),
also includes srilatha's patch for bug 124059 (adding ldaps so that they show
up right away)
Attachment #86476 - Attachment is obsolete: true
Attachment #108776 - Flags: review?(sspitzer)
taking.  I'm going to work on driving this into the trunk, while shuehan focuses
on browser.
Assignee: shliang → sspitzer
Mail triage team: nsbeta1+/adt2
Keywords: nsbeta1nsbeta1+
Whiteboard: [adt2 rtm], nab-ldap [ue1] → [adt2], nab-ldap [ue1]
over to cavin.

we should take this one step at a time, and fix #124059 and #17230 first
Assignee: sspitzer → cavin
Depends on: 17230, 124059
Comment on attachment 108776 [details] [diff] [review]
patch

needs work.
Attachment #108776 - Flags: review?(sspitzer) → review-
Target Milestone: --- → mozilla1.4alpha
Comment on attachment 108776 [details] [diff] [review]
patch

this code has either landed in another form, or is being done slightly
differently by cavin.

(we now use the tree builder, thanks to shuehan)
Attachment #108776 - Attachment is obsolete: true
Attached patch Proposed patch, v1 (obsolete) — Splinter Review
This patch does not address the following issues and I will log bugs for each
of them:

1. If you rename an addrbook, the renamed addrbook is not sorted correctly (ie,
the addrbook is not re-positioned).

2. Under Peferences|Addressing the Directory Server list (ie, LDAP) is not
sorted correctly. This is because the list is created by LoadDirectories() js
function.
Attachment #117039 - Flags: superreview?(sspitzer)
Comment on attachment 117039 [details] [diff] [review]
Proposed patch, v1

looks good, except for one problem, your sort will not be i18n friendly.

instead of:

+  sortString += name;
+  rv = createNode(sortString.get(), target);

look at nsMsgFolderDataSource::createFolderNameNode() and
nsMsgFolder::GetSortKey().

notice that for folder name sorting, we can't use the actual name.

we basically do:  rdf literal("special int" + "collation key for name")

you will need to do something similar for addressbook name sorting.
Attachment #117039 - Flags: superreview?(sspitzer) → superreview-
Attached patch Proposed patch, v2 (obsolete) — Splinter Review
Use collation key for sorting.
Attachment #117039 - Attachment is obsolete: true
Attachment #117505 - Flags: superreview?(sspitzer)
1)

how about:
 
+nsresult nsAbRDFDataSource::createBlobNode(PRUint8 *value, PRUint32 &length,
nsIRDFNode **node, nsIRDFService *rdfService)
+{
+  NS_ENSURE_ARG_POINTER(node);
+  NS_ENSURE_ARG_POINTER(rdfService);
+
+  *node = nsnull;
+  nsCOMPtr<nsIRDFBlob> blob;
+  nsresult rv = rdfService->GetBlobLiteral(value, length, getter_AddRefs(blob));
+  NS_ENSURE_SUCCESS(rv,rv);
+  NS_IF_ADDREF(*node = blob);
+  return rv;
+}

this is probably code copied from the folder ds.  if so, can you fix it there, too?

2)

+  nsresult rv;
+
+  nsXPIDLString name;
+  rv = directory->GetDirName(getter_Copies(name));
+	NS_ENSURE_SUCCESS(rv, rv);

instead:

+  nsXPIDLString name;
+  nsresult rv = directory->GetDirName(getter_Copies(name));
+	NS_ENSURE_SUCCESS(rv, rv);


3)

I think this can be optimized.

+  nsAutoString sortString;
+  if (strcmp(uri, kPersonalAddressbookUri) == 0)
+    sortString.AppendInt(0);  // Personal addrbook
+  else if (strcmp(uri, kCollectedAddressbookUri) == 0)
+    sortString.AppendInt(1);  // Collected addrbook
+  else if (dirType == PABDirectory)
+    sortString.AppendInt(2);  // Normal addrbook  
+  else if (dirType == LDAPDirectory)
+    sortString.AppendInt(3);  // LDAP addrbook
+  else if (dirType == MAPIDirectory)
+    sortString.AppendInt(4);  // MAPI addrbook
+  else
+    sortString.AppendInt(5);  // everything else comes last


how about:

nsAutoString sortString;
if (dirType == PABDirectory)
{
  if (strcmp(uri, kPersonalAddressbookUri) == 0)
    sortString.AppendInt(0);  // Personal addrbook
  else if (strcmp(uri, kCollectedAddressbookUri) == 0)
    sortString.AppendInt(1);  // Collected addrbook
  else
    sortString.AppendInt(2);  // Normal addrbook or mailing lists
}
else if (dirType == LDAPDirectory)
  sortString.AppendInt(3);  // LDAP addrbook
else if (dirType == MAPIDirectory)
  sortString.AppendInt(4);  // MAPI addrbook
else
  sortString.AppendInt(5);  // everything else comes last

that saves us 2 strcmps for ldap, at the cost of a int compare.

curious, do mailing lists map to 2 or 5?  (I think 2, so see my comment.)

4)

we should probably move this (and the existing) collation key code to baseutil
somewhere, and share it.

can you log a spin off bug?

c:\trees\trunk\mozilla\mailnews\addrbook\src\nsAbView.cpp(810):  rv =
mCollationKeyGenerator->GetSortKeyLen(kCollationCaseInSensitive, sourceString,
aKeyLen);
c:\trees\trunk\mozilla\mailnews\addrbook\src\nsAbView.cpp(817):  rv =
mCollationKeyGenerator->CreateRawSortKey(kCollationCaseInSensitive,
sourceString, *aKey, aKeyLen);
c:\trees\trunk\mozilla\mailnews\base\util\nsMsgFolder.cpp(2875):  rv =
kCollationKeyGenerator->GetSortKeyLen(kCollationCaseInSensitive, aSource, aLength);
c:\trees\trunk\mozilla\mailnews\base\util\nsMsgFolder.cpp(2885):  return
kCollationKeyGenerator->CreateRawSortKey(kCollationCaseInSensitive, aSource,
*aKey, aLength);
c:\trees\trunk\mozilla\mailnews\db\msgdb\src\nsMsgDatabase.cpp(3016):  err =
m_collationKeyGenerator->GetSortKeyLen(kCollationCaseInSensitive, sourceStr, len);
c:\trees\trunk\mozilla\mailnews\db\msgdb\src\nsMsgDatabase.cpp(3022):  err =
m_collationKeyGenerator->CreateRawSortKey(kCollationCaseInSensitive, sourceStr,
*result, len);

but don't bother fixing it now, it's not worth it.
Comment on attachment 117505 [details] [diff] [review]
Proposed patch, v2

will sr the final patch.
Attachment #117505 - Flags: superreview?(sspitzer) → superreview-
> we should probably move this (and the existing) collation key code to baseutil
> somewhere, and share it. can you log a spin off bug?
>
It's bug 197909.
Incorporated last comment.
Attachment #117505 - Attachment is obsolete: true
Comment on attachment 117527 [details] [diff] [review]
Proposed patch, v3

r/sr=sspitzer

nice cavin.  something the addrbook has needed for a long time.
Attachment #117527 - Flags: superreview+
Fix checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Yeeeah!! :-)
Trunk build 2003-03-19: Mac 10.1.5, WinXP, Linux RH 8
Verified Fixed. I tried adding a variety of address book in different orders
(i.e. creating a new address book, importing address books, creating LDAP
directories.) In all cases they appeared in the correct order where the user
defined address books and imported address books appeared in alphabetical order
and the LDAP directories always appeared last.
Status: RESOLVED → VERIFIED
QA Contact: gchan → nbaca
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.