Bugzilla

Comment 7

•

16 years ago

Attached patch Patch v1 (obsolete) — Details — 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)

Comment 8

•

16 years ago

Attached file ics file with some VJOURNALs — Details

This ics file contains an event, a task and two journals (including the one mentioned in comment #0 with the dates changed to 2009).

Updated

•

16 years ago

No longer depends on: 115226

Updated

•

16 years ago

Assignee: nobody → vcssupport1

Status: NEW → ASSIGNED

Summary: [RFE] Request for iCalendar VJOURNAL Item support in Calendar → iCalendar VJOURNAL support

Updated

•

16 years ago

Attachment #357540 - Attachment mime type: text/calendar → text/plain

Comment 9

•

16 years ago

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!

Comment 10

•

16 years ago

The iterator stuff I was talking about is handled in bug 474219.

Depends on: 474219

Comment 11

•

16 years ago

(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!

Bas van den Bosch

Comment 12

•

16 years ago

Phillip did a "support tasks" thing for different providers before as google doesn't support tasks, you can set the support per provider somehow.

Comment 13

•

16 years ago

(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 14

•

16 years ago

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.

Bas van den Bosch

Comment 15

•

16 years ago

>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?

Comment 16

•

16 years ago

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

Comment 17

•

16 years ago

Attached patch Patch v2 (obsolete) — Details — 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)

Daniel Boelzle [:dbo]

Comment 18

•

16 years ago

WCAP definitely doesn't support journals, so a calendar capability property like "capabilities.journal.supported" seems the right approach to me, too.

Updated

•

16 years ago

Attachment #358715 - Flags: review?(philipp) → review-

Comment 19

•

16 years ago

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

Comment 20

•

15 years ago

Attached patch Patch v3 (obsolete) — Details — 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)

Comment 21

•

15 years ago

Attached patch [after beta] Patch v4 — Details — Splinter Review

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 22

•

15 years ago

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)

Daniel Boelzle [:dbo]

Comment 23

•

15 years ago

Not to excuse myself, but do we really want this massive change for the coming release? Massive changes tend to pose regressions...

Comment 24

•

15 years ago

With next release you mean beta1? No, I'd say this targets beta2 if possible.

Stefan Sitter [:ssitter]

Updated

•

15 years ago

Attachment #367377 - Flags: review?(ssitter)

Updated

•

15 years ago

Attachment #367377 - Attachment description: Patch v4 → [after beta] Patch v4

Daniel Boelzle [:dbo]

Comment 25

•

14 years ago

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 26

•

14 years ago

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)

Comment 27

•

14 years ago

Thanks Martin!

Comment 29

•

13 years ago

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)

Petr Špaček

Comment 31

•

9 years ago

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)

Comment 32

•

9 years ago

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

Updated

•

4 years ago

Assignee: vcssupport1 → nobody

Status: ASSIGNED → NEW

Updated

•

3 years ago

No longer depends on: 474219

BMO Automation

Updated

•

2 years ago

Severity: normal → S3