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

Status: VERIFIED FIXED

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

Description

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

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

they need to appear after all the local addressbooks.

Comment 1

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

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.]

Comment 2

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

*** Bug 124201 has been marked as a duplicate of this bug. ***

Comment 3

[jglick]

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

Comment 4

[shliang]

taking

Comment 5

[shliang]

Attachment: patch

rewrote DIR_SortServers

Comment 6

[Lori Kaplan]

UI doesn't match spec, not having addressbooks correctly ordered makes it hard 
to locate the one you are looking for.

Comment 7

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

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.

Comment 8

[shliang]

Attachment: patch

Comment 9

[shliang]

Attachment: additional patch

sort all the menulists with ab directories in them as well (new card dialog,
new list dialog, search, etc.)

Comment 10

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

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

Comment 11

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

Comment on attachment 85359 [details] [diff] [review]
additional patch

r=sspitzer.  nice work.

Comment 12

[shliang]

Attachment: revised patch

Comment 13

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

Comment on attachment 86476 [details] [diff] [review]
revised patch

r=sspitzer

looks good.

this will improve the addressbook UI, for those who use LDAP.

Comment 14

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

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?

Comment 15

[jglick]

Note, this applies to the Select Addresses dialog as well.

Comment 16

[shliang]

Attachment: patch

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)

Comment 17

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

taking.  I'm going to work on driving this into the trunk, while shuehan focuses
on browser.

Comment 18

[Samir Gehani]

Mail triage team: nsbeta1+/adt2

Comment 19

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

over to cavin.

we should take this one step at a time, and fix #124059 and #17230 first

Comment 20

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

Comment on attachment 108776 [details] [diff] [review]
patch

needs work.

Comment 21

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

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)

Comment 22

[Cavin Song]

Attachment: Proposed patch, v1

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.

Comment 23

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

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.

Comment 24

[Cavin Song]

Attachment: Proposed patch, v2

Use collation key for sorting.

Comment 25

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

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 26

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

Comment on attachment 117505 [details] [diff] [review]
Proposed patch, v2

will sr the final patch.

Comment 27

[Cavin Song]

> 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.

Comment 28

[Cavin Song]

Attachment: Proposed patch, v3

Incorporated last comment.

Comment 29

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

Comment on attachment 117527 [details] [diff] [review]
Proposed patch, v3

r/sr=sspitzer

nice cavin.  something the addrbook has needed for a long time.

Comment 30

[Cavin Song]

Fix checked in.

Comment 31

[jglick]

Yeeeah!! :-)

Comment 32

[Ninoschka Baca]

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.