Closed Bug 348660 Opened 14 years ago Closed 14 years ago

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

Categories

(Calendar :: Lightning Only, defect, minor)

x86
All
defect
Not set
minor

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ssitter, Assigned: jminta)

Details

Attachments

(1 file)

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]
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.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
Attachment #233664 - Flags: second-review?(dmose)
Attachment #233664 - Flags: first-review?(mattwillis)
Comment on attachment 233664 [details] [diff] [review]
persist view options

> +
> +    switchToView(type);

Remove this extra switchToView() per IRC.

r1=lilmatt
Attachment #233664 - Flags: first-review?(mattwillis) → first-review+
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.
Attachment #233664 - Flags: second-review?(dmose)
(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 on attachment 233664 [details] [diff] [review]
persist view options

Re-requesting review since I believe comment #4 addresses the concerns.
Attachment #233664 - Flags: second-review?(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.
Attachment #233664 - Flags: second-review?(dmose) → second-review+
patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
VERIFIED with Thunderbird/1.5.0.5 (20060719) and Lightning/0.1+ (2006090112).
Status: RESOLVED → VERIFIED
Whiteboard: [litmus testcase wanted]
Litmus testcase 2645 created
Litmus testcase 2646 created
Whiteboard: [litmus testcase wanted]
You need to log in before you can comment on or make changes to this bug.