Comment 16 for bug 174961

Revision history for this message
In , Philipp-bugzilla (philipp-bugzilla) wrote :

Ingo, thanks for giving me credit :)

* General style:
  Your tabs are strange. Consider expanding tabs to 4 spaces. If you like I can
  give you the full calendar style review. See [1] for how we do it. By no means
  required though.
* The first part
  All it does, is create a
  get name() { var calMgr = ...; return calMgr.getCalendarPref(this, "name"); }
  set name(v) { var calMgr = ...; calMgr.setCalendarPref(this, "name", v); return v; }

  for the current object and also the same for suppressAlarms.
* add/delete/modifyItem calls:
  I think its fine to throw CAL_IS_READONLY as you probably already read in
  bug 395956. If you return the original item, then it will be added to the views
  which is not really what you want.
* addItem/adoptItem
  Notifying with onAddItem is correct, there is no onAdoptItem
* getItem:
  Don't worry too much about it, its not used anywhere currently. I don't
  even know if my getItem works correctly. If we do decide to use it, we will
  probably shift to using calIItemBase's hashId, which is globally unique.
* Other code
  convertAbCardToEvent:
    - You are setting recurrenceStartDate, which is by definition readonly. This
      should be automatically calculated using the recurrence rule.
    - You should set lastModifiedTime using setProperty("LAST-MODIFIED"), also
      since it is readonly. Additionally, you must set that date as late as
      possible, since setting anything else (i.e privacy) will modify the date
  Helper functions:
    - You could load calUtils via chrome://calendar/content/calUtils.js
      directly, then you don't have to include all those calendar specific
      helpers. See bug 391506 for how I did it.

Future features:
* calListenerBag (bug 389397) makes it easier to call notifiers, available with
  lightning 0.7
* extension dependencies: You can use <em:requires> to require lightning and
  the other extension. This is also currently possible since you don't support
  sunbird. See [2] and the linked detailed description. This keep people from
  wondering why it doesn't work without ltn installed ;)
* I'd agree that its probably better to replace the event dialog with the
  addressbook properties card. When that card is saved, you can notify with
  the onModifyItem listener. This doesn't have to happen in a modifyItem call.
* For creating new events, you could make the addItem call open the new
  addressbook card and asynchronously call the onOperationComplete listener
  when the user saves the card.

[1] http://wiki.mozilla.org/Calendar:Style_Guide
[2] http://wiki.mozilla.org/Calendar:Creating_an_Extension#Extension_Dependencies