calendar overlapp when floating timezone is involved

RESOLVED FIXED in 0.9

Status

Calendar
Calendar Views
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Berend Cornelius, Assigned: Berend Cornelius)

Tracking

unspecified
Bug Flags:
wanted-calendar0.9 +

Details

Attachments

(2 attachments)

(Assignee)

Description

9 years ago
There are special conditions with eastern timezones and floating timezones where the multiday view eventboxes are drawn over one another. Attached there is an ics file with which this scenario can be reproduced when the application timezone is "Europe/Paris".
Flags: wanted-calendar0.9?
(Assignee)

Comment 1

9 years ago
Created attachment 337429 [details]
calendar with overlapping events

This file contains events taht overlap themselver in the calendar-view (Application tz == Europe-Paris)
Assignee: nobody → Berend.Cornelius
(Assignee)

Comment 2

9 years ago
Created attachment 337430 [details] [diff] [review]
patch v. #1

This patch takes care that the timezone in which the events are displayed is considered in a array sort.
Attachment #337430 - Flags: review?(daniel.boelzle)

Updated

9 years ago
Flags: wanted-calendar0.9? → wanted-calendar0.9+
Comment on attachment 337430 [details] [diff] [review]
patch v. #1

Please change the condition to test

if (dt.timezone.floating) then convert to mTimezone,

because floating timezones are presumably more uncommon (saving us some conversion cycles) and it shouldn't make a difference w.r.t this bug.

r=dbo
Attachment #337430 - Flags: review?(daniel.boelzle) → review+
(Assignee)

Comment 4

9 years ago
I worked in the comment of daniel, consolidated a bit more and checked in on trunk, MOZILLA_1_8_BRANCH and SUNBIRD_0_9_BRANCH
-> fixed
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Comment on attachment 337430 [details] [diff] [review]
patch v. #1

>           function sortByStart(a, b) {
>+                  aStart = aStart.getInTimezone(self.mTimezone);

This looks like a very bad idea to me. It converts the timezone in a sort loop. This means that it's very likely to do the conversion for one event quite a few times. That's just a waste of time. I'm pretty sure that this patch will slow the views down quite a bit.
It would be better to convert all events tom mTimezone before this loop. It need to done anyway, and right now, it's done a bit later. Why not do it right at the start?
(In reply to comment #5)
> (From update of attachment 337430 [details] [diff] [review])
> >           function sortByStart(a, b) {
> >+                  aStart = aStart.getInTimezone(self.mTimezone);
> 
> This looks like a very bad idea to me. It converts the timezone in a sort loop.
> This means that it's very likely to do the conversion for one event quite a few
> times. That's just a waste of time. I'm pretty sure that this patch will slow
> the views down quite a bit.
> It would be better to convert all events tom mTimezone before this loop. It
> need to done anyway, and right now, it's done a bit later. Why not do it right
> at the start?
Good idea. But do you really think that floating events are that common and have impact? Only those are converted, and even a conversion from floating to some timezone is really a cheap clone of the datetime.
I don't have real numbers on floating events. But the conversion is in a loop. The function is called a lot. Even the crossing of the xpconnect border that's happening can slow down the sorting.
But anyways, the conversion of every item to the display zone has to happen anyway. I don't see a reason for not doing it before the sort loop.

Actually, I do see a reason: The compare call will call libical, which compares each item to utc. I think we can save a few cycles by doing the conversion to UTC before the loop, so it's only done once per event.
Another approach is to make compare look at .nativeTime instead.
(we are getting pretty close to a new bug, here :) )
(In reply to comment #7)
> I don't have real numbers on floating events. But the conversion is in a loop.
> The function is called a lot. Even the crossing of the xpconnect border that's
> happening can slow down the sorting.
> But anyways, the conversion of every item to the display zone has to happen
> anyway. I don't see a reason for not doing it before the sort loop.
Yes.

> Actually, I do see a reason: The compare call will call libical, which compares
> each item to utc. I think we can save a few cycles by doing the conversion to
> UTC before the loop, so it's only done once per event.
> Another approach is to make compare look at .nativeTime instead.
> (we are getting pretty close to a new bug, here :) )
A conversion into UTC won't work for floating datetimes, because the timeline is based on the view's timezone. Floating events would need to be converted into the view's timezone first.
IMO it's hard to say how many events an average day array of items has, and if this pays off.
Good point on the UTC from floating conversion.

On moving the timezone conversion out of the sort function, should we take care of that here, or in a separate bug?
IMO a matter of taste. Since the discussion has already happened here, this bug could be used. BTW: I think Berend is already working on improving the code.
(Assignee)

Comment 11

9 years ago
I submitted Bug 454543 -  Calendar Multiday-View: Performance improvement is required for this issue. I find the code around the war-on-boxes quite fragile and the patch introduces quite some changes. Therefor I am not sure whether we decide to take this for the 0.9 release.
Target Milestone: --- → 0.9
You need to log in before you can comment on or make changes to this bug.