450917 - Create and populate nsIAbCollection

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Joshua Cranmer [:jcranmer]]

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.

Comment 1

[Joshua Cranmer [:jcranmer]]

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

Comment 2

[Joshua Cranmer [:jcranmer]]

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

Comment 3

[Mark Banner (:standard8)]

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.

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 2

Bitrots are fun, aren't they?

I've also added the tests.

Comment 5

[Mark Banner (:standard8)]

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

Comment 6

[Mark Banner (:standard8)]

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.

Comment 7

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 3

Comment 8

[Joshua Cranmer [:jcranmer]]

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

Accidentally hit enter too early...

Comment 9

[David :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

Comment 10

[Joshua Cranmer [:jcranmer]]

Checked in with nits, changeset dbc99e82183a.