332492 - Errors in multiweek view if 'First Day Of The Week' is not Sunday

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Stefan Sitter [:ssitter]]

Sunbirds multiweek view has problems if
- 'First Day Of The Week' is not Sunday
- and the current day is the day before 'First Day Of The Week'

This might be a problem with weeknumber calculation where first day of week setting is not taken into account

Tested on 02-Apr-2006 with timezone UTC+2 (Europe/Berlin) and sunbird-0.3a1+.en-US.win32-2006-04-02-01-trunk

Steps to Reproduce:

(0. Do not click into view until you are told to)

1. Start Sunbird with a new profile

2. Switch to multiweek view and notice that today (2006-04-02) is selected,
   weeks 14-18 are displayed (2006-04-02 until 2006-04-29) --> Ok

3. Set 'First Day Of The Week' to Monday in Sunbird options/preferences.

4. Look at multiweek view and notice that no day is selected,
   weeks 14-18 are displayed (2006-04-03 until 2006-04-30)
   
5. Select 'Go to Today' --> Error, Nothing happens, weeks 14-18 are still
   displayed

6. Press 'Weeks 13-17' navigation button --> Ok, weeks 13-17 are displayed,
   2006-04-02 is marked at today (blue color)
   
7. Select 'Go to Today' --> *Error*, weeks 14-18 (2006-04-03 until 2006-04-30)
   are displayed, the current day is not visible
   
8. Quit and Restart Sunbird

9. Look at multiweek view and notice that no day is selected, weeks 14-18 
   are displayed (2006-04-03 until 2006-04-30), the current day is not visible
   
10. Select 'Go to Today' --> *Error*, Nothing happens, weeks 14-18 are still
    displayed
    
11. Press 'Weeks 13-17' navigation button --> *Error*, Nothing happens, 
    weeks 14-18 are still displayed. Error in JavaScript Console:
    
    Error: this.selectedDay has no properties
    Source File: chrome://calendar/content/calendar-decorated-multiweek-view.xml
    Line: 210

Comment 1

[Martin Schröder [:mschroeder]]

Attachment: Patch v1

I "borrowed" the code from calendar-month-view.xml. I'm not sure if it is the correct idea for solving the problem. Maybe more testing than I did is neccessary.

Comment 2

[Joey Minta]

Comment on attachment 216980 [details] [diff] [review]
Patch v1

I tend to think that this logic should go into calendar-month-view's setDateRange, otherwise it will need to be copied everywhere that someone wants to call it.  I'm open to arguments why it belongs here instead, though.

Comment 3

[Martin Schröder [:mschroeder]]

Comment on attachment 216980 [details] [diff] [review]
Patch v1

Looking for a better fix in a better place. New patch coming soon.

Comment 4

[Martin Schröder [:mschroeder]]

After some testing I prefer calendar-decorated-multiweek-view's goToDay for the code.

In multiweek view we have to look at the preference calendar.previousweeks.inview and calculation for displaying the selected day has to be done before taking calendar.previousweeks.inview into account. Only then the day selected before changing the weekStartOffset is displayed after returning from the preference window. This is also true for a correct workingn "Go To Today" command.

For the month view this calculation is done in calendar-month-view's showDate. The real adjustment is done in setDateRange (used by month & multiweek view). So placing the code there would also complicate this method.  For the week view this is done in calendar-decorated-week-view's goToDay, like i would do for multiweek view.

Comment 5

[Joey Minta]

(In reply to comment #4)
> After some testing I prefer calendar-decorated-multiweek-view's goToDay for the
> code.
> 
> In multiweek view we have to look at the preference
> calendar.previousweeks.inview and calculation for displaying the selected day
> has to be done before taking calendar.previousweeks.inview into account. Only
> then the day selected before changing the weekStartOffset is displayed after
> returning from the preference window. This is also true for a correct workingn
> "Go To Today" command.
> 
> For the month view this calculation is done in calendar-month-view's showDate.
> The real adjustment is done in setDateRange (used by month & multiweek view).
> So placing the code there would also complicate this method.  For the week view
> this is done in calendar-decorated-week-view's goToDay, like i would do for
> multiweek view.
> 

OK, I'm sold on the calendar-decorated-week-view's argument.  The current patch is marked obsolete and you said you're providing a new one soon.  Is this still the case?

Comment 6

[Martin Schröder [:mschroeder]]

Attachment: Patch v2

calendar-decorated-multiweek-view.xml:
* goToDay takes calendar.week.start into account (selectedDay remains visible after changing preference)
* added missing "var" in PreferenceObserver

rootCalendarPref.js:
* removed call to refresh() because GoToDay is called to refresh the views

Comment 7

[Martin Schröder [:mschroeder]]

Remaining problem (for a follow-up bug?):
Adjustment needed if the number of previous weeks to show is greater or equal to weeks in multiweek view (3 possible combinations).

I don't know the intended behavior after changing those preferences. But in these cases we have similar problems.

1. What week(s) should be showed in multiweek view?
2. What day should be selected?

I would do a patch if i know the answers.

Comment 8

[Joey Minta]

Comment on attachment 217547 [details] [diff] [review]
Patch v2

Looks quite good.  r=jminta, and we're going to take this for 0.3a2.  ssitter, could you take an extra look at this post landing so we know we're in good shape here?

Comment 9

[Joey Minta]

Patch checked in.  If there are other outstanding edge cases, let's go ahead and file follow-up bugs.  I think the issues related to alternative week-starts are fixed.

Comment 10

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!

Comment 11

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060929 Sunbird/0.3