Closed
Bug 124007
Opened 19 years ago
Closed 18 years ago
LDAP addressbooks appear above personal addressbook / collected addressbook in the directory pane
Categories
(SeaMonkey :: MailNews: Address Book & Contacts, defect)
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)
28.05 KB,
patch
|
sspitzer
:
superreview+
|
Details | Diff | Splinter Review |
LDAP addressbooks appear above personal addressbook / collected addressbook in the directory pane they need to appear after all the local addressbooks.
Reporter | ||
Updated•19 years ago
|
URL: http://http://
Whiteboard: nab-ldap
Reporter | ||
Comment 1•19 years ago
|
||
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.]
Reporter | ||
Comment 2•19 years ago
|
||
*** 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
Comment 6•19 years ago
|
||
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
Reporter | ||
Comment 7•19 years ago
|
||
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.
Attachment #82505 -
Attachment is obsolete: true
sort all the menulists with ab directories in them as well (new card dialog, new list dialog, search, etc.)
Reporter | ||
Comment 10•19 years ago
|
||
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+
Reporter | ||
Comment 11•19 years ago
|
||
Comment on attachment 85359 [details] [diff] [review] additional patch r=sspitzer. nice work.
Attachment #85359 -
Flags: review+
Comment 12•19 years ago
|
||
Attachment #84344 -
Attachment is obsolete: true
Attachment #85359 -
Attachment is obsolete: true
Reporter | ||
Comment 13•19 years ago
|
||
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+
Reporter | ||
Comment 14•19 years ago
|
||
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•19 years ago
|
||
Note, this applies to the Select Addresses dialog as well.
Updated•18 years ago
|
QA Contact: nbaca → gchan
Comment 16•18 years ago
|
||
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)
Reporter | ||
Comment 17•18 years ago
|
||
taking. I'm going to work on driving this into the trunk, while shuehan focuses on browser.
Assignee: shliang → sspitzer
Comment 18•18 years ago
|
||
Mail triage team: nsbeta1+/adt2
Reporter | ||
Comment 19•18 years ago
|
||
over to cavin. we should take this one step at a time, and fix #124059 and #17230 first
Reporter | ||
Comment 20•18 years ago
|
||
Comment on attachment 108776 [details] [diff] [review] patch needs work.
Attachment #108776 -
Flags: review?(sspitzer) → review-
Assignee | ||
Updated•18 years ago
|
Target Milestone: --- → mozilla1.4alpha
Reporter | ||
Comment 21•18 years ago
|
||
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
Assignee | ||
Comment 22•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Attachment #117039 -
Flags: superreview?(sspitzer)
Reporter | ||
Comment 23•18 years ago
|
||
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-
Assignee | ||
Comment 24•18 years ago
|
||
Use collation key for sorting.
Assignee | ||
Updated•18 years ago
|
Attachment #117039 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Attachment #117505 -
Flags: superreview?(sspitzer)
Reporter | ||
Comment 25•18 years ago
|
||
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.
Reporter | ||
Comment 26•18 years ago
|
||
Comment on attachment 117505 [details] [diff] [review] Proposed patch, v2 will sr the final patch.
Attachment #117505 -
Flags: superreview?(sspitzer) → superreview-
Assignee | ||
Comment 27•18 years ago
|
||
> 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.
Assignee | ||
Comment 28•18 years ago
|
||
Incorporated last comment.
Attachment #117505 -
Attachment is obsolete: true
Reporter | ||
Comment 29•18 years ago
|
||
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+
Assignee | ||
Comment 30•18 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 31•18 years ago
|
||
Yeeeah!! :-)
Comment 32•18 years ago
|
||
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
Updated•16 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•