348660 - User selection for 'Tasks in View'/'Workweek days only' reset on exit

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[Stefan Sitter [:ssitter]]

I saw this when working on Bug 346169: The user selection for 'Tasks in View'/'Workweek days only' is reset on exit.

Steps to Reproduce:
1. Start Thunderbird with Lightning
2. Set 'Calendar -> View -> Workweek days only' to true
3. Set 'Calendar -> View -> Tasks in View' to true
4. Restart Thunderbird

Actual Results:
Both settings are set back to default value.

Expected Results:
Lightning should remember the last user selection.

Sunbird already does this by using a persistent attribute, see [http://lxr.mozilla.org/mozilla/source/calendar/resources/content/calendar.js#78]

Comment 1

[Joey Minta]

Attachment: persist view options

Patch allows lightning to persist its view options.  This is slightly different from Sunbird, in that Lightning shouldn't bother setting these options until it shows the view.

Comment 2

[Matthew (lilmatt) Willis]

Comment on attachment 233664 [details] [diff] [review]
persist view options

> +
> +    switchToView(type);

Remove this extra switchToView() per IRC.

r1=lilmatt

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 233664 [details] [diff] [review]
persist view options

>+    if (view.displayCalendar != getCompositeCalendar()) {

Two questions about the above line:

a) do we need to address the usual JS interface pointer identity testing lossage here?

b) with our current front-end code, don't we _always_ use the composite calendar as our display calendar?  If that's correct, it's not clear to me how this patch could fix this bug as filed.

Comment 4

[Joey Minta]

(In reply to comment #3) 
> a) do we need to address the usual JS interface pointer identity testing
> lossage here?
No.  The views and getCompositeCalendar() all stay entirely in javaScript.  Furthermore, we take care to ensure that we only get the object from XPCOM once.  These have to be exactly the same object.

> 
> b) with our current front-end code, don't we _always_ use the composite
> calendar as our display calendar?  If that's correct, it's not clear to me how
> this patch could fix this bug as filed.
> 
If this is our first time displaying a view, then the displayComposite match will fail (.displayComposite is null) then we check the (persisted) value of the menu-item and assign the tasksInView attribute as appropriate.  I'll note that the displayComposite equality check is the "standard" way we've been checking for first-time-showing-a-view, as in http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-views.js#180 and http://lxr.mozilla.org/mozilla/source/calendar/lightning/content/messenger-overlay-sidebar.js#121

Comment 5

[Joey Minta]

Comment on attachment 233664 [details] [diff] [review]
persist view options

Re-requesting review since I believe comment #4 addresses the concerns.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 233664 [details] [diff] [review]
persist view options

"standard" it may be, but it makes for hard-to-read code.  There wants to be an attribute (readonly, perhaps?) on calICalendarView called "initialized" that one can check for this.  r=dmose with such an attribute added.

Comment 7

[Joey Minta]

patch checked in.

Comment 8

[Stefan Sitter [:ssitter]]

VERIFIED with Thunderbird/1.5.0.5 (20060719) and Lightning/0.1+ (2006090112).

Comment 9

[Damian Szczepanik]

Litmus testcase 2645 created

Comment 10

[Damian Szczepanik]

Litmus testcase 2646 created