292350 - nsIAbListener.idl should have documentation

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Mark Banner (:standard8)]

This is being raised as a result of bug 292313. We should ensure that the
interface functions in nsIAbListener.idl (and possibly others) are documented to
prevent confusion.

Comment 1

[Mark Banner (:standard8)]

Attachment: Documentation for onItemRemoved

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 187536 [details] [diff] [review]
Documentation for onItemRemoved

Opps, clicked the submit button too early :-(

This patch is a WIP and provides initial documentation for onItemRemoved. I
should be working near the other two functions soon, so will document them as I
go.

Comment 3

[Mark Banner (:standard8)]

Attachment: Documentation for all nsIAbListener functions

I think this is the complete documentation for this interface. I'll get a few
comments before reviews though.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 188132 [details] [diff] [review]
Documentation for all nsIAbListener functions

I think you don't strictly need reviews on documentation only patches, but I'd
like to check that you both agree with me that this documentation looks correct
before we put it in.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 188132 [details] [diff] [review]
Documentation for all nsIAbListener functions


>+/**
>+  * nsIAbListener
>+  *
>+  * Derive from this class to receive notifications of address book
>+  * items being added, removed or changed with loaded address books.

As written, this comment only applies to C++.  How about replacing "Derive from
this class" with "Implement this interface".

>+  * Subscribe to events by using nsIAddrBookSession.
>+  *
>+  */
> [scriptable, uuid(1920E484-0709-11d3-A2EC-001083003D0C)]    
> interface nsIAbListener : nsISupports {
>+  /**
>+   * Called when an address book item (book, card or list) is added
>+   *
>+   * @param       parentDir       The parent of the item being added.
>+   *
>+   * @param       item            The item being added to the database (a
>+   *                              directory or card).
>+   *
>+   */
>   const abListenerNotifyFlagValue added = 0x1;
>   void onItemAdded(in nsISupports parentDir, in nsISupports item);

Doxygen is going to think that comment applies to the const rather than the
function.  Can you move the const below the function?  Or, even better, to
nsIAddrBookSession where it really belongs.

>+  /**
>+   * Called when an address book item is removed.
>+   *
>+   * Subscribe to directoryRemoved to receive notification of an address
>+   * book being removed.
>+   *
>+   * Subscribe to directoryItemRemove to receive notification of mailing list
>+   * or address book cards being removed.
>+   *
>+   * @param       parentDir       The parent of the item being removed, this
>+   *                              may be an empty directory in the case of a
>+   *                              top level address book.
>+   *
>+   * @param       item            The item being removed from the database.
>+   *
>+   */
>   const abListenerNotifyFlagValue directoryItemRemoved = 0x2;
>   const abListenerNotifyFlagValue directoryRemoved = 0x4;
>   void onItemRemoved(in nsISupports parentDir, in nsISupports item);

Same ordering issues as above.

>+  /**
>+   * Called when an address book item is changed. Note the current
>+   * implementation means that property is either DirName or null, with
>+   * oldValue and newValue being specified if the property is DirName otherwise
>+   * they are null.
>+   *
>+   * @param       item            The item being updated (a directory or a
>+   *                              card).
>+   *
>+   * @param       property        The property of the item being changed.
>+   *                              
>+   * @param       oldValue        The old value of the item property being
>+   *                              changed if it is known, null otherwise.
>+   *                              
>+   * @param       newValue        The new value of the item property being
>+   *                              changed.
>+   *                              
>+   */
>   const abListenerNotifyFlagValue changed = 0x8;
>   void onItemPropertyChanged(in nsISupports item, in string property, in wstring oldValue, in wstring newValue);

It's not clear from reading the above comments whether the literal string
"DirName" is meant, or whether that is intended as a placeholder for the
(display?) name of the nsIAbDirectory.	Also, same ordering problem as before.

Comment 6

[Mark Banner (:standard8)]

Attachment: Document and tidy nsIAbListener and part of nsIAddrBookSession

This patch addresses dmose's review comments - tidying up the documentation and
moving the constants from nsIAbListener to nsIAddrBookSession.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 189825 [details] [diff] [review]
Document and tidy nsIAbListener and part of nsIAddrBookSession

r=dmose; nice work!

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 189825 [details] [diff] [review]
Document and tidy nsIAbListener and part of nsIAddrBookSession

>Index: mailnews/addrbook/resources/content/addressbook-panel.js
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook-panel.js,v
>retrieving revision 1.12
>diff -u -r1.12 addressbook-panel.js
>--- mailnews/addrbook/resources/content/addressbook-panel.js	1 May 2005 09:48:33 -0000	1.12
>+++ mailnews/addrbook/resources/content/addressbook-panel.js	19 Jul 2005 20:22:46 -0000
>@@ -102,8 +102,10 @@
>   // selected directory's name is modified
>   var addrbookSession = Components.classes["@mozilla.org/addressbook/services/session;1"].getService().QueryInterface(Components.interfaces.nsIAddrBookSession);
>   // this listener only cares when a directory is removed or modified
>-  addrbookSession.addAddressBookListener(gAddressBookPanelAbListener,
>-                  Components.interfaces.nsIAbListener.directoryRemoved | Components.interfaces.nsIAbListener.changed);
>+  addrbookSession.addAddressBookListener(
>+    gAddressBookPanelAbListener,
>+    Components.interfaces.nsIAddrBookSession.directoryRemoved |

Note that you may (but don't have to) refer to these as
addrbookSession.directoryRemoved, etc.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 189825 [details] [diff] [review]
Document and tidy nsIAbListener and part of nsIAddrBookSession

Requesting approval, mainly documentation, though also moved some constants to
a different interface as they fit better there, should only be a minor risk.

Comment 10

[Mark Banner (:standard8)]

Patch checked in:
/cvsroot/mozilla/mailnews/addrbook/public/nsIAbListener.idl,v  <-- 
nsIAbListener.idl
new revision: 1.10; previous revision: 1.9
/cvsroot/mozilla/mailnews/addrbook/public/nsIAddrBookSession.idl,v  <-- 
nsIAddrBookSession.idl
new revision: 1.14; previous revision: 1.13
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook-panel.js,v  <--
 addressbook-panel.js
new revision: 1.13; previous revision: 1.12
/cvsroot/mozilla/mailnews/addrbook/resources/content/addressbook.js,v  <-- 
addressbook.js
new revision: 1.116; previous revision: 1.115
/cvsroot/mozilla/mailnews/addrbook/src/nsAbView.cpp,v  <--  nsAbView.cpp
new revision: 1.56; previous revision: 1.55
/cvsroot/mozilla/mailnews/addrbook/src/nsAddrBookSession.cpp,v  <-- 
nsAddrBookSession.cpp
new revision: 1.30; previous revision: 1.29
/cvsroot/mozilla/mailnews/addrbook/src/nsDirectoryDataSource.cpp,v  <-- 
nsDirectoryDataSource.cpp
new revision: 1.67; previous revision: 1.66
/cvsroot/mozilla/mail/components/addrbook/content/abContactsPanel.js,v  <-- 
abContactsPanel.js
new revision: 1.11; previous revision: 1.10
/cvsroot/mozilla/mail/components/addrbook/content/addressbook.js,v  <-- 
addressbook.js
new revision: 1.19; previous revision: 1.18