365639 - Navigation in Multiweek view is faulty if 'Previous weeks to show' is not None

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Stefan Sitter [:ssitter]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a2pre) Gecko/20070102 Calendar/0.4a1

If preference 'Previous weeks to show' is set to one week the command 'Next week' does nothing and the command 'Previous week' navigates the view two weeks back.

If preference 'Previous weeks to show' is set to two weeks the command 'Next week' navigates the view one week back and the command 'Previous week' navigates the view three weeks back.

If preference 'Previous weeks to show' is set to None the navigation works as expected.

This is regressed by the checkin for Bug 336287.

Comment 1

[Joey Minta]

Attachment: compensate for previous weeks

Since goToDay is going to subtract days in order to display the previous weeks, we need to make sure to add enough to cover that compensation.

Comment 2

[Matthew (lilmatt) Willis]

Comment on attachment 250781 [details] [diff] [review]
compensate for previous weeks

>Index: calendar/base/content/calendar-decorated-multiweek-view.xml
>===================================================================

>                     // aNumber only corresponds to the number of weeks to move
>-                    d1.day += 7*aNumber;
>+                    // make sure to compensate for previous weeks in view too
>+                    d1.day += 7 * (aNumber + getPrefSafe("calendar.previousweeks.inview", 4));

If we can't find the preference, is 4 weeks really the correct value?  I'm not sure where 4 came from, and I'd like to know before approving this.

Comment 3

[Stefan Sitter [:ssitter]]

(In reply to comment #2)

The default value for 'calendar.previousweeks.inview' is 0 (equals to none in the preference dialog).

Comment 4

[Joey Minta]

(In reply to comment #2)
> If we can't find the preference, is 4 weeks really the correct value?  I'm not
> sure where 4 came from, and I'd like to know before approving this.
> 
Stolen from http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-decorated-multiweek-view.xml#185
but you're right that that's not the correct default.

Comment 5

[Matthew (lilmatt) Willis]

Comment on attachment 250781 [details] [diff] [review]
compensate for previous weeks

>+                    // make sure to compensate for previous weeks in view too
>+                    d1.day += 7 * (aNumber + getPrefSafe("calendar.previousweeks.inview", 4));

r=lilmatt with this changed from 4 to 0 as per the discussion.


What strikes me as The Right Fix is to use a constant of some sort, be it a build-type substitution, or something in one of the *Utils.js files so that we define the actual number in one place, and it gets updated everywhere else (including in lightning|sunbird.js).

Comment 6

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

The Right place fto define this constant only once would be our own prefs file. Then you don't need the fallback anymore, because you know the pref will always be set to something.

Comment 7

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

Comment on attachment 250781 [details] [diff] [review]
compensate for previous weeks

r2=mvl

Comment 9

[Joey Minta]

patch checked in.

Comment 10

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070214 Calendar/0.4a1

Comment 11

[Damian Szczepanik]

Litmus testcase 3021 created