347216 - Week view: 'Workweek days only' always hides last day of week regardless of setting

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Stefan Sitter [:ssitter]]

Week view: 'Workweek days only' always hides last day of week regardless of setting

Steps to Reproduce:
0. Start Sunbird with clean profile.
1. Go to Sunbird preferences -> Views. 
2. Select Days in workweek: 
   [X] Sun [ ] Mon [ ] Tue [X] Wed  [ ] Thu [ ] Fri  [X] Sat
3. Switch to Week view
4. Activate 'View -> Workweek days only'

Actual Results:
Only Sunday + Wednesday are shown. Saturday is not show as expected.

Additional Information:
If you set 'First Day of Week' to Monday only Wednesday + Saturday are shown. Sunday is not shown.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060803 Calendar/0.3a2+

Comment 1

[Matthew (lilmatt) Willis]

The main problem seems to be that the dateList returned by 
    var dateList = viewElement.getDateList({});
at
    http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-decorated-week-view.xml#251
only contains the first 6 days of the week!

This may be because d2 at 
    http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-decorated-week-view.xml#202
is 00:00:00 on day 7 (or 24:00:00 on day 6). Perhaps the endWeek needs to be the end of the last day (23:59:59 or 24:00:00) rather than the beginning?

Before I make a fix and post a patch I want to get some enlightenment from jminta as to what is the Right Way to do it since he wrote the views.

Comment 2

[Matthew (lilmatt) Willis]

Attachment: rev0 - getDateList isn't including mEndDate in the list it returns

(In reply to comment #1)
> Before I make a fix and post a patch I want to get some enlightenment from
> jminta as to what is the Right Way to do it since he wrote the views.

...except that he appears to be out for the evening and I'm being impatient.

So getDateList() returns a list NOT including mEndDate.
removeNonWorkdays() checks the list it gets from getDateList() against the days off preferences to make a list (array) of days to SHOW. Since the last day of the week was not in the list from getDateList(), it never makes it into that "show list".

This patch makes getDateList() INCLUDE mEndDate in the list it returns.

Based on http://lxr.mozilla.org/mozilla/search?string=getDateList I think this patch shouldn't impact other stuff, but jminta's probably the best one to know about such things.

Comment 3

[Joey Minta]

Comment on attachment 232073 [details] [diff] [review]
rev0 - getDateList isn't including mEndDate in the list it returns

+               var end = this.mEndDate.clone();
+               end.day += 1;
+               while (d.compare(end) != 0) {
At the very least, you need to normalize, as I have no idea whether .compare returns sane answers on un-normalized times.  But really, you should make the while loop a < 1 (or a <= 0 like the other places we loop) and just solve it that way.

Comment 4

[Matthew (lilmatt) Willis]

Attachment: rev1 - a better way to do it (thanks jminta)

(In reply to comment #3)
> (From update of attachment 232073 [details] [diff] [review] [edit])
> But really, you should make the while loop a < 1 (or a <= 0 like the other
> places we loop) and just solve it that way.

...which is why I gave the review to you. 'Cause I knew you'd know the tricks.

Fixed it your way.

Comment 5

[Joey Minta]

Comment on attachment 232368 [details] [diff] [review]
rev1 - a better way to do it (thanks jminta)

r=jminta

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

> At the very least, you need to normalize, as I have no idea whether .compare
> returns sane answers on un-normalized times. 

Sounds like it's worth ensuring that the behavior of .compare on non-normalized dates is well-defined.  Without having looked at either the IDL or the code, it seems to me that the behavior probably wants to be Just Works, modulo integer overflow issues.  This probably warrants a separate bug.

Comment 7

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk

-> FIXED

Comment 8

[Damian Szczepanik]

Litmus testcase 2635 created