Closed Bug 450917 Opened 11 years ago Closed 11 years ago

Create and populate nsIAbCollection

Categories

(MailNews Core :: Address Book, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0a3

People

(Reporter: jcranmer, Assigned: jcranmer)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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).
Attached patch patch, version 1 (obsolete) — Splinter Review
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-
Attached patch Patch, version 2 (obsolete) — Splinter Review
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.
Attached patch Patch, version 3Splinter Review
Attachment #339852 - Flags: review?(bugzilla)
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+
Attachment #339852 - Flags: superreview?(bienvenu)
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+
Checked in with nits, changeset dbc99e82183a.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0a3
You need to log in before you can comment on or make changes to this bug.