Closed Bug 350965 Opened 14 years ago Closed 13 years ago

iTIP information must be cached in an XPCOM object in order to pass between the display and the Mime Converter

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cmtalbert, Assigned: cmtalbert)

Details

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Lightning BuildID 2006082320 Thunderbird version 3 alpha 1 20060811

We need a method to pass the iTIP information between the mime html converter and the iTIP components that will distill the iMIP message into a set of actions and present those actions to the user. This iTIP Item should encapsulate any calendar item, add the iTIP methods and attributes to it and be able to be used to add to a calendar by the user.

Reproducible: Always

Steps to Reproduce:
1. Attempt to (programmaticly) get the information  from the mime pathway into a form that can be used by the UI layer and other internal layers to implement iTIP

Actual Results:  
You can't without some object to encapsulate that data.

Expected Results:  
This object should exist.

This is more a development bug to serve as a placeholder for a patch.
Assignee: nobody → cmtalbert
This is the iTIPItem that Thomas reviewed. I incorporated his comments into this patch, so it might make sense for him to do the first review. I'm not totally sure about how that should work. It has been moved to live within the src directory so that it is more like its javascript interface cousins of calIEvent and calIRecurrenceInfo etc. That seemed to make more sense to me.
Attachment #236459 - Flags: second-review?(dmose)
Attachment #236459 - Flags: first-review?(mattwillis)
blocking0.3+ per dmose

We want to improve the iMIP/iTIP story in 0.3
Flags: blocking0.3+
Whiteboard: [patch in hand][no l10n impact][needs review lilmatt dmose]
Comment on attachment 236459 [details] [diff] [review]
iTIP Item interface pulled away from the C++ interfaces

First off, we have two schools of CamelHumps going on in filenames.
There's "capitalize all letters in acronyms" as found in calIICSService.idl and "capitalize only the first letter of acronyms" as found in calIcsImportExportModule.js.
Please choose one of those styles rather than creating a third style with "calIITipItem.idl". I personally prefer the second style, but it's up to you.


>Index: base/public/Makefile.in
>===================================================================
>+ifdef MOZ_THUNDERBIRD
>+ITIP_EXTRA_IDL=calIITipItem.idl
>+endif
>+
>+
> XPIDLSRCS	= calIAlarmService.idl \
> 		  calIAttachment.idl \
> 		  calIAttendee.idl   \
> 		  calICalendar.idl   \
> 		  calICalendarManager.idl \
> 		  calICalendarProvider.idl \
> 		  calICalendarView.idl \
> 		  calICalendarViewController.idl \
>@@ -66,13 +71,14 @@ XPIDLSRCS	= calIAlarmService.idl \
> 		  calIRecurrenceDateSet.idl \
> 		  calIRecurrenceItem.idl \
> 		  calIRecurrenceRule.idl \
> 		  calIImportExport.idl \
> 		  calITodo.idl       \
> 		  calIDateTimeFormatter.idl \
> 		  calIWeekTitleService.idl \
> 		  calIPrintFormatter.idl \
>+		  $(ITIP_EXTRA_IDL) \
> 		  $(NULL)
After chatting with dmose on IRC about this, we decided that since when we enable Sunbird to interact with a non-Thunderbird mail client we'll need this stuff, we shouldn't ifdef this.
Just insert it in XPIDLSRCS in alpha order.


>Index: base/public/calBaseCID.h
>===================================================================
>+#ifdef MOZ_THUNDERBIRD
>+#define CAL_ITIPITEM_CID \
>+    { 0xb84de879, 0x4b85, 0x4d68, { 0x85, 0x50, 0xe0, 0xc5, 0x27, 0xe4, 0x6f, 0x98 } }
>+#define CAL_ITIPITEM_CONTRACTID \
>+    "@mozilla.org/calendar/itip-item;1"
>+#endif
>+
Same thing here. No ifdef.


>-#define NS_ERROR_CALENDAR_WRONG_COMPONENT_TYPE		NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_CALENDAR, 1)
>+#define NS_ERROR_CALENDAR_WRONG_COMPONENT_TYPE      NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_CALENDAR, 1)
I know errant tabs suck, but leave this alone for now so we don't hose cvs blame.


Choosing how to capitalize calIITipItem becomes important here. VVV  Please use whichever form you picked throughout.
>Index: base/public/calIITipItem.idl
>===================================================================
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * Contributor(s):
>+ *   Clint Talbert <cmtalbert@gmail.com>
You're missing a section in the license header for what the original code is.
Add that.

>+
>+#include "nsISupports.idl"
>+
>+interface calIItemBase;
>+interface calICalendar;
>+
>+/** 
>+ * calIITipItem is an interface item that contains a calIItemBase,
>+ * extending it for use by the graphical display and iTIP processor
>+ * it is used to carry information between the mime, the UI, and the iTIP
>+ * processor
>+ */
Wicked run-on sentence-ish thing.

>+     * Attribute: responseMethod - method that the protocol handler (or the user)
line >= 80 chars ^^^

>+     * Attribute: autoResponse - whether to respond automatically: "AUTO", allow
line >= 80 chars ^^^

>+     * Attribute: targetCalendar - the calendar that this thing should be stored
line >= 80 chars ^^^

>+     * Modifies a calIItemBase that is in the component list. Internally, the 
trailing space ^^^

>+     * interface will update the proper component. It does this via the UID of
line >= 80 chars ^^^

>+    /**
>+     * Modifies the state of the given attendee in the item's ics
>+     * @arg in parameter - AUTF8String containing attendee address
>+     * @arg in parameter - AUTF8String contianing the new attendee status
>+     */
>+    void setAttendeeStatus(in AUTF8String attendeeID, in AUTF8String status);

tbe had this comment that doesn't appear to be addressed. If his suggestion isn't appropriate, please explain why.
  "If the calIITipItem contains a list of calIItemBase, does this method set
  the status on all calendar items? If so, wouldn't it be better to include
  a parameter "in calIItemBase item"?


>Index: base/src/Makefile.in
>===================================================================
>+ifdef MOZ_THUNDERBIRD
>+ITIP_EXTRA_SCRIPTS=calITipItem.js
>+endif
>+
> REQUIRES = xpcom \
> 	   js \
> 	   xpconnect \
>            string \
> 	   ical \
> 	   $(NULL)
> 
> XPIDL_MODULE	= calbaseinternal
>@@ -82,16 +86,17 @@ EXTRA_SCRIPTS = \
>     calCalendarManager.js \
>     calDateTimeFormatter.js \
>     calEvent.js \
>     calItemBase.js \
>     calRecurrenceInfo.js \
>     calTodo.js \
>     calUtils.js \
>     calWeekTitleService.js \
>+    $(ITIP_EXTRA_SCRIPTS) \
>     $(NULL)
Same here. No ifdef. Just insert into XPIDL_MODULE in alpha order


Use the same capitalization choice here VVV
>Index: base/src/calITipItem.js
>===================================================================
>+ * The Initial Developer of the Original Code is Mozilla Corporation
>+ * Portions created by the Initial Developer are Copyright (C) 2005
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Clint Talbert <cmtalbert@myfastmail.com>
This is brand new code, right? If so, the initial developer should be you, unless you got hired by MoCo in 2005.

>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+
>+// constructor
>+function calITipItem() {
>+    this.wrappedJSObject = this;
>+    this.mIsInitialized = false;
>+    this.mCurrentItemIndex = 0;
>+}
>+
>+var calITipItemClassInfo = {
>+    getInterfaces: function (count) {
>+        var ifaces = [
>+            Components.interfaces.nsISupports,
>+            Components.interfaces.calIITipItem,
>+            Components.interfaces.nsIClassInfo
>+        ];
>+        count.value = ifaces.length;
>+        return ifaces;
>+    },
>+
>+    getHelperForLanguage: function (language) {
>+        return null;
>+    },
>+
>+    contractID: "@mozilla.org/calendar/itip-item;1",
>+    classDescription: "Calendar ITIP item",
>+    classID: Components.ID("{b84de879-4b85-4d68-8550-e0C527e46f98}"),
>+    implementationLanguage: Components.interfaces.nsIProgrammingLanguage.JAVASCRIPT,
You've declared the Ci constant. Use it here ^^^ to keep the line < 80 chars
>+    flags: 0
>+};
>+
>+calITipItem.prototype = {
>+    QueryInterface: function (aIID) {
>+        if (aIID.equals(Ci.nsIClassInfo))
>+            return calITipItemClassInfo;
Missing braces here

>+
>+        if (aIID.equals(Ci.nsISupports) ||
>+            aIID.equals(Ci.calIITipItem))
>+        {
Make this if statement one line. It'll be under 80 chars.

>+            return this;
>+        }
>+        else {
"} else {" please


>+    modifyItem: function(item) {
>+        // Bug xxxx: This will be used when we support delegation and COUNTER
Is this spinoff bug filed? If not, do so. Either way include the bug # here.

>+    initialize: function(icalData) {
>+        this.mItemList = new Array();
>+        var icsService = Cc["@mozilla.org/calendar/ics-service;1"]
>+                       .getService(Components.interfaces.calIICSService);
Add two spaces to this line for alignment ^^^

>+        // libical returns the vcalendar component if there is just
>+        // one vcalendar. If there are multiple vcalendars, it returns
>+        // an xroot component, with those vcalendar childs. We need to
How about "children" instead?

>+        // Try to get the method property out of the calendar to set our methods
Remove "Try to" so the comment line is < 80 chars.

>+        try {
>+            var method = calComp.method;
>+            // We set both since we do not know exactly what our state will be.
>+            this.mReceiveMethod = method;
>+            this.mResponseMethod = method;
Unaddressed tbe comment:
 "Is this approach right for both cases (one or multiple vcalendars)?"

>+        }
>+        catch (e) {
} catch (e) { please

>+        while (calComp) {
>+            var subComp = calComp.getFirstSubcomponent("ANY");
>+            while (subComp) {
>+                switch (subComp.componentType) {
>+                case "VEVENT":
>+                    var event = Cc["@mozilla.org/calendar/event;1"]
>+                                .createInstance(Ci.calIEvent);
>+                    event.icalComponent = subComp;
>+                    this.mItemList.push(event);
>+                    break;
>+                case "VTODO":
>+                    var todo = Cc["@mozilla.org/calendar/todo;1"]
>+                               .createInstance(Ci.calITodo);
>+                    todo.icalComponent = subComp;
>+                    this.mItemList.push(todo);
>+                    break;
>+                default:
>+                    // Nothing -- Bug xxxx: Implement Freebusy, VJournal etc.
File a bug here and include the number.

>+    getFirstItem: function() {
>+        if (!this.mIsInitialized) {
>+            throw Components.results.NS_ERROR_NOT_INITIALIZED;
>+        }
>+        this.mCurrentItemIndex = 0;
>+        return this.mItemList[0];
>+    },
Unaddressed tbe comment:
 "What about the case, that mItemList is empty?"

>+    getNextItem: function() {
>+        if (!this.mIsInitialized) {
>+            throw Components.results.NS_ERROR_NOT_INITIALIZED;
>+        }
>+        ++this.mCurrentItemIndex;
>+        if (this.mCurrentItemIndex < this.mItemList.length) {
>+            return this.mItemList[this.mCurrentItemIndex];
>+        }
>+        else {
} else { please

>+    setAttendeeStatus: function (attendeeID, status) {        
Lots of trailing spaces on this line ^^^

>+            if (attendee) {
>+                // BUG xxxx: It appears setting status won't work via updating
>+                //           attendee entry, is this by design or is it a bug?
>+                anItem.removeAttendee(attendee);
>+                attendee.participationStatus = status;
>+                anItem.addAttendee(attendee);
File a bug. Use the #.



>Index: base/src/calItemModule.js
>===================================================================
>+var CalITipItem = null;
Use the same capitalization here

>+    CalITipItem = new Components.Constructor("@mozilla.org/calendar/itip-item;1",
>+                                            Components.interfaces.calIITipItem);
Add 1 space to this line ^^^ to fix alignment


r- mostly based on the unaddressed tbe comments
Attachment #236459 - Flags: first-review?(mattwillis) → first-review-
Whiteboard: [patch in hand][no l10n impact][needs review lilmatt dmose] → [patch in hand][no l10n impact][needs review dmose]
I did a quick review of this patch. As lilmatt already pointed out,
there are three unadressed comments from my side on your iTipItem
patch from Bug 334685. Apart from those and lilmatt's comments,
this patch is fine.
Whiteboard: [patch in hand][no l10n impact][needs review dmose] → [no l10n impact][needs new patch rev]
(In reply to comment #4)
> I did a quick review of this patch. As lilmatt already pointed out,
> there are three unadressed comments from my side on your iTipItem
> patch from Bug 334685. Apart from those and lilmatt's comments,
> this patch is fine.
>
Thomas, I'm sorry, I thought I did address your comments. Perhaps that got lost in the shuffle. Here they are:
>+    /**
>+     * Modifies the state of the given attendee in the item's ics
>+     * @arg in parameter - AUTF8String containing attendee address
>+     * @arg in parameter - AUTF8String contianing the new attendee status
>+     */
>+    void setAttendeeStatus(in AUTF8String attendeeID, in AUTF8String status);
>tbe had this comment that doesn't appear to be addressed. If his suggestion
>isn't appropriate, please explain why.
>  "If the calIITipItem contains a list of calIItemBase, does this method set
>  the status on all calendar items? If so, wouldn't it be better to include
>  a parameter "in calIItemBase item"?
Yes, modifying any of the encapsulated items in any way means modifying all the items. Since we always modify all the encapsulated items, I don't see why you'd want this parameter. To me, it seems that you'd want this parameter if the calling function had the ability to specify which calIItemBase in the internal list that you are modifying. The spec states that if multiple events/todos etc are encapsulated in one iMIP message, then the recipient must respond in the same manner to both events/todos/etc. Am I missing something?

Second comment:
 try {
>+            var method = calComp.method;
>+            // We set both since we do not know exactly what our state will be.
>+            this.mReceiveMethod = method;
>+            this.mResponseMethod = method;
Unaddressed tbe comment:
 "Is this approach right for both cases (one or multiple vcalendars)?"
Yes, it is due to the same point above. The spec states that if you have multiple items in the iMIP message, that all those items must have the same method.

Third Comment:
>+    getFirstItem: function() {
>+        if (!this.mIsInitialized) {
>+            throw Components.results.NS_ERROR_NOT_INITIALIZED;
>+        }
>+        this.mCurrentItemIndex = 0;
>+        return this.mItemList[0];
>+    },
Unaddressed tbe comment:
 "What about the case, that mItemList is empty?"
This is addressed by the check to see if the object is initialized. The only way mIsInitialized can be set is via the "intialize" method completing successfully. Since no one can remove anything from the internal item list, once the list is initialized, we know we are not empty. If the list is empty, then we are not initialized, and we will throw NS_ERROR_NOT_INITIALIZED. Would you prefer a different check?

This changes the name to calIItipItem on the interface
From Thomas's comment I think that he and Matt agree about this set of changes. I will ask for a first review from them and second from dmose.
Attachment #236459 - Attachment is obsolete: true
Attachment #237022 - Flags: second-review?(dmose)
Attachment #237022 - Flags: first-review?(mattwillis)
Attachment #236459 - Flags: second-review?(dmose)
Comment on attachment 237022 [details] [diff] [review]
Addressing Thomas's and Matt's comments in the patch

>Index: base/public/calIItipItem.idl
>===================================================================
>+/* -*- Mode: idl; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
tab-width: 20 please

>+ * Contributor(s):
>+ * Clint Talbert <cmtalbert@myfastmail.com>
It's a new file. There's no contributors. :)

>Index: base/src/calItipItem.js
>===================================================================
>+/* -*- Mode: javascript; tab-width: 4; indent-tabs-mode: 20; c-basic-offset: 4 -*- */
Same here. 20.

>+ * Contributor(s):
>+ *   Clint Talbert <cmtalbert@myfastmail.com>
..and here. It's a new file.

>+// constructor
>+function calItipItem() {
Can you make that a javadoc-style comment?

>+    classDescription: "Calendar ITIP item",
Should this be "iTIP"?

>+    classID: Components.ID("{b84de879-4b85-4d68-8550-e0C527e46f98}"),
>+    implementationLanguage: Ci.nsIProgrammingLanguage.JAVASCRIPT,
>+    flags: 0
>+};
>+
>+calItipItem.prototype = {
>+    QueryInterface: function (aIID) {
>+        if (aIID.equals(Ci.nsIClassInfo))
>+        {
Put the { on the same line as the if

>+        if (aIID.equals(Ci.nsISupports) || aIID.equals(Ci.calIItipItem))
>+        {
Same here.


It's all whiny crap.
r1=lilmatt with these fixed
Attachment #237022 - Flags: first-review?(mattwillis) → first-review+
Whiteboard: [no l10n impact][needs new patch rev] → [no l10n impact][needs review dmose]
(In reply to comment #5)
> Yes, modifying any of the encapsulated items in any way means modifying all the
> items. Since we always modify all the encapsulated items, I don't see why you'd
> want this parameter. To me, it seems that you'd want this parameter if the
> calling function had the ability to specify which calIItemBase in the internal
> list that you are modifying. The spec states that if multiple events/todos etc
> are encapsulated in one iMIP message, then the recipient must respond in the
> same manner to both events/todos/etc. Am I missing something?

O.K., if the spec says, that the attendee status must be the same for all items
in the iMIP message, then this method is fine.

> Yes, it is due to the same point above. The spec states that if you have
> multiple items in the iMIP message, that all those items must have the same
> method.

O.K.

> This is addressed by the check to see if the object is initialized. The only
> way mIsInitialized can be set is via the "intialize" method completing
> successfully. Since no one can remove anything from the internal item list,
> once the list is initialized, we know we are not empty. If the list is empty,
> then we are not initialized, and we will throw NS_ERROR_NOT_INITIALIZED. Would
> you prefer a different check?

No, this check is fine.
(In reply to comment #6)
I don't have any additional comments and think this patch is fine.
r1+ tbe
Attachment #237022 - Attachment is obsolete: true
Attachment #237128 - Flags: second-review?(dmose)
Attachment #237128 - Flags: first-review+
Attachment #237022 - Flags: second-review?(dmose)
Comment on attachment 237128 [details] [diff] [review]
Minor changes to address Matt's nits

In generaly, it looks good; most of my comments are minor style stuff.

>+/** 
>+ * calIItipItem is an interface used to carry information between the mime
>+ * parser, the imip-bar UI, and the iTIP processor. It encapsulates a list of
>+ * calIItemBase objects and provides specialized iTIP methods for those items.
>+ */
>+[scriptable, uuid(B84DE879-4B85-4d68-8550-E0C527E46F98)]
>+interface calIItipItem : nsISupports
>+{
>+    /**
>+     * Attribute: isSend - set to TRUE when sending this item to initiate an
>+     * iMIP communication
>+     */
>+    attribute PRBool isSend;

It would be helpful to explain this attribute in more detail, at least talking about who is expected/allowed to set it as well as read it.

>+    /**
>+     * Attribute: receiveMethod - method the iTIP item had upon reciept
>+     */
>+    attribute AUTF8String receiveMethod;

In general, we've decided that if something is going to be used mostly from JS, it's best to use AString for that method, because SpiderMonkey doesn't speak UTF8 internally, which will cost extra conversions.  If you could tweak this and the other strings in this interface, that would be great. Also, maybe naming this "receivedMethod" would be a little clearer.

>+
>+    /**
>+     * Attribute: autoResponse - whether to respond automatically: "AUTO",
>+     * allow user to edit the response "USER" or don't respond at all "NONE"
>+     */
>+    attribute AUTF8String autoResponse;

This looks like it wants to a |long| with three |const| definitions.

>+    /**
>+     * Get the first item from the iTIP message
>+     * @return calIItemBase
>+     */
>+    calIItemBase getFirstItem();
>+
>+    /**
>+     * Get next item from the iTIP message. If there is no next item then it
>+     * returns NULL
>+     * @return calIItemBase
>+     */
>+    calIItemBase getNextItem();

Let's replace these two methods with an nsISimpleEnumerator to be more in line with Mozilla style.

>+    /**
>+     * Modifies a calIItemBase that is in the component list. Internally, the
>+     * interface will update the proper component. It does this via the
>+     * UID of the component.
>+     * @arg in parameter - item to modify
>+     */
>+    void modifyItem(in calIItemBase item);

Presumably, this wants to not just check the UID itself, but call hasSameIds().  I'd also suggest having it return the item it was passed as a convenience to the caller.

Also, Mozilla convention is generally to put initialization methods at the top of the IDL and name them |init| rather than initialize.


>+        // Get the method property out of the calendar to set our methods
>+        try {

Why do a try/catch here?  If something goes wrong, shouldn't we be propagating the exception up the stack, since this item will not be correctly initialized?

>+            var method = calComp.method;
>+            // We set both since we do not know exactly what our state will be.
>+            this.mReceiveMethod = method;
>+            this.mResponseMethod = method;

It's not clear to me what the above comment means.  Can you make it more explicit?

>+    setAttendeeStatus: function (attendeeID, status) {        
>+        // Note that this code forces the user to respond to all items
>+        // in the same way, which is a current limitation of the spec
>+        if ((attendeeID.indexOf("mailto:", 0) != 0) &&
>+            (attendeeID.indexOf("MAILTO:", 0) != 0)) {

Instead of comparing twice, I'd suggest using .match() to match against a regexp, or else use .toLowerCase and .substr and just compare once.

>+            attendeeID = "mailto:" + attendeeID; // prepend mailto
>+        }
>+        for (var i=0; i < this.mItemList.length; ++i) {
>+            var anItem = this.mItemList[i];
>+            var attendee = anItem.getAttendeeById(attendeeID);
>+            if (attendee) {
>+                // BUG 351589: workaround for updating an attendee

If you could add an XXX here, since this would, ideally, change at some point in the future, that would be great.
Attachment #237128 - Flags: second-review?(dmose) → second-review-
Whiteboard: [no l10n impact][needs review dmose] → [no l10n impact][needs new patch]
I've asked for new reviews because the interface changed drastically and I'm really tired. Good chance I may have missed something, but I tried not to.
Attachment #237128 - Attachment is obsolete: true
Attachment #237267 - Flags: second-review?(dmose)
Attachment #237267 - Flags: first-review?(mattwillis)
Comment on attachment 237267 [details] [diff] [review]
Substantial changes to address dmose's comments

Matt already gave his r+, so that can just be carried forward.  r=dmose; nice work!
Attachment #237267 - Flags: second-review?(dmose)
Attachment #237267 - Flags: second-review+
Attachment #237267 - Flags: first-review?(mattwillis)
Attachment #237267 - Flags: first-review+
(In reply to comment #12)
> Created an attachment (id=237267) [edit]
> Substantial changes to address dmose's comments
> 
> I've asked for new reviews because the interface changed drastically and I'm
> really tired. Good chance I may have missed something, but I tried not to.

Ah, I missed that comment.  However, they were all addressing my comments, so I think my re-review should be sufficient.

Changes landed on the trunk and branch.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [no l10n impact][needs new patch]
You need to log in before you can comment on or make changes to this bug.