Create and populate nsIAbCollection

RESOLVED FIXED in Thunderbird 3.0a3

Status

MailNews Core
Address Book
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 3.0a3

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
The initial version of nsIAbCollection will contain the following methods/attributes:

readOnly
isRemote
isSecure
cardForEmail()
cardForProperty() (if it gets there first)

This will replace the operations, isRemote, and isSecure attributes, and the cardForEmail and cardForProperty methods from nsIAbDirectory.
(Assignee)

Comment 1

9 years ago
Preliminary changes will be up on my address book rewrite repository as I make them. <http://hg.mozilla.org/users/Pidgeot18_gmail.com/ab_rewrite/>, branch "nsIAbCollection". Feel free to comment before I put up patches if you see anything troubling on those changes (especially in the realm of documentation).
(Assignee)

Comment 2

9 years ago
Created attachment 338375 [details] [diff] [review]
patch, version 1

I'm working on this next, as this would make my repo management a little less nebulous.

This does what comment 0 says it should do.
Attachment #338375 - Flags: review?(bugzilla)
Comment on attachment 338375 [details] [diff] [review]
patch, version 1

The only thing I'm concerned about here at the moment is loosing the read/search clarification that operations currently gives us for LDAP.

I'm not entirely sure its worth keeping it though. Maybe isRemote is enough for now.

I definitely like the .readOnly addition.

-  do_check_eq(AB.operations, abConfig.operations);
+  do_check_eq(AB.readOnly, abConfig.readOnly);
   do_check_eq(AB.dirName, abConfig.dirName);

I think we should add a test for .generateName as well (so that we don't break it).

Please also add a .readOnly check for ldap in test_ldap.js

I'd like to give it very quick once over once you've fixed the bustage/bitrot (hence the r-), but otherwise I'm happy.
Attachment #338375 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 4

9 years ago
Created attachment 339695 [details] [diff] [review]
Patch, version 2

Bitrots are fun, aren't they?

I've also added the tests.
Attachment #338375 - Attachment is obsolete: true
Attachment #339695 - Flags: review?(bugzilla)
Comment on attachment 339695 [details] [diff] [review]
Patch, version 2

On a card I opened up from LDAP (and OS X book), I'm seeing:

JavaScript strict warning: chrome://messenger/content/addressbook/abCardOverlay.js, line 233: reference to undefined property directory.operations
JavaScript error: chrome://messenger/content/addressbook/abCardOverlay.js, line 233: directory.operations is undefined
Attachment #339695 - Flags: review?(bugzilla) → review-
Comment on attachment 339695 [details] [diff] [review]
Patch, version 2

+NS_IMETHODIMP nsAbDirProperty::GetReadOnly(PRBool *aReadOnly)
+{
+  NS_ENSURE_ARG_POINTER(aReadOnly);
+  // Default is that we are writable. Any implementation that is read-only must
+  // override the method.
+  *aReadOnly = PR_FALSE;
+	return NS_OK;
+}
+

nit: no tab please.
(Assignee)

Comment 7

9 years ago
Created attachment 339852 [details] [diff] [review]
Patch, version 3
Attachment #339852 - Flags: review?(bugzilla)
(Assignee)

Comment 8

9 years ago
Comment on attachment 339695 [details] [diff] [review]
Patch, version 2

Accidentally hit enter too early...
Attachment #339695 - Attachment is obsolete: true
Attachment #339852 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

9 years ago
Attachment #339852 - Flags: superreview?(bienvenu)

Comment 9

9 years ago
Comment on attachment 339852 [details] [diff] [review]
Patch, version 3

some nits:

if we support n e-mail addresses per card, it would work for any of them, right? Maybe "secondary" captures that but it's not quite explicit.

+   * @param  emailAddress The email address to find in either the primary or
+   *                      secondary email address fields. If email address is
+   *                      empty, the database won't be searched and the function
+   *                      will return as if no card was found.

typo - should be "case-sensitively"

+   * @param  aCaseSensitive True if matching should be done case-senstively.

I think "override this method".
+  // Default is that we are writable. Any implementation that is read-only must
+  // override the method.


if this diff isn't ignoring whitespace changes, can you fix the tabs here:

class nsAbDirProperty: public nsIAbDirectory
 {
 public: 
 	nsAbDirProperty(void);
 	virtual ~nsAbDirProperty(void);
 
 	NS_DECL_ISUPPORTS
+  NS_DECL_NSIABITEM
+  NS_DECL_NSIABCOLLECTION
 	NS_DECL_NSIABDIRECTORY
Attachment #339852 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 10

9 years ago
Checked in with nits, changeset dbc99e82183a.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Target Milestone: --- → Thunderbird 3.0a3
You need to log in before you can comment on or make changes to this bug.