347113 - Unsubscribing from calendar causes js error and no remaining calendar is selected afterwards

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Stefan Sitter [:ssitter]]

Unsubscribing from calendar causes js error and no remaining calendar is selected afterwards

Steps to Reproduce:
1. Start Thunderbird with Lightning installed
2. Switch to Calendars tab and create some calendars
3. Select one of the calendars, press [Delete] button and confirm dialog

Actual Results:
The calendar is removed from list but none of the remaining calendars is selected afterwards. JavaScript error:

  Error: cal has no properties
  Source File: file://<snip>/components/calCompositeCalendar.js Line: 226

This will cause more problems if you want to continue with more calendar related actions, e.g. [Edit] or [Delete].

Expected Results:
If there are remaining calendars in the list one of them should be selected afterwards.

Thunderbird 1.5.0.5 (20060719) + Lightning/0.1+ (2006080207).

Comment 1

[Stefan Sitter [:ssitter]]

Sunbird behaves similarly. After deletion there is no remaining calendar selected but no js error is shown. 
If you now try to create an event with drag'n'drop in week view nothing happens. To make it work you have to select a calendar first.

Comment 2

[Joey Minta]

Is there a solid regression range here?

Comment 3

[Stefan Sitter [:ssitter]]

(In reply to comment #2)
> Is there a solid regression range here?

Works in lightning-2006-03-13-07-mozilla1.8 + Tb 1.5.0.5 (20060802)
Fails in lightning-2006-03-29-16-mozilla1.8 + Tb 1.5.0.5 (20060802)

Works in sunbird-0.3a1+.en-US.win32-2006-03-13-06-trunk
Fails in sunbird-0.3a1+.en-US.win32-2006-03-29-15-trunk

For the time in between there are neither Sunbird or Lightning win32 builds. 
(Maybe I find time to check on linux later that day.)

Comment 4

[Lukas Ramach]

Attachment: Patch to correct handling if null is assigned as defaultcalendar

I changed the handling of the assignment of a default calendar, so that it works without a js error.
I'm unsure howver if a new calendar should be assigned in the prefs, since this is ommited now (in the not patched version this was not excuted and still had as pref the already deleted calendar, which in my opinion is worse.).

Could somebody please review this?

Comment 5

[Lukas Ramach]

My previous Patch does not! handle the selection chages.

Comment 6

[Joey Minta]

(In reply to comment #5)
> My previous Patch does not! handle the selection chages.
> 
Lukas, does that mean you still think this patch is ready for review or not?

Comment 7

[Joey Minta]

Lukas,
    The attachment above can't be read because its in a zip file.  Please attach a plaintext file before clicking the 'Patch' checkbox.  I'll be happy to review it then.  For more info see http://wiki.mozilla.org/Calendar:Hacking#Creating_and_submitting_a_patch

Comment 8

[Lukas Ramach]

Attachment: patch that removes the JS-Error [checked in]

This patch removes the JS-Error if null is passed as argument to set default calendar.

Comment 9

[Lukas Ramach]

> Lukas, does that mean you still think this patch is ready for review or not?

Yes i think my patch is still fit for review. I'm just unsure how the selection issue should be resolved. In my solution its ok if no calendar is selected but in the bug report its stated to select a different calendar as final result. But i wouldn't know which calendar should be selected: the one above the removed, the last created, the first one in the list? A further question for me is if the valid selection should only be maintained after removing a calendar, or for every action?

If somenone would give me a hint of the desired behaviour i'm willing to alter my patch so a valid selection remains after removing a calnedar.

Thank you all for your patience.

Comment 10

[Joey Minta]

Comment on attachment 232509 [details] [diff] [review]
patch that removes the JS-Error [checked in]

I'll look at this today or tomorrow.

Comment 11

[Joey Minta]

Comment on attachment 232509 [details] [diff] [review]
patch that removes the JS-Error [checked in]

This looks greats.
+           // if not null set the new calendar as default in the preferences
+            if (cal)  {
+               getCalendarManager().setCalendarPref(cal, this.mDefaultPref,
                                          "true");
+            }
Looks like there's some weirdness here will indenting.  Did a tab sneak in?  (Calendar code only uses spaces.)  Also,  the getCalendarManager() line should be indented 4 spaces.

I'll fix these few nits prior to checkin, but I'm noting them here for your future reference.  Thanks for the patch! r=jminta

Comment 12

[Joey Minta]

Comment on attachment 232509 [details] [diff] [review]
patch that removes the JS-Error [checked in]

Patch landed on trunk and branch.
Checking in calendar/providers/composite/calCompositeCalendar.js;
/cvsroot/mozilla/calendar/providers/composite/calCompositeCalendar.js,v  <--  calCompositeCalendar.js
new revision: 1.27; previous revision: 1.26

Checking in calendar/providers/composite/calCompositeCalendar.js;
/cvsroot/mozilla/calendar/providers/composite/calCompositeCalendar.js,v  <--  calCompositeCalendar.js
new revision: 1.14.2.9; previous revision: 1.14.2.8

Leaving bug open until we determine the severity of the selection issue.  ssitter, what are the failure paths/modes now?  If need be, we should just select the calendar that's now at the index of the old calendar.  And, if the old calendar was the last one, just select the new last calendar.

Comment 13

[Stefan Sitter [:ssitter]]

(In reply to comment #12)
> Leaving bug open until we determine the severity of the selection
> issue. ssitter, what are the failure paths/modes now?

Further calendar related actions like [Edit...] or [Delete] will fail.
Event creation via drag and drop will fail.

> If need be, we should just select the calendar that's now at the 
> index of the old calendar.  And, if the old calendar was the last
> one, just select the new last calendar.

If I remember correctly that is what was done before.

Comment 14

[Joey Minta]

Attachment: fix selection [checked in]

Patch explicitly sets the selection once we delete a row.  This will reset the default calendar which will allow for event creation, etc. to work.

Comment 15

[Matthew (lilmatt) Willis]

Comment on attachment 232958 [details] [diff] [review]
fix selection [checked in]

r=lilmatt

Comment 16

[Stefan Sitter [:ssitter]]

Comment on attachment 232958 [details] [diff] [review]
fix selection [checked in]

This patch seems to fix Lightning only. Can I convince you to fix Sunbird too?

Comment 17

[Joey Minta]

(In reply to comment #16)
> (From update of attachment 232958 [details] [diff] [review] [edit])
> This patch seems to fix Lightning only. Can I convince you to fix Sunbird too?
> 
This is a Sunbird bug too?  Why is it in the Lightning-only component? :-(

Comment 18

[Stefan Sitter [:ssitter]]

(In reply to comment #17)
> Why is it in the Lightning-only component? :-(

I forgot to update when adding Comment #1.

Comment 19

[Joey Minta]

Attachment: sunbird selection

Sunbird version of the previous patch.

Comment 20

[Matthew (lilmatt) Willis]

Comment on attachment 233254 [details] [diff] [review]
sunbird selection

>Index: calendar/resources/content/calendarManagement.js
>===================================================================
>@@ -233,21 +246,19 @@ function setCalendarManagerUI()
>         } catch(e) {}
>         listItem.appendChild(colorCell);
> 
>         var nameCell = document.createElement("listcell");
>         nameCell.setAttribute("label", calendar.name);
>         listItem.appendChild(nameCell);
> 
>         listItem.calendar = calendar;
>-        calendarList.appendChild(listItem);
>-
>-        if (calendarList.selectedIndex == -1)
>-            calendarList.selectedIndex = 0;    
>+        calendarList.appendChild(listItem);    
>     }

Looks like the calendarList.appendChild line picked up some trailing spaces or something.

r=lilmatt with that fixed

Comment 21

[Joey Minta]

Comment on attachment 232958 [details] [diff] [review]
fix selection [checked in]

+    if (index == calTree.view.rowCount-1) {
+        index--;
+    }
r=dmose with a comment making that more clear.

Comment 22

[Joey Minta]

Comment on attachment 232958 [details] [diff] [review]
fix selection [checked in]

patch checked in.

Comment 23

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

Comment on attachment 233254 [details] [diff] [review]
sunbird selection

r=mvl

Comment 24

[Joey Minta]

sunbird selection patch checked in.