Closed Bug 390523 Opened 17 years ago Closed 17 years ago

Persist calendar visibility and selection in calendar list

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Currently when loading sb/ltn, all calendars are visibile by default. We should be able to persist the state here.
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
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.
It worked in 0.5 release, so I think it should be fixed for 0.7.
Flags: blocking-calendar0.7?
Properly marking as regression from Bug 388405.
Blocks: 388405
No longer depends on: 388405
Keywords: regression
Summary: Persist calendar visibility → Persist calendar visibility and selection in calendar list
Flags: in-litmus?
Flags: blocking-calendar0.7? → blocking-calendar0.7+
Please don't mark as blocking unless you are a release driver
Flags: blocking-calendar0.7+ → blocking-calendar0.7?
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.
Attached patch Fix persist and selection (obsolete) β€” β€” Splinter Review
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.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #277592 - Flags: review?(michael.buettner)
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
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.
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 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.
Attachment #277592 - Flags: review?(michael.buettner) → review+
Marking blocking, since we're taking this checkin. :-)
Flags: blocking-calendar0.7? → blocking-calendar0.7+
Adressed issue, checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7
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
Status: RESOLVED → VERIFIED
Philipp, can you attach a diff of what you actually checked in? This makes regression tracking easier..
Attached patch Patch as checked in β€” β€” Splinter Review
Sure, sorry about that. I think this should be the correct one.
Attachment #277592 - Attachment is obsolete: true
Attachment #278238 - Flags: review+
Verified on linux with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.7pre) Gecko/20070906 Calendar/0.7pre
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: