Wrong day selected in new views when using minimonth or datepicker

RESOLVED FIXED

Status

RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: ssitter, Assigned: jminta)

Tracking

({regression})

Trunk
x86
Windows 2000
regression
Dependency tree / graph

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
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.
(Assignee)

Comment 2

13 years ago
Created attachment 206874 [details] [diff] [review]
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.
Attachment #206874 - Flags: first-review?(mvl)
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 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.
Attachment #206874 - Flags: first-review?(mvl) → first-review-
(Assignee)

Comment 5

13 years ago
Created attachment 211516 [details] [diff] [review]
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.
Assignee: mostafah → jminta
Attachment #206874 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #211516 - Flags: first-review?(mvl)
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)
Attachment #211516 - Flags: first-review?(mvl) → first-review+
(Assignee)

Comment 7

13 years ago
Patch checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

13 years ago
Created attachment 211677 [details] [diff] [review]
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.
Attachment #211677 - Flags: first-review?(dmose)
(Reporter)

Comment 9

13 years ago
Reopen this bug as the problem is only partly fixed until now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

13 years ago
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.
Attachment #211677 - Flags: first-review?(dmose) → first-review?(mvl)
Comment on attachment 211677 [details] [diff] [review]
follow-up patch

What does this patch try to fix?
(Reporter)

Comment 12

13 years ago
(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 on attachment 211677 [details] [diff] [review]
follow-up patch

ah, ok. looks good.
r=mvl
Attachment #211677 - Flags: first-review?(mvl) → first-review+
(Assignee)

Updated

13 years ago
Blocks: 330188
(Assignee)

Comment 14

13 years ago
patch checked in.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.