336072 - Make calIItemBase creationDate, lastModifiedTime initializable

Status: NEW

(Calendar :: Internal Components, enhancement)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: trunk

Currently calIItemBase has readonly attributes for creationDate and lastModifiedTime.

This makes it impossible to initialize these attributes.  
Result is bugs like bug 327930.  

Note that lastModifiedTime needs to be initialized after all other properties are initialized, since writing any property changes the lastModifiedTime.

1. One approach is to make createDate and lastModifiedTime attributes writable.

2. Another approach would be to have three states:
  initializable --> mutable --> immutable

creationDate and lastModifiedTime would only be writable while an item is initializable.

(Currently items have two states:  mutable and immutable, and the state can only change mutable --> immutable, not vice versa.)


Reproducible: Always

Comment 1

[Gary van der Merwe]

I think it would be a good idea to have an initializing status for the following  reason:

When setting other properties of a calIItemBase, this.modify(); is called, which causes lastModifiedTime to be set to now. (in ensureNotDirty()).

This is the correct behavior for after the item has been initialized, but not for before. By adding an initializing status, this can be fixed easily.

We should also change the following properties to make it not possible to edit them when not in the initializing status:
* calendar
* id
* icalString

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

How would that work with cloning? For example, you need to change the .calendar property to move or copy an item to a different calendar.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

One possibility re cloning might be to have clone() leave the cloned version in the initializable state.  However, that would essentially force us to add another call after ever call to clone currently in the codebase.

Comment 4

[gekacheka]

CASES

Creation seems to be used in four ways:
1. create a new item (event or todo).
2. create an existing item from a data provider.
3. clone an item and modify mutable properties, for writing into a calendar.
4. clone an item to make a new event or todo (e.g., paste into same calendar).

Case 1 should make new       creationDate, id, lastModifiedTime, etc.
Case 2 should write previous creationDate, id, lastModifiedTime, etc.
Case 3 should clone the      creationDate, id, lastModifiedTime, etc. 
Case 4 should make new       creationDate, id, lastModifiedTime, etc.

Case 1 would be handled by constructor.  New item would be mutable.
Case 2 would be handled by constructor.  New item would be initializable
  only during construction.
Case 3 would be handled by clone.  Cloned item would be mutable.
Case 4 would be handled by cloneNew.  Cloned item would be mutable.

Comment 5

[gekacheka]

FIELDS

creationDate would be an initialize-only property.

lastModifiedTime would be an initialize-only property.

stampTime would be an initialize-only property.

icsComponent would be an initilize-only property.

The id/uid could be an initialize-only property, but isn't currently set by the constructor (does it need to be set by the calendar provider?).

The calendar might need to be a mutable property, so that events can be moved without changing the uid.  (The uid is needed for identifying the event/task from iTIP messages about it.)  

Comment 6

[gekacheka]

IMPLEMENTATION ISSUE

Issue: how to guarantee that init-only properties are no longer settable after construction.  The clone and cloneNew methods can initialize state correctly, but createInstance does not pass parameters so it cannot.

Approach: Use a callback during construction.  A callback is a typical way to ensure code is followed by a final operation.

  var newEvent = new CalEvent();

  var providedEvent = new CalEvent(function(eventToInit) {
    eventToInit.id = /* provided id */;
    eventToInit.creationDate = /* provided creationDate */;
    eventToInit.lastModifiedDate = /* provided lastModifiedDate */;
    ...initialize all other properties from provided data...
  });

The init method would run the call back (if one were passed) and initialize state (whether or not a callback was passed).  Then constructing an event or task without a callback would implement case 1, and constructing it with a callback that set all the fields would implement case 2.  The lastModifiedTime would not be updated (dirty not set) for changes made while initializing in the callBack.

In XPCOM, createInstance() does not take user parameters.  There appears to be no way to pass parameters to a javascript constructor.  But a Constructor can pass parameters to an init method defined on the interface.

  const kCalEventContractID = "@mozilla.org/calendar/event;1";
  const kCalIEvent = Components.interfaces.calIEvent;
  const CalEvent = 
    new Components.Constructor(kCalEventContractID, kCalIEvent, "initEvent");

In XPCOM, it is up to the constructor caller to specify the optional initializer ("init") method when specifying the constructor to call.
http://www.mozilla.org/scriptable/components_object.html#_Constructor
It looks like there is no way to avoid the possibility of calling createInstance without calling the init method.  

So to guarantee that the init method gets called requires runtime checks.  The object could start in an 'initializable' state that only a call to the init method could exit, and some operations would throw errors if accessed in the 'initializable' state.  (Reading the creationDate is a candidate operation to catch improperly initialized items before/as they are written to store.)

(Have I overlooked any XPCOM issues?)

Comment 7

[gekacheka]

Attachment: (untested) draft changes to calItemBase, calEvent, calToDo

(patch -l -p 2 -i file.patch)

Draft implementation of CalItemBase with constructor callback for initialization. 

Includes mState to represent state, one of 
  STATE_INITIALIZABLE, STATE_MUTABLE, or STATE_IMMUTABLE
Prototype state is STATE_INITIALIZABLE.
Javascript constructor does not change state.

Constructing:

CalEvent.initEvent and calTodo.initTodo take the callback passed to the Constructor (if any) and pass it to calItemBase.initItemBase, which runs it.  After running the callBack, it sets creationDate, modifiedDate, and stamp if they weren't initialized, and sets the state to STATE_MUTABLE.

While in STATE_INITIALIZABLE, modify() does not mark the item dirty, so modifications do not cause the lastModifiedDate to be updated.  Reads of creationData, lastModifiedTime, or stampTime throw NOT_INITIALIZED (implements run time check to make sure initialized correctly before storing in calendar).

After in STATE_MUTABLE, item is no longer initializable, so calItemBase setters for creationDate, lastModifiedTime, and stampTime throw ALREADY_INITIALIZED.  CalEvent and calToDo setter for icalComponent also does so (this covers their setters for icalString as well).

Cloning:

If cloning into a new item, then calItemBase.cloneItemBaseInto sets creationDate, lastModifiedTime, and stampTime to the current time.

calEvent.clone and calToDo.clone methods are modified to pass an isNew parameter to calItemBase.cloneItemBaseInto.

Interface additions:

Adds cloneNew to calIItemBase.
Adds callBack parameter to calIEvent.initEvent and calIToDo.initTodo.

CalIItemBase creationDate, lastModifiedTime, stampTime are no longer readonly so they can be set by callBack.

Comment 8

[gekacheka]

Attachment: (untested) draft changes to callers: providers, clipboard, calendarUtils

(patch -l -p 2 -i file.patch)

Looks like providers express their event and todo initializations in a local way, so requiring providers to use a callBack to initialize all the properties of an event or todo at once is not a problem.

clipBoard shows example use of cloneNew.

calendarUtils functions call constructor.

----------

Someone else will need to take over from here, either with this approach or another approach.  (My computer is not set up to compile, as needed for the interface changes.)

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #6)
> Approach: Use a callback during construction.  A callback is a typical way to
> ensure code is followed by a final operation.

I really don't like that approach. It's not really the way things should work in xpcom. afaik, you can't do the components.contsructor thing from c++.

Comment 10

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

We can still initialize the properties using setProperty(), right? Can we close this bug, or should we push the above approach in some form?