Closed Bug 347113 Opened 14 years ago Closed 14 years ago

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

Categories

(Calendar :: General, defect)

x86
Windows 2000
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ssitter, Assigned: ramach)

Details

(Keywords: regression, Whiteboard: [patch in hand])

Attachments

(3 files, 1 obsolete file)

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).
Flags: blocking0.3+
Keywords: regression
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.
Is there a solid regression range here?
(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.)
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?
My previous Patch does not! handle the selection chages.
(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?
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
Assignee: nobody → ramach
This patch removes the JS-Error if null is passed as argument to set default calendar.
Attachment #231929 - Attachment is obsolete: true
Attachment #232509 - Flags: first-review?
> 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 on attachment 232509 [details] [diff] [review]
patch that removes the JS-Error [checked in]

I'll look at this today or tomorrow.
Attachment #232509 - Flags: first-review? → first-review?(jminta)
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
Attachment #232509 - Flags: first-review?(jminta) → first-review+
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.
(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.
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.
Attachment #232958 - Flags: second-review?(dmose)
Attachment #232958 - Flags: first-review?(mattwillis)
Whiteboard: [patch in hand]
Comment on attachment 232958 [details] [diff] [review]
fix selection [checked in]

r=lilmatt
Attachment #232958 - Flags: first-review?(mattwillis) → first-review+
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?
(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? :-(
(In reply to comment #17)
> Why is it in the Lightning-only component? :-(

I forgot to update when adding Comment #1.
Component: Lightning Only → General
QA Contact: lightning → general
Sunbird version of the previous patch.
Attachment #233254 - Flags: second-review?(mvl)
Attachment #233254 - Flags: first-review?(mattwillis)
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
Attachment #233254 - Flags: first-review?(mattwillis) → first-review+
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.
Attachment #232958 - Flags: second-review?(dmose) → second-review+
Attachment #232509 - Attachment description: patch that removes the JS-Error → patch that removes the JS-Error [checked in]
Comment on attachment 232958 [details] [diff] [review]
fix selection [checked in]

patch checked in.
Attachment #232958 - Attachment description: fix selection → fix selection [checked in]
Comment on attachment 233254 [details] [diff] [review]
sunbird selection

r=mvl
Attachment #233254 - Flags: second-review?(mvl) → second-review+
sunbird selection patch checked in.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.