454195 - calendar overlapp when floating timezone is involved

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Berend Cornelius]

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".

Comment 1

[Berend Cornelius]

Attachment: calendar with overlapping events

This file contains events taht overlap themselver in the calendar-view (Application tz == Europe-Paris)

Comment 2

[Berend Cornelius]

Attachment: patch v. #1

This patch takes care that the timezone in which the events are displayed is considered in a array sort.

Comment 3

[Daniel Boelzle [:dbo]]

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

Comment 4

[Berend Cornelius]

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

Comment 5

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

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?

Comment 6

[Daniel Boelzle [:dbo]]

(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.

Comment 7

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

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 :) )

Comment 8

[Daniel Boelzle [:dbo]]

(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.

Comment 9

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

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?

Comment 10

[Daniel Boelzle [:dbo]]

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.

Comment 11

[Berend Cornelius]

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.