Closed Bug 337941 Opened 18 years ago Closed 18 years ago

Unify view switching/control code

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jminta, Assigned: jminta)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch unify view code v1 (obsolete) — Splinter Review
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. :-)
Assignee: base → jminta
Status: NEW → ASSIGNED
Attachment #221983 - Flags: second-review?(mvl)
Attachment #221983 - Flags: first-review?(dmose)
Blocks: 329775
Blocks: 340195
blocking0.3?

Filter bugspam out using maggieIsMyCat
Flags: blocking0.3?(dmose)
Flags: blocking0.3?(dmose) → blocking0.3+
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 <jminta@gmail.com>
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...
(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?
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.)
Attachment #221983 - Attachment is obsolete: true
Attachment #225336 - Flags: second-review?(mvl)
Attachment #225336 - Flags: first-review?(dmose)
Attachment #221983 - Flags: second-review?(mvl)
Attachment #221983 - Flags: first-review?(dmose)
Comment on attachment 225336 [details] [diff] [review]
unify view code v2

r2=mvl
Attachment #225336 - Flags: second-review?(mvl) → second-review+
Blocks: 324666
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
Attachment #225336 - Flags: first-review?(dmose) → first-review+
patch checked in.  We'll handle additional unification in more bugs.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Depends on: 345606
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: