390523 - Persist calendar visibility and selection in calendar list

Status: VERIFIED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Philipp Kewisch [:Fallen][☁️📆]]

Currently when loading sb/ltn, all calendars are visibile by default. We should be able to persist the state here.

Comment 1

[Stefan Sitter [:ssitter]]

Works for me using Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.6pre) Gecko/20070731 Calendar/0.7pre

Comment 2

[Philipp Kewisch [:Fallen][☁️📆]]

Ah yes, it does work on a clean tree. I am interested what piece of code makes this persist? If bug 388405 lands the way it is now (v5), then this bug will actually exist.

Comment 3

[Omar Bajraszewski]

It worked in 0.5 release, so I think it should be fixed for 0.7.

Comment 4

[Stefan Sitter [:ssitter]]

Properly marking as regression from Bug 388405.

Comment 6

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

Please don't mark as blocking unless you are a release driver

Comment 7

[Stefan Sitter [:ssitter]]

Just for information: Bug 362930 added selection persistent to the previous calendar list. Maybe this helps to find a solution for the new calendar list added with Bug 388405.

Comment 9

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: Fix persist and selection

This patch takes care of it. The problem was setting the selection in the addCalendar function for remembering the selection, and the init function for the persist. More specifically, I was manually firing the onCalendarRegistered function in the initialization function, which automatically added the calendars to the composite calendars. To fix this I added an argument to onCalendarRegistered, which is undefined if not passed (the way it should normally be) and true when initializing. Only when it is true, the calendar is auto-added to the default calendar.

Comment 10

[Stefan Sitter [:ssitter]]

Comment on attachment 277592 [details] [diff] [review]
Fix persist and selection

>  // calICalendarManagerObserver
>- onCalendarRegistered: function cMO_onCalendarRegistered(aCalendar) {
>+ onCalendarRegistered: function cMO_onCalendarRegistered(aCalendar, aInit) {

I wonder if this works because the calICalendarManagerObserver interface defines just one argument for onCalendarRegistered http://lxr.mozilla.org/mozilla/source/calendar/base/public/calICalendarManager.idl#81

Comment 11

[Philipp Kewisch [:Fallen][☁️📆]]

I assume it works, but if it doesn't I'll try to find an alternative. I could find no errors during initial testing. I believe xpconnect doesn't need a function signature exactly like it is in the interface.

Comment 12

[Daniel Boelzle [:dbo]]

Yes, there's no problem with xpconnect, but it obviously confuses people. I'd rather use two functions, e.g

// private, called from load:
onCalendarRegistered_(aCalendar, aInit)

// interface, called from outside:
onCalendarRegistered(aCalendar) { this.onCalendarRegistered_(aCalendar, false); }

just my 2 cents.

Comment 13

[Michael Büttner (no reviews TFN)]

Comment on attachment 277592 [details] [diff] [review]
Fix persist and selection

> I believe xpconnect doesn't need a function signature exactly like it is
> in the interface.
Indeed this works since the js-engine allows for optional arguments, as far as I know it doesn't even have something to do with xpconnect. Access to arguments outside of the stackframe are assumed to be undefined, that's why it works. But I second Daniels opinion on that, as I also find his suggestion reasonable.

Apart from that the patch works as advertised and the code looks fine to me. r=mickey with the above mentioned issue being addressed.

Comment 14

[cmtalbert]

Marking blocking, since we're taking this checkin. :-)

Comment 15

[Philipp Kewisch [:Fallen][☁️📆]]

Adressed issue, checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 16

[Omar Bajraszewski]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070824 Calendar/0.7pre and lightning 2007082405

Comment 17

[Sebastian Schwieger]

Philipp, can you attach a diff of what you actually checked in? This makes regression tracking easier..

Comment 18

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: Patch as checked in

Sure, sorry about that. I think this should be the correct one.

Comment 19

[Omar Bajraszewski]

Verified on linux with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.7pre) Gecko/20070906 Calendar/0.7pre