Closed Bug 332492 Opened 14 years ago Closed 14 years ago

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

Categories

(Calendar :: Internal Components, defect)

x86
Windows 2000
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ssitter, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

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
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #216980 - Flags: first-review?(jminta)
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 on attachment 216980 [details] [diff] [review]
Patch v1

Looking for a better fix in a better place. New patch coming soon.
Attachment #216980 - Attachment is obsolete: true
Attachment #216980 - Flags: first-review?(jminta)
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.
(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?
Attached patch Patch v2Splinter Review
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
Attachment #217547 - Flags: first-review?(jminta)
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 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?
Attachment #217547 - Flags: first-review?(jminta) → first-review+
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.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060929 Sunbird/0.3
Status: RESOLVED → VERIFIED
Whiteboard: [litmus testcase wanted]
Whiteboard: [litmus testcase wanted]
You need to log in before you can comment on or make changes to this bug.