Open Bug 185537 Opened 18 years ago Updated 11 months ago

iCalendar VJOURNAL support

Categories

(Calendar :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: jason, Assigned: vcssupport1)

References

(Depends on 1 open bug, )

Details

Attachments

(2 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.2.1) Gecko/20021130
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.2.1) Gecko/20021130

I would like to see support for the VJOURNAL item from the iCalendar spec. I
would like to be able to make/view/edit/delete journal entries. I would like to
be able to toggle journal entries on/off in the current calendar view and I
would like to see a journal only view. I see journal support as a way to merge
weblogs into the calendar.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.




An excerpt of the journal info from the ical spec follows.

4.6.3 Journal Component

   Component Name: VJOURNAL

   Purpose: Provide a grouping of component properties that describe a
   journal entry.

   Formal Definition: A "VJOURNAL" calendar component is defined by the
   following notation:

     journalc   = "BEGIN" ":" "VJOURNAL" CRLF
                  jourprop
                  "END" ":" "VJOURNAL" CRLF

     jourprop   = *(

                ; the following are optional,
                ; but MUST NOT occur more than once

                class / created / description / dtstart / dtstamp /
                last-mod / organizer / recurid / seq / status /
                summary / uid / url /

                ; the following are optional,
                ; and MAY occur more than once

                attach / attendee / categories / comment /
                contact / exdate / exrule / related / rdate /
                rrule / rstatus / x-prop
                )

   Description: A "VJOURNAL" calendar component is a grouping of
   component properties that represent one or more descriptive text
   notes associated with a particular calendar date. The "DTSTART"
   property is used to specify the calendar date that the journal entry
   is associated with. Generally, it will have a DATE value data type,
   but it can also be used to specify a DATE-TIME value data type.
   Examples of a journal entry include a daily record of a legislative
   body or a journal entry of individual telephone contacts for the day
   or an ordered list of accomplishments for the day. The "VJOURNAL"
   calendar component can also be used to associate a document with a
   calendar date.

   The "VJOURNAL" calendar component does not take up time on a
   calendar. Hence, it does not play a role in free or busy time
   searches - - it is as though it has a time transparency value of
   TRANSPARENT. It is transparent to any such searches.

   The "VJOURNAL" calendar component cannot be nested within another
   calendar component. However, "VJOURNAL" calendar components can be
   related to each other or to a "VEVENT" or to a "VTODO" calendar
   component, with the "RELATED-TO" property.

   Example: The following is an example of the "VJOURNAL" calendar
   component:

     BEGIN:VJOURNAL
     UID:19970901T130000Z-123405@host.com
     DTSTAMP:19970901T1300Z
     DTSTART;VALUE=DATE:19970317
     SUMMARY:Staff meeting minutes
     DESCRIPTION:1. Staff meeting: Participants include Joe\, Lisa
       and Bob. Aurora project plans were reviewed. There is currently
       no budget reserves for this project. Lisa will escalate to
       management. Next meeting on Tuesday.\n
       2. Telephone Conference: ABC Corp. sales representative called
       to discuss new printer. Promised to get us a demo by Friday.\n
       3. Henry Miller (Handsoff Insurance): Car was totaled by tree.
       Is looking into a loaner car. 654-2323 (tel).
     END:VJOURNAL
I think its covered in bug 116945

*** This bug has been marked as a duplicate of 116945 ***
Status: UNCONFIRMED → RESOLVED
Closed: 18 years ago
Resolution: --- → DUPLICATE
VJOURNAL support isn't the same as a notepad, and has a very different use. This
is for keeping track of e-mail conversations, logging phone calls, etc. It
should be an extension, but reopened anyway.
Status: RESOLVED → UNCONFIRMED
Depends on: 115226
Resolution: DUPLICATE → ---
New contact from mikep@oeone.com to mostafah@oeone.com
Filter on string OttawaMBA to get rid of these messages. 
Sorry for the spam.
Assignee: mikep → mostafah
Confirming to get it on the radar. 
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: gurganbl → general
*** Bug 327815 has been marked as a duplicate of this bug. ***
Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove
Assignee: mostafah → nobody
Attached patch Patch v1 (obsolete) — Splinter Review
This patch adds backend support for the VJOURNAL item type.  With it, VJOURNALs can be imported and exported to and from a storage calendar but cannot be viewed as there is no UI.  A new cal_journals table is created in storage.sdb.

Most of this patch is just copying what was done for events and tasks, and the rest is changing stuff from working differently for events and tasks to working differently for events, tasks and journals.
Attachment #357539 - Flags: review?(philipp)
This ics file contains an event, a task and two journals (including the one mentioned in comment #0 with the dates changed to 2009).
No longer depends on: 115226
Assignee: nobody → vcssupport1
Status: NEW → ASSIGNED
Summary: [RFE] Request for iCalendar VJOURNAL Item support in Calendar → iCalendar VJOURNAL support
Attachment #357540 - Attachment mime type: text/calendar → text/plain
I'm looking forward to getting this patch in, it looks like a good start. Note however there are probably many more places that need to be changed to support VJOURNALs correctly. We have many places like 

let stringName = (isEvent(item) ? "event" : "task")

that need to be resolved to also support journal entries. I haven't looked into this in detail, but I could imagine some additional methods/properties on calIItemBase like |readonly attribute AUTF8String itemType| which would be something like "event","task","journal". Or maybe (also?) |readonly attribute nsIIDRef itemTypeIID| which returns Components.interfaces.calIEvent or such. This might make things easier to read in code.

The same goes for some UI strings that depend on the item type. I know you haven't added UI yet (which is great, makes reviewing easier), but if you plan to add a journal view as an extension, we should talk about this in detail since it might be eligible for a core feature. Please write me an email or comment on the bug!

Also I'd like to see thunderbird's iteratorUtils hg cp'd to sunbird so we can fix enumerators to be easier to read. This is definitely a different bug, but since you'll probably be touching more of them it would be nice to move them to the new iterators soon.

More comments to come, just wanted to give you some initial feedback!
The iterator stuff I was talking about is handled in bug 474219.
Depends on: 474219
(In reply to comment #9)
> I'm looking forward to getting this patch in, it looks like a good start. Note
> however there are probably many more places that need to be changed to support
> VJOURNALs correctly. We have many places like 
> 
> let stringName = (isEvent(item) ? "event" : "task")
> 
> that need to be resolved to also support journal entries. 

I only really touched on the storage and memory calendars in this patch.  I know that there's stuff to be done with the WCAP provider and probably CALDAV as well, but I've no way to test whether it actually works.  Are there any public WCAP or CALDAV servers that I could use (that support journals)?  I've tried google calendar's CALDAV, and there don't seem to be any problems, but it only supports events. There's also some bits in the UI which will eventually need work, but for now, since there's no journal UI, they don't appear to cause a problem.

> I know you
> haven't added UI yet (which is great, makes reviewing easier), but if you plan
> to add a journal view as an extension, we should talk about this in detail
> since it might be eligible for a core feature. Please write me an email or
> comment on the bug!
> 

I haven't really looked at UI yet, but I'd imagine that something similar to the Tasks UI would work fairly well.

> More comments to come, just wanted to give you some initial feedback!
Phillip did a "support tasks" thing for different providers before as google doesn't support tasks, you can set the support per provider somehow.
(In reply to comment #11)
> I only really touched on the storage and memory calendars in this patch.  I
> know that there's stuff to be done with the WCAP provider and probably CALDAV
> as well, but I've no way to test whether it actually works.  Are there any
> public WCAP or CALDAV servers that I could use (that support journals)?
Maybe cosmo's public server supports VJOURNAL, but I'm less worried about providers. Its ok if some providers don't support VJOURNAL at first. See comment #12, I'd like to see a calendar property "capabilities.journal.supported" at some point.
Comment on attachment 357539 [details] [diff] [review]
Patch v1

Next up, I'll give you a code level review of what you have. Note I haven't tested this or looked for missing pieces. Since its unlikely that the existing pieces will be changed much, I would appreciate a patch with the issues mentioned fixed.



>+          if (isEvent(aItem))
>             return ["TENTATIVE", "CONFIRMED", "CANCELLED"].indexOf(aItem.status);
>+          else if (isToDo(aItem))
>+            return ["NEEDS-ACTION", "IN-PROCESS", "COMPLETED", "CANCELLED" ].indexOf(aItem.status); 
>+          else if (isJournal(aItem))
>+            return ["DRAFT", "FINAL", "CANCELLED"].indexOf(aItem.status);

please add brackets here while you are at it.

>+++ b/calendar/base/public/calIJournal.idl
>@@ -0,0 +1,56 @@
>+/* -*- Mode: idl; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
Please don't add mode headers. Go ahead and add a new boilerplate to this file (i.e 2008, your name as the initial developer, and either your company or something like "The Original Code is Mozilla Calendar Code"


>+//
>+// calIJournal
>+//
>+// An interface for a journal item (analogous to a VJOURNAL)
>+//
Please use
/**
 * calIJournal
 * ...
 */

>         for (let subComp in cal.ical.subcomponentIterator(calComp)) {
>             let item = null;
>+            
>             switch (subComp.componentType) {
>                 case "VEVENT":
I don't think we need an extra line here.

>+            if (parent = isEvent(item)) 
>+                createEvent();
>+            if (parent = isToDo(item))
>+                createTodo();
>+            if (parent = isJournal(item))
>+                createJournal();
Please add brackets here, also while you are here, use cal.create*().


>+
>+//
>+// constructor
>+//
Comment not needed.


>+function calJournal() {
>+    this.initItemBase();
>+
>+    this.journalPromotedProps = {
>+        "DTSTART": true,
>+        __proto__: this.itemBasePromotedProps
>+    }
>+}
>+
>+var calJournalClassInfo = {
>+    getInterfaces: function (count) {
>+        var ifaces = [
Use let instead of var. Also it would be nice to prefix the arguments with "a" here and anywhere else you've added new code. (i.e aCount). Personally I'd prefer to add the classinfo properties/methods to the calJournal prototype itself (i.e |this| as last argument for doQueryInterface.


> function calGetEndDateProp(aItem) {
>     if (isEvent(aItem)) {
>         return "endDate";
>     } else if (isToDo(aItem)) {
>         return "dueDate";
>+    } else if (isJournal(aItem)) {
>+        return null;
Although I agree that a journal won't have an end date, I'm a bit worried that this might cause problems or unclean code when calling calGetEndDateProp: you'd have to special case for journal items. I'd suggest to just return "startDate" and add a comment that a journal entry only has a start date.


>+    } else if(isToDo(item)) {
Space between if and (

>+    } else if (isJournal(item)) {
>+            startDate = item.journalDate;
Indentation.


>         if (isEvent(item)) {
>             iid = Components.interfaces.calIEvent;
>         } else if (isToDo(item)) {
>             iid = Components.interfaces.calITodo;
>+        } else if (isJournal(item)) {
>+            iid = Components.interfaces.calIJournal;
We should think about adding an itemIID attribute to calIItemBase to simplify these cases.

>+            if (wantEvents) {
>+                if (wantTodos) {
>+                    typeIID = Components.interfaces.calIItemBase;
>+                } else {
>+                    typeIID = Components.interfaces.calIEvent;
>+                }
>             } else if (wantTodos) {
>                 typeIID = Components.interfaces.calITodo;
>+            } else if (wantJournals) {
>+                typeIID = Components.interfaces.calIJournal;
>             }
What exactly is this block trying to do? I haven't looked into it closer, but this probably works better:

if (wantEvents + wantTodos + wantJournals > 1) {
  typeIID = Components.interfaces.calIItemBase;
} else if (wantEvents) {
  typeIID = Components.interfaces.calIEvent;
} else if (wantTodos) {
  typeIID = Components.interfaces.calITodo;
} else if (wantJournals) {
  typeIID = Components.interfaces.calIJournal;
}
  


>             if (isEvent_) {
>                 if (!wantEvents) {
>                     continue;
>                 }
>+            } else if (isToDo(item)) {
>+                if (!wantTodos) {
>+                    continue;
>+                }
>+            } else if (isJournal(item)) {
>+               if (!wantJournals) {
>+                    continue;
>+                }
Could be compacted:

if ((isEvent_ && !wantEvents) ||
    (isToDo(item) && !wantTodos) ||
    (isJournal(item) && !wantJournals)) {
    continue;
}

>+                if (isToDo(item)) {
in files other than calUtils.js, please use cal.isToDo and cal.isEvent and import calUtils.jsm if not already done.
>+        sp = this.mSelectJournalsWithRecurrence.params;
>+        try {
>+            while (this.mSelectJournalsWithRecurrence.step()) {
>+                var row = this.mSelectJournalsWithRecurrence.row;
>+                var item = this.getJournalFromRow(row, {});
Please use |let| instead of |var| in new code. I know you've just copied these parts, but it would be better to use the new syntax in new code.

>         if (isEvent(item))
>             this.writeEvent(item, olditem, flags);
>         else if (isToDo(item))
>             this.writeTodo(item, olditem, flags);
>+        else if (isJournal(item))
>+            this.writeJournal(item, olditem, flags);
>         else
>             throw Components.results.NS_ERROR_UNEXPECTED;
Add brackets here.
>Although I agree that a journal won't have an end date, I'm a bit worried that
>this might cause problems or unclean code when calling calGetEndDateProp: you'd
>have to special case for journal items. I'd suggest to just return "startDate"
>and add a comment that a journal entry only has a start date.

Couldn't we use the end-date as a "last edited" property? So we could set the end-date but not represent it in the views. From the specs it's not allowed to set dtend but I think VJOURNAL didn't get much attention in RFC2445 as if yet. http://tools.ietf.org/html/draft-ietf-calsify-rfc2445bis-09#section-3.6.3

Perhaps we can convince Bernard et.al. to allow dtend?
(In reply to comment #15)
> Couldn't we use the end-date as a "last edited" property? So we could set the
> end-date but not represent it in the views. From the specs it's not allowed to
> set dtend but I think VJOURNAL didn't get much attention in RFC2445 as if yet.
> http://tools.ietf.org/html/draft-ietf-calsify-rfc2445bis-09#section-3.6.3
> 
> Perhaps we can convince Bernard et.al. to allow dtend?

I don't really think last-modified fits well as an end date. This would a journal entry from one year ago that I edit today would have a duration of a year. When you think of a journal entry as a blog post, you also see that there is no need for an end date.

from an rfc2445 standpoint, the item can already have a LAST-MODIFIED property, so no need for introducing DTEND. In this case I don't think we should force the specs to match the implementation, rather the other way around.
Attached patch Patch v2 (obsolete) — Splinter Review
(In reply to comment #14)
> (From update of attachment 357539 [details] [diff] [review])
> Next up, I'll give you a code level review of what you have. Note I haven't
> tested this or looked for missing pieces. Since its unlikely that the existing
> pieces will be changed much, I would appreciate a patch with the issues
> mentioned fixed.
> 
> 
> 
> >+          if (isEvent(aItem))
> >             return ["TENTATIVE", "CONFIRMED", "CANCELLED"].indexOf(aItem.status);
> >+          else if (isToDo(aItem))
> >+            return ["NEEDS-ACTION", "IN-PROCESS", "COMPLETED", "CANCELLED" ].indexOf(aItem.status); 
> >+          else if (isJournal(aItem))
> >+            return ["DRAFT", "FINAL", "CANCELLED"].indexOf(aItem.status);
> 
> please add brackets here while you are at it.
> 

Done

> >+++ b/calendar/base/public/calIJournal.idl
> >@@ -0,0 +1,56 @@
> >+/* -*- Mode: idl; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> Please don't add mode headers. Go ahead and add a new boilerplate to this file
> (i.e 2008, your name as the initial developer, and either your company or
> something like "The Original Code is Mozilla Calendar Code"
> 

Done.  Note that mode headers exist in both calIEvent.idl and calITodoi.idl.

> 
> >+//
> >+// calIJournal
> >+//
> >+// An interface for a journal item (analogous to a VJOURNAL)
> >+//
> Please use
> /**
>  * calIJournal
>  * ...
>  */
> 

Done

> >         for (let subComp in cal.ical.subcomponentIterator(calComp)) {
> >             let item = null;
> >+            
> >             switch (subComp.componentType) {
> >                 case "VEVENT":
> I don't think we need an extra line here.
> 

Done

> >+            if (parent = isEvent(item)) 
> >+                createEvent();
> >+            if (parent = isToDo(item))
> >+                createTodo();
> >+            if (parent = isJournal(item))
> >+                createJournal();
> Please add brackets here, also while you are here, use cal.create*().
> 

Done

> 
> >+
> >+//
> >+// constructor
> >+//
> Comment not needed.
> 

Done

> 
> >+function calJournal() {
> >+    this.initItemBase();
> >+
> >+    this.journalPromotedProps = {
> >+        "DTSTART": true,
> >+        __proto__: this.itemBasePromotedProps
> >+    }
> >+}
> >+
> >+var calJournalClassInfo = {
> >+    getInterfaces: function (count) {
> >+        var ifaces = [
> Use let instead of var. Also it would be nice to prefix the arguments with "a"
> here and anywhere else you've added new code. (i.e aCount). Personally I'd
> prefer to add the classinfo properties/methods to the calJournal prototype
> itself (i.e |this| as last argument for doQueryInterface.
> 

Done
Not sure about this.  This is how it is done in calEvent() and calTodo().  For the sake of code consistancy, would it be better to leave it be for now and make changes in all three later on at the same time?

> 
> > function calGetEndDateProp(aItem) {
> >     if (isEvent(aItem)) {
> >         return "endDate";
> >     } else if (isToDo(aItem)) {
> >         return "dueDate";
> >+    } else if (isJournal(aItem)) {
> >+        return null;
> Although I agree that a journal won't have an end date, I'm a bit worried that
> this might cause problems or unclean code when calling calGetEndDateProp: you'd
> have to special case for journal items. I'd suggest to just return "startDate"
> and add a comment that a journal entry only has a start date.
> 

Done
There's no problem using journalDate here and making all journals zero length.  I don't think that there's any problem returning null either, as tasks without due dates also return null.  For safety's sake, we are now returning journalDate.

> 
> >+    } else if(isToDo(item)) {
> Space between if and (
> 
> >+    } else if (isJournal(item)) {
> >+            startDate = item.journalDate;
> Indentation.
> 

Done

> 
> >         if (isEvent(item)) {
> >             iid = Components.interfaces.calIEvent;
> >         } else if (isToDo(item)) {
> >             iid = Components.interfaces.calITodo;
> >+        } else if (isJournal(item)) {
> >+            iid = Components.interfaces.calIJournal;
> We should think about adding an itemIID attribute to calIItemBase to simplify
> these cases.
> 

Is this something that should be added as part of this patch, or something which could be done later?

> >+            if (wantEvents) {
> >+                if (wantTodos) {
> >+                    typeIID = Components.interfaces.calIItemBase;
> >+                } else {
> >+                    typeIID = Components.interfaces.calIEvent;
> >+                }
> >             } else if (wantTodos) {
> >                 typeIID = Components.interfaces.calITodo;
> >+            } else if (wantJournals) {
> >+                typeIID = Components.interfaces.calIJournal;
> >             }
> What exactly is this block trying to do? I haven't looked into it closer, but
> this probably works better:
> 
> if (wantEvents + wantTodos + wantJournals > 1) {
>   typeIID = Components.interfaces.calIItemBase;
> } else if (wantEvents) {
>   typeIID = Components.interfaces.calIEvent;
> } else if (wantTodos) {
>   typeIID = Components.interfaces.calITodo;
> } else if (wantJournals) {
>   typeIID = Components.interfaces.calIJournal;
> }
> 
> 

Done

> 
> >             if (isEvent_) {
> >                 if (!wantEvents) {
> >                     continue;
> >                 }
> >+            } else if (isToDo(item)) {
> >+                if (!wantTodos) {
> >+                    continue;
> >+                }
> >+            } else if (isJournal(item)) {
> >+               if (!wantJournals) {
> >+                    continue;
> >+                }
> Could be compacted:
> 
> if ((isEvent_ && !wantEvents) ||
>     (isToDo(item) && !wantTodos) ||
>     (isJournal(item) && !wantJournals)) {
>     continue;
> }
> 

Done

> >+                if (isToDo(item)) {
> in files other than calUtils.js, please use cal.isToDo and cal.isEvent and
> import calUtils.jsm if not already done.
> >+        sp = this.mSelectJournalsWithRecurrence.params;
> >+        try {
> >+            while (this.mSelectJournalsWithRecurrence.step()) {
> >+                var row = this.mSelectJournalsWithRecurrence.row;
> >+                var item = this.getJournalFromRow(row, {});
> Please use |let| instead of |var| in new code. I know you've just copied these
> parts, but it would be better to use the new syntax in new code.
> 

Done

> >         if (isEvent(item))
> >             this.writeEvent(item, olditem, flags);
> >         else if (isToDo(item))
> >             this.writeTodo(item, olditem, flags);
> >+        else if (isJournal(item))
> >+            this.writeJournal(item, olditem, flags);
> >         else
> >             throw Components.results.NS_ERROR_UNEXPECTED;
> Add brackets here.

Done
Attachment #357539 - Attachment is obsolete: true
Attachment #358715 - Flags: review?(philipp)
Attachment #357539 - Flags: review?(philipp)
WCAP definitely doesn't support journals, so a calendar capability property like "capabilities.journal.supported" seems the right approach to me, too.
Attachment #358715 - Flags: review?(philipp) → review-
Comment on attachment 358715 [details] [diff] [review]
Patch v2

(In reply to comment #17)
> Done.  Note that mode headers exist in both calIEvent.idl and calITodoi.idl.
Yes, we should be getting rid of them, anyone who changes a such file should try to get rid of them :-)

> > Use let instead of var. Also it would be nice to prefix the arguments with "a"
> > here and anywhere else you've added new code. (i.e aCount). Personally I'd
> > prefer to add the classinfo properties/methods to the calJournal prototype
> > itself (i.e |this| as last argument for doQueryInterface.
> > 
> 
> Done
> Not sure about this.  This is how it is done in calEvent() and calTodo().  For
> the sake of code consistancy, would it be better to leave it be for now and
> make changes in all three later on at the same time?

Since we probably won't be creating a bug extra to do this, I think it would be better to do it this way from the start. If you'd like to change this for calEvent and calTodo in this bug, I'm fine with that too.

> I don't think that there's any problem returning null either, as tasks without
> due dates also return null.  For safety's sake, we are now returning
> journalDate.
Since we return the property name instead of the property itself, it could cause problems, since we will be calling something like item[calGetEndDateProp(item)] => item[null]. But since we return journalDate everything is fine.


> > We should think about adding an itemIID attribute to calIItemBase to simplify
> > these cases.
> > 
> 
> Is this something that should be added as part of this patch, or something
> which could be done later?
If you like, go ahead and do it in this patch. We could also instead create a calUtils.jsm function that takes care, but I think its better kept in the item. Doing it in this bug cold simplify some code locations too, i.e:

switch (item.typeIID) {
  case Components.interfaces.calIEvent:
    ...
}

or:

let stringMap = {
   Components.interfaces.calIEvent: "Event",
   Components.interfaces.calITodo: "Task",
   Components.interfaces.calIJournal: "Journal"
};

let theString = calGetString("calendar", "theStringFor" + stringMap[item.typeIID]);

In the IDL file you probably need to use nsIIDRef or nsIIDPtr or nsIID, see http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/base/nsrootidl.idl#79


I have grep'd for (isEvent|isTodo) in the source and have found many locations that just check if (cal.isEvent()) { ... } else { ... } Are you planning to change these in a further patch? I'd prefer it if you could fix these locations in this patch, or at least change the block like:

if (cal.isEvent(item) {
  ...
} else if (cal.isTodo(item)) {
  ...
} else {
  throw "Item type not supported"
}

until we have UI that supports it.

>+          if (isEvent(aItem)) {
>+              return ["TENTATIVE", "CONFIRMED", "CANCELLED"].indexOf(aItem.status);
>+          } else if (isToDo(aItem)) {
>+              return ["NEEDS-ACTION", "IN-PROCESS", "COMPLETED", "CANCELLED" ].indexOf(aItem.status); 
>+          } else if (isJournal(aItem)) {
>+              return ["DRAFT", "FINAL", "CANCELLED"].indexOf(aItem.status);
>+          }
Please prefix all calUtils functions with cal. and make sure the file imports calUtils.jsm. We are transitioning from using calUtils.js to using calUtils.jsm, and the latter loads all functions from calUtils and puts them into the cal namespace.


>+  /**
>+   *  calIJournal
>+   *
>+   *  An interface for a journal item (analogous to a VJOURNAL)
>+   */
remove (2) leading spaces.


>+   */
>+  attribute calIDateTime journalDate;
>+
>+};
>+
Remove trailing blank line.


>-            parent = isEvent(item) ? createEvent() : createTodo();
>+            if (parent = cal.isEvent(item)) {
>+                cal.createEvent();
>+            } else if (parent = cal.isToDo(item)) {
>+                cal.createTodo();
>+            } else if (parent = cal.isJournal(item)) {
>+                cal.createJournal();
>+            }
Not quite right, you mean:

if (cal.isEvent(item)) {
  parent = cal.createEvent();
} else if (cal.isTodo(item)) {
...

With the typeIID thing we can do:

let type = {
  Components.interfaces.calIEvent: "Event",
  Components.interfaces.calITodo: "Todo",
  Components.interfaces.calIJournal: "Journal"
}[item.typeIID];

let parent = cal["create" + type]();


>+    getInterfaces: function (aCount) {
Please unanonymize functions. We have been using the class name, i.e

function cJ_getInterfaces(aCount)


>+    set icalString(value) {
The same goes for [gs]etters, i.e:

set icalString cJ_set_icalString(value)

>+    get icalString() {
>+        var calcomp = getIcsService().createIcalComponent("VCALENDAR");
Use let instead of var here and elsewhere.

> Components.utils.import("resource://calendar/modules/calProviderUtils.jsm");
>+
>+Components.utils.import("resource://calendar/modules/calUtils.jsm");
Remove blank line

>+        var floatingJournalEntry = "journal_entry_tz = 'floating' AND journal_entry"
>+        var nonFloatingJournalEntry = "journal_entry_tz != 'floating' AND journal_entry"
let vs var


r- to get a further iteration of this patch, not to be taken personally :-)
Attached patch Patch v3 (obsolete) — Splinter Review
(In reply to comment #19)

Finally got a chance to get back to this!

I've changed calEvent, calTodo and calJournal to use the same layout and also unanonymised all the functions in all three using cE_, cT_ and cJ_ respectively.

> 
> > > We should think about adding an itemIID attribute to calIItemBase to simplify
> > > these cases.
> > > 
> > 
> > Is this something that should be added as part of this patch, or something
> > which could be done later?
> If you like, go ahead and do it in this patch. We could also instead create a
> calUtils.jsm function that takes care, but I think its better kept in the item.
> Doing it in this bug cold simplify some code locations too, i.e:
> 
> switch (item.typeIID) {
>   case Components.interfaces.calIEvent:
>     ...
> }
> 
> or:
> 
> let stringMap = {
>    Components.interfaces.calIEvent: "Event",
>    Components.interfaces.calITodo: "Task",
>    Components.interfaces.calIJournal: "Journal"
> };
> 
> let theString = calGetString("calendar", "theStringFor" +
> stringMap[item.typeIID]);
> 
> In the IDL file you probably need to use nsIIDRef or nsIIDPtr or nsIID, see
> http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/base/nsrootidl.idl#79
> 

I've ended up using item.itemType which returns "event", "todo" or "journal", and item.itemIID (which I can't seem to get to return anything at all)

I've used item.itemType in calMemoryCalendar.js and calAlarms.js in the patch.

> 
> I have grep'd for (isEvent|isTodo) in the source and have found many locations
> that just check if (cal.isEvent()) { ... } else { ... } Are you planning to
> change these in a further patch? I'd prefer it if you could fix these locations
> in this patch, or at least change the block like:
> 
> if (cal.isEvent(item) {
>   ...
> } else if (cal.isTodo(item)) {
>   ...
> } else {
>   throw "Item type not supported"
> }
> 
> until we have UI that supports it.
> 

I was going to wait until UI was being implemented before messing with anything UI based.  I have gone through the results from:

http://mxr.mozilla.org/comm-central/search?string=isEvent(&find=calendar&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
and
http://mxr.mozilla.org/comm-central/search?string=isTodo(&find=calendar&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

and fixed everything (I hope) that wasn't related to UI, or just relating to a single item type.

> 
> 
> >-            parent = isEvent(item) ? createEvent() : createTodo();
> >+            if (parent = cal.isEvent(item)) {
> >+                cal.createEvent();
> >+            } else if (parent = cal.isToDo(item)) {
> >+                cal.createTodo();
> >+            } else if (parent = cal.isJournal(item)) {
> >+                cal.createJournal();
> >+            }
> Not quite right, you mean:
> 
> if (cal.isEvent(item)) {
>   parent = cal.createEvent();
> } else if (cal.isTodo(item)) {
> ...
> 

Yep :) Fixed.
Other nits also fixed.
Attachment #358715 - Attachment is obsolete: true
Attachment #364814 - Flags: review?(philipp)
Fixed some small nits and added some more fallback in UI code. I'd just like to make the transition easier when we add UI code for places where we assume that everything that is not an event is a task.

I've also made more use of the itemType attribute and fixed itemIID. Feel free to review the interdiff changes between your patch and mine.

I've done some testing but I would appreciate if ssitter could also take a look. Aside from that I'm requesting additional review for the storage calendar changes.
Attachment #364814 - Attachment is obsolete: true
Attachment #367377 - Flags: review?(ssitter)
Attachment #364814 - Flags: review?(philipp)
Comment on attachment 367377 [details] [diff] [review]
[after beta] Patch v4

Daniel, maybe you can also (or instead) take a look at this one?
Attachment #367377 - Flags: review?(dbo.moz)
Not to excuse myself, but do we really want this massive change for the coming release? Massive changes tend to pose regressions...
With next release you mean beta1? No, I'd say this targets beta2 if possible.
Attachment #367377 - Flags: review?(ssitter)
Attachment #367377 - Attachment description: Patch v4 → [after beta] Patch v4
Comment on attachment 367377 [details] [diff] [review]
[after beta] Patch v4

Sorry Fallen, I don't find time for this.
Attachment #367377 - Flags: review?(dbo.moz)
Comment on attachment 367377 [details] [diff] [review]
[after beta] Patch v4

I'll push this patch in my review queue.
Attachment #367377 - Flags: review?(mschroeder)
Duplicate of this bug: 649395
Comment on attachment 367377 [details] [diff] [review]
[after beta] Patch v4

One more patch to give back without a review...
Attachment #367377 - Flags: review?(mschroeder)
Duplicate of this bug: 946128
Philipp, would it make sense for me to start with review/functional testing of this patch before I start looking into bug 415376?
Flags: needinfo?(philipp)
(In reply to bugzilla.mozilla.org from comment #31)
> Philipp, would it make sense for me to start with review/functional testing
> of this patch before I start looking into bug 415376?

Getting this patch into shape will likely be a lot of work, and might be a bit ambitious for a first patch. It would likely make sense to split the work into various parts, e.g. just the code to make sure we don't assume everything that is not an event is a task. Work can be done in separate bugs. If you are seriously interested in fixing this one and bug 415376, we should probably do a quick video conference to discuss it :-)
Flags: needinfo?(philipp)
You need to log in before you can comment on or make changes to this bug.