Closed Bug 454543 Opened 16 years ago Closed 16 years ago

Calendar Multiday-View: Performance improvement is required

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: berend.cornelius09, Assigned: berend.cornelius09)

Details

Attachments

(1 file, 1 obsolete file)

One of the first steps in the process of arranging the eventboxes in the calendar-multiday-view (calendar-day-view or calendar-week-view) is to sort the eventboxes according to the start and end date. One other step is to calculate the  duration of the event considering a certain minimum. Both steps should be consolidated because they contain constant repeating calculations processed in a loop. This has been noticed  already by mvl and me (see Bug 454195 comment#5 and bug437412 comment#9).
Flags: wanted-calendar0.9?
Attached patch patch v. #1 (obsolete) — Splinter Review
In this patch I wrapped an EventInfo object around the event array and added the 'layoutStart' and 'layoutEnd' members according to which the events are now sorted. By doing so I reduce the sorting process and also take care that the 'layoutStart' and 'layoutEnd' values are not calculated again and again. Besides that it was a bug to sort the events according to the 'startDate' and "endDate' and not according to the 'visual' representations "layoutStart' and 'layoutEnd'
Assignee: nobody → Berend.Cornelius
Status: NEW → ASSIGNED
Attachment #337841 - Flags: review?(daniel.boelzle)
I think the changes are too risky for 0.9 this late in the game. Setting qawanted on the patch, since view changes are quite too often good for regressions.
Flags: wanted-calendar0.9? → wanted-calendar0.9-
Whiteboard: qawanted
Flags: wanted-calendar0.9- → wanted-calendar1.0+
Attached patch patch v. #2Splinter Review
I unbitrotted the first patch...
Attachment #337841 - Attachment is obsolete: true
Attachment #343211 - Flags: review?(daniel.boelzle)
Attachment #337841 - Flags: review?(daniel.boelzle)
I checked this patch and it looks okay. One issuee I found was bug 460030, but this bug is not caused by Berends patch.
Comment on attachment 343211 [details] [diff] [review]
patch v. #2

I think you could use |let| instead of |var| throughout the patch.

>+              var item = aEventInfo.event.clone();
>+              var start = item.startDate || item.entryDate;
>+              if (!compareObjects(start.timezone, self.mTimezone)) {
>+                  start = start.getInTimezone(self.mTimezone);
>+              }
>+              aEventInfo.layoutStart = start;
>+              var end = item.endDate || item.dueDate
>+              if (!compareObjects(end.timezone, self.mTimezone)) {
>+                  end = end.getInTimezone(self.mTimezone);
>+              }
I am not sure if the "compare-then-convert" really buys us better performance. IMO it's worth to measure whether simply calling

end = end.getInTimezone(self.mTimezone)

in any case is better.

r=dbo
Attachment #343211 - Flags: review?(daniel.boelzle) → review+
I observed the comments of Daniel. 
I think that the replacement of all "vars" agains "lets" should be done in a separate bug over the whole file.
pushed patch to comm-central
http://hg.mozilla.org/comm-central/rev/588651b7b680
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Whiteboard: qawanted
Checked in lightning build 2008102203557 and sunbird 20081022 -> VERIFIED.
Status: RESOLVED → VERIFIED
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.