337941 - Unify view switching/control code

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

Currently both Sunbird and Lightning provide copies of almost identical view controllers.  Their code for switching views contains a lot of common bits as well.  Sunbird has already gotten burned once when Lightning's view controller got additional changes for recurrence editing that Sunbird's didn't.  This bug is to unify that code so that changes only have to be made once, to avoid situations like that again.

Comment 1

[Joey Minta]

Attachment: unify view code v1

This patch does the first part of unifying the view code, bringing together the controllers and the core view switching.  I've ripped these parts out of their respective locations in Sunbird and Lightning and placed them in a common file in base/content/.  I'm asking review from both mvl and dmose since the code touches both programs pretty heavily.

This ought to get both Sunbird and Lightning pretty close to being view-pluggable too. :-)

Comment 2

[Matthew (lilmatt) Willis]

blocking0.3?

Filter bugspam out using maggieIsMyCat

Comment 3

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

Comment on attachment 221983 [details] [diff] [review]
unify view code v1

>Index: base/content/calendar-views.js
>+ * The Initial Developer of the Original Code is
>+ *   Joey Minta <[e-mail]>
You copied most of this code from existing sunbird or lightning code. I don't think it's reasonable to claim that you are the initial developer.
>+ * Contributor(s): 
You should at least list some contributors.

>+    createNewEvent: function (aCalendar, aStartTime, aEndTime) {
>+        } else {
>+            // default pop up the dialog
>+            if ("newEvent" in window) {
>+                // Sunbird specific code
>+                newEvent();
>+                return;
Drop the return, and move the lightning part in an else block. That would be easier to read, by making it more clear that there are really two cases.

>+    modifyOccurrence: function (aOccurrence, aNewStartTime, aNewEndTime) {
>+            if ("editEvent" in window) {
>+                // Sunbird specific code
>+                editEvent();
>+                return;
>+            }
>+            modifyEventWithDialog(itemToEdit);
Same here.

>+function switchToView(aViewType) {
>+    try {
>+        var selectedDay = viewDeck.selectedPanel.selectedDay;
>+    } catch(ex) {} // This dies if no view has even been chosen this session
It would be clearer to move the comment inside the catch block, and explain why you are not doing anything.

>+    var compositeCal;
>+    if ("getCompositeCalendar" in window) {
>+        // Lightning
>+        compositeCal = getCompositeCalendar();
>+    } else {
>+        // Sunbird
>+        compositeCal = getDisplayComposite();
>+    }
Won't it be easier to just sync the name of that function, instead of this check here, and a few other places?

>Index: resources/content/calendar.xul

>+   // Extension authors can tweak this array to make gCalendarWindow.switchToView 
>+   // play nicely with any additional views
>+   this.availableViews = ["day", "week", "multiweek", "month"];
How can they do that? I don't think that they want to replace the entire javascript file...

Comment 4

[Joey Minta]

(In reply to comment #3)
> >+   // Extension authors can tweak this array to make gCalendarWindow.switchToView 
> >+   // play nicely with any additional views
> >+   this.availableViews = ["day", "week", "multiweek", "month"];
> How can they do that? I don't think that they want to replace the entire
> javascript file...
> 
Wouldn't they just need to include in an overlay
gCalendarWindow.availableViews.push("my-cool-new-view");
Or am I missing something?

Comment 5

[Joey Minta]

Attachment: unify view code v2

Patch updated to mvl's comments.  It's bigger now because of the gDisplayComposite->gCompositeCalendar rename for Sunbird.  I added some XXX's for other areas where we have forked function names, since I think it'd be better if this patch doesn't get much bigger.  (getDisplayComposite needed to move to calendarManagement.js since calendarUtil.js is shared.)

Comment 6

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

Comment on attachment 225336 [details] [diff] [review]
unify view code v2

r2=mvl

Comment 7

[Joey Minta]

Comment on attachment 225336 [details] [diff] [review]
unify view code v2

+        // XXX If we're adding an item from the view, let's make sure that
+        // XXX the calendar in question is visible!
+        // XXX unify these
File a bug on that, if it doesn't exist already.

+            // if we're about to modify the parentItem, we need to account
+            // for the possibility that the item passed as argument was
+            // some other ocurrence, but the user said she would like to
+            // modify all ocurrences instead.
+            if (instance.parentItem == instance) {
+
+                var startDiff = instance.startDate.subtractDate(aOccurrence.startDate);
+                aNewStartTime.addDuration(startDiff);
+                var endDiff = instance.endDate.subtractDate(aOccurrence.endDate);
+                aNewEndTime.addDuration(endDiff);
+            }
Fix spelling and add a comment about what sort of corrections we're making in this case.

+        // if we can modify this thing directly (e.g. just the time changed),
+        // then do so; otherwise pop up the dialog
+        var itemToEdit = getOccurrenceOrParent(aOccurrence);
+        if (aNewStartTime && aNewEndTime && !aNewStartTime.isDate && !aNewEndTime.isDate) {
Add a xxx comment and file a bug to make us not modify the time inparams here.

+            if (instance.parentItem == instance) {
+
Use hasSameId here and elsewhere

+    var view = document.getElementById(aViewType+"-view");
+    viewDeck.selectedPanel = view;
+
Comment here that this is related to view-plugging api.

Add comments to the functions in calendar-views describing the expected behavior.

r=dmose

Comment 8

[Joey Minta]

patch checked in.  We'll handle additional unification in more bugs.