289493 - Lightning improvements, v1

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This patch has a bunch of lightning improvements, including improvements to some
core base bits.  I'm posting it all up in one patch here.  Included:

- calICalendarManagerObserver
- fixes to calCompositeCalendar
- lightning observers for the new composite calendar/calendar manager
- ability to click and add/remove calendards from the currently active composite
calendar

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: lightning-stuff-1.patch

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: lightning-stuff-2.patch

now with |for each|, and without hardcoded /home/vladimir path (cvs up'd to get
shaver's itembase fix)

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 180001 [details] [diff] [review]
lightning-stuff-2.patch

>+  /**
>+   * If a calendar for the given URI exists in the CompositeCalendar,
>+   * return it; otherwise return null.
>+   *
>+   * @param aServer  URI of the server whose calendar to return
>+   * @return calendar for aServer, or null if nonoe
>+   */
>   calICalendar getCalendar( in nsIURI aServer );

I think we decided on IRC that this was better as
  boolean containsCalendar(in nsIURI aCalendar);

But dmose wasn't there, so I ask for his opinion.

Also: "nonoe" wants spelling-love.

>+[scriptable, uuid(383f36f1-e669-4ca4-be7f-06b43910f44a)]
>+interface calICalendarManagerObserver : nsISupports
>+{
>+  void onCalendarRegistered(in calICalendar aCalendar);
>+  void onCalendarUnregistered(in calICalendar aCalendar);
>+  // called before the delete actually takes place
>+  void onCalendarDeleted(in calICalendar aCalendar);
>+
>+  void onCalendarPrefSet(in calICalendar aCalendar, in AUTF8String aName, in AUTF8String aValue);
>+  void onCalendarPrefDeleted(in calICalendar aCalendar, in AUTF8String aName);
> };

IMO, we should call these notifications as follows:
- onCalendarRegistered after we register
- onCalendarUnregistering before we unregister
- onCalendarDeleting before we delete
- onCalendarPrefSet after we set
- onCalendarPrefDeleting before we delete

This allows the callee to usefully inspect state, rather than have to determine
how
things will look after registration has completed, or what the calendar was
like
before we deleted it.

>+        for each (obs in this.mObservers)
>+            obs.onCalendarRegistered(calendar);
>     },

for each (var obs in this.mObservers), please.	(And elsewhere.)

And if I catch you talking smack about |for each|, I'm coming for you.

>+            dump ("*** Trying to load: " + f.path + "\n");
>+

Pls remove!

>+                    dump ("++++++++++++++++ component load: " + componentData[i].onComponentLoad + "\n");

Et aussi.

>+  content/lightning/ltnutils.js			(content/ltnutils.js) 

We decided that we prefer files-named-like.this.  (I would prefer functions and
variables
named that way too, TBH, but such is life in a C-like language world.)

And we don't need to abbreviate lightning in our filenames, as we might in
otherwise
unbearable symbol names.

>+  content/lightning/lightning.css	(content/lightning.css)

skin/ ?

(You get to change chrome.maaaaanifeeeest....)

>+var ltnCompositeCalendarObserver = {
>+    QueryInterface: function(aIID) {
>+        // I almost wish that calICompositeObserver did not inherit from calIObserver,
>+        // and that the composite calendar maintined its own observer list

Can you say more about why?

>+    // calICompositeObserver
>+    onCalendarAdded: function (aCalendar) {
>+        document.getElementById("calendarTree").boxObject.invalidate();

Also, I think we should use hyphen-naming for our elements, and wish I'd done
so
when I was typing.  I was trying to be consistent with thunderbird, before
iRealized
that_it was-a losingbattle.

>+        for (i in this.mCalendars) {
>+            var cal = this.mCalendars[i];
>+            if (aCalendar.uri.equals(cal.uri)) {
>                 // throw exception if calendar already exists?
>                 return;

for each (var cal in this.mCalendars)

(And elsewhere.)

>+function initCalStorageCalendarComponent() {
>+    CalEvent = new Components.Constructor(kCalEventContractID, kCalIEvent);
>+    CalTodo = new Components.Constructor(kCalTodoContractID, kCalITodo);
>+    CalDateTime = new Components.Constructor(kCalDateTimeContractID, kCalIDateTime);
>+    CalAttendee = new Components.Constructor(kCalAttendeeContractID, kCalIAttendee);
>+    CalItemOccurrence = new Components.Constructor(kCalItemOccurrenceContractID, kCalIItemOccurrence);
>+    CalRecurrenceInfo = new Components.Constructor(kCalRecurrenceInfoContractID, kCalIRecurrenceInfo);
>+    CalRecurrenceRule = new Components.Constructor(kCalRecurrenceRuleContractID, kCalIRecurrenceRule);
>+    CalRecurrenceDateSet = new Components.Constructor(kCalRecurrenceDateSetContractID, kCalIRecurrenceDateSet);
>+    CalRecurrenceDate = new Components.Constructor(kCalRecurrenceDateContractID, kCalIRecurrenceDate);
>+    MozStorageStatementWrapper = new Components.Constructor(kMozStorageStatementWrapperContractID, kMozStorageStatementWrapperIID);
>+}

In a non-storage build, that's gonna throw something fierce, but maybe that's
not a problem...

Me, I can't wait for JS2.

r=shaver with appropriate changes, we'll leave the containsCalendar thing for
dmose to
weigh in on and deal with separately.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

> >+[scriptable, uuid(383f36f1-e669-4ca4-be7f-06b43910f44a)]
> >+interface calICalendarManagerObserver : nsISupports
> >+{
> >+  void onCalendarRegistered(in calICalendar aCalendar);
> >+  void onCalendarUnregistered(in calICalendar aCalendar);
> >+  // called before the delete actually takes place
> >+  void onCalendarDeleted(in calICalendar aCalendar);
> >+
> >+  void onCalendarPrefSet(in calICalendar aCalendar, in AUTF8String aName, in
AUTF8String aValue);
> >+  void onCalendarPrefDeleted(in calICalendar aCalendar, in AUTF8String aName);
> > };
> 
> IMO, we should call these notifications as follows:
> - onCalendarRegistered after we register
> - onCalendarUnregistering before we unregister
> - onCalendarDeleting before we delete
> - onCalendarPrefSet after we set
> - onCalendarPrefDeleting before we delete
> 
> This allows the callee to usefully inspect state, rather than have to
determine how things will look after registration has completed, or what the
calendar was like before we deleted it.

Works for me.

> >+var ltnCompositeCalendarObserver = {
> >+    QueryInterface: function(aIID) {
> >+        // I almost wish that calICompositeObserver did not inherit from
calIObserver,
> >+        // and that the composite calendar maintined its own observer list
> 
> Can you say more about why?

Because the things one observes on the composite calendar are things like
"calendar added" and "calendar removed", whereas the things one observes on the
calendars themselves is things like "event added".  As in this case, it seems
that things will often care about one but not the other -- here I just have all
the "event added" calIObserver methods stubbed out.

> blah blah |for each| blah blah

They're all fixed, sheesh!

> >+function initCalStorageCalendarComponent() {
> >+    CalEvent = new Components.Constructor(kCalEventContractID, kCalIEvent);
> >+    CalTodo = new Components.Constructor(kCalTodoContractID, kCalITodo);
> >+    CalDateTime = new Components.Constructor(kCalDateTimeContractID,
kCalIDateTime);
> >+    CalAttendee = new Components.Constructor(kCalAttendeeContractID,
kCalIAttendee);
> >+    CalItemOccurrence = new
Components.Constructor(kCalItemOccurrenceContractID, kCalIItemOccurrence);
> >+    CalRecurrenceInfo = new
Components.Constructor(kCalRecurrenceInfoContractID, kCalIRecurrenceInfo);
> >+    CalRecurrenceRule = new
Components.Constructor(kCalRecurrenceRuleContractID, kCalIRecurrenceRule);
> >+    CalRecurrenceDateSet = new
Components.Constructor(kCalRecurrenceDateSetContractID, kCalIRecurrenceDateSet);
> >+    CalRecurrenceDate = new
Components.Constructor(kCalRecurrenceDateContractID, kCalIRecurrenceDate);
> >+    MozStorageStatementWrapper = new
Components.Constructor(kMozStorageStatementWrapperContractID,
kMozStorageStatementWrapperIID);
> >+}
> 
> In a non-storage build, that's gonna throw something fierce, but maybe that's
> not a problem...

Yeah, I think that I really want to move the storage-checking into the component
init, and just have the component init bail.. but we may run into order issues
if this component gets initialized before storage does.

> r=shaver with appropriate changes, we'll leave the containsCalendar thing for
> dmose to weigh in on and deal with separately.

Ok, i'll get this in shortly.

Comment 5

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

(In reply to comment #0)
> - ability to click and add/remove calendards from the currently active composite
> calendar

Can we share that goodness with calendar/sunbird/whatever?

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

checked in means its fixed!

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Whether or not getCalendar should morph into containsCalendar depends on what
sort of semantics we want to support in the calendarManager and
compositeCalendar for creating brand new calendars (eg CalDAV MKCALENDAR). 
That's perhaps worth a separate bug.