321560 - Wrong day selected in new views when using minimonth or datepicker

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Stefan Sitter [:ssitter]]

Wrong day selected in new views when using minimonth or datepicker

If you click a date on the minimonth or select a date using the 'Go Date' command the wrong date is selected in the view. The date selected/highlighted in the view is always one day too little. For example click 28-Dec-2005 in minimonth --> 27-Dec-2005 is selected in view. Happened in all views.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051226 Mozilla Sunbird/0.3a1+ with clean profile.

Comment 1

[Joey Minta]

Most likely a issue in converting the jsDate to a calDateTime.  Eventually, I'd like most of the jsDate's that are being used in reference to the views to disappear.  I had planned to do that when we killed gCalendarWindow, but now it seems that this may need to be done sooner.

Comment 2

[Joey Minta]

Attachment: use getInTimezone, consolidate goToDay calls

The problem here is that since we were just using jsDateTo
DateTime, we were getting a date in UTC, rather than in the default timezone.  This patch uses getInTimezone in the appropriate places, and also converts code that previously used gCalendarWindow.currentView.goToDay to using something a bit more sensible, since the gCalendarWindow.currentView object is actually a bad compatibility hack.

Note that this may help with bug 321562 and bug 321563, since sending a goToDay command with a UTC time would throw them off.

Comment 3

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

Comment on attachment 206874 [details] [diff] [review]
use getInTimezone, consolidate goToDay calls

>Index: mozilla/calendar/resources/content/calendarWindow.js
> CalendarWindow.prototype.pickAndGoToDate = function calWin_pickAndGoToDate( )
>-    currentView.goToDay( jsDateToDateTime(pickedDate) );
>+    currentView.goToDay(jsDateToDateTime(pickedDate).getInTimezone(calendarDefaultTimezone()));

I'm a bit uneasy about having the callers set the right timezone. In the end, the view is what cares about the timezone. Shouldn't that set the timezone?

Comment 4

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

Comment on attachment 206874 [details] [diff] [review]
use getInTimezone, consolidate goToDay calls

>Index: mozilla/calendar/resources/content/calendarWindow.js
>-    currentView.goToDay( jsDateToDateTime(pickedDate) );
>+    currentView.goToDay(jsDateToDateTime(pickedDate).getInTimezone(calendarDefaultTimezone()));

So the caller sets the timezone of the view by setting the timezone of the startdate? I think i can live with that, if it's properly documented.

>Index: mozilla/calendar/resources/content/unifinder.js
>@@ -302,22 +302,21 @@ function unifinderOnSelect( event )

>+      if (gCalendarWindow.currentView.getVisibleEvent(calendarEvent) == false) {
>+         var currentView = document.getElementById("view-deck").selectedPanel;
>+         currentView.goToDay(calendarEvent.startDate);
>+      }

But then won't this set the view's timezone to whatever the timezone of the selected element happens to be? Sounds wrong.
Maybe we shouldn't use the timezone of the starttime as timezone of the view. It it too confusing, in cases like this. Setting the timezone explicitly might be better.

Comment 5

[Joey Minta]

Attachment: set view timezones explicitly

Updated to reflect previous review comments.  Introduces a timezone attribute to the views.  They'll try to put everything in that timezone.  Tested in both GMT+ and GMT- tzs without a problem.

Comment 6

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

Comment on attachment 211516 [details] [diff] [review]
set view timezones explicitly

looks good, thanks!
r=mvl
(we might consider optimizing calDateTime.getInTimezone for cases where the timezone is the same as the current timezone, to speed things up somewhat)

Comment 7

[Joey Minta]

Patch checked in.

Comment 8

[Joey Minta]

Attachment: follow-up patch

(This is the patch from IRC)  Follow-up patch based on some feedback by ssitter.  Basically we're just changing the order of where getInTimezone is called.

Comment 9

[Stefan Sitter [:ssitter]]

Reopen this bug as the problem is only partly fixed until now.

Comment 10

[Joey Minta]

Comment on attachment 211677 [details] [diff] [review]
follow-up patch

This is sunbird only, so I suppose it probably should have been mvl from the beginning.

Comment 11

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

Comment on attachment 211677 [details] [diff] [review]
follow-up patch

What does this patch try to fix?

Comment 12

[Stefan Sitter [:ssitter]]

(In reply to comment #11)
> (From update of attachment 211677 [details] [diff] [review] [edit])
> What does this patch try to fix?

Happens because of my UTC+1 timezone offset:

Week view: If you click first day of week in minimonth the previous week is displayed.

Multiweek view: If you click first day of week in minimonth there is one more previous week displayed as configured.

Month view: If you click first day of month in minimonth the previous month is displayed.

Comment 13

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

Comment on attachment 211677 [details] [diff] [review]
follow-up patch

ah, ok. looks good.
r=mvl

Comment 14

[Joey Minta]

patch checked in.