323953 - lightning pane doesn't repaint after calendar added or deleted

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

While on the plane this past weekend, I wasn't able to work on 0.1 blockers for various reasons, so I poked at a few other things.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: work in progress patch, v1

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

More complete description of the bug: if you add or delete a calendar, it's still listed in the calendar pane until you manually go to another tab and then come back, forcing a repaint.

Comment 3

[Stefan Sitter [:ssitter]]

Dmose, is this related to Bug 305578? Not the same problem but maybe the same cause?

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Stephan pointed out there there's a really bad failure mode to this bug: since there's not currently any confirmation for deleting a calendar, someone is reasonably likely to hit delete twice since it appears as though nothing happens, thus deleting two calendars.  Adding this back to the blocking list for now.  Alterately, fixing bug 298373 before 0.1 would allow us to drop this bug from the blocker list.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #3)
> Dmose, is this related to Bug 305578? Not the same problem but maybe the same
> cause?
> 

I kinda suspect not, but I'm not really sure.  There are several bugs along those lines, however, and some of them are probably related.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

This patch removes onCalendarUnregistering, since it's insufficent for our needs here, and replaces it with onCalendarUnregistered.  At some point in the future we can re-add onCalendarUnregistering, should it be necessary for something else.

As far as the XXX comments in the patch, addressing them will, at best, give us a slight performance improvement.  I propose to file another bug to look into that down the road.

Comment 7

[Joey Minta]

Comment on attachment 210403 [details] [diff] [review]
patch, v2

I don't really understand how this could work:

         this.mUnregisterCalendar.step();
         this.mUnregisterCalendar.reset();
 
         // delete prefs for the calendar too
         pp = this.mDeletePrefs.params;
         pp.calendar = calendarID;
         this.mDeletePrefs.step();
         this.mDeletePrefs.reset();
 
         delete this.mCache[calendarID];
+
+        this.notifyObservers("onCalendarUnregistered", [calendar]);

and then when we notify the observer, that calls removeCalendarFromTree(aCalendar) which does
+    var changedRow = getCalendars().indexOf(aCalendar);
+    boxobj.rowCountChanged(changedRow, -1);

So we remove the calendar from the database, and then try to find its index in the list of calendars?  Either I'm not understanding this codeflow correctly, or something is wrong here.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v3

Good catch, Joey.  I was misunderstanding how my own patch was working.  The real problem here is the lack of identity of "interface pointers" in JS.  Here's a new hackaround.  I'll post two more bugs about that (one for fixing in XPConnect, and one for tweaking the calendar interfaces to workaround XPConnect so that we continue to play nice in 1.5).  Then I'll file another bug to undo this hackaround once the calendar interfaces bug workaround lands.

Comment 9

[Joey Minta]

Comment on attachment 210501 [details] [diff] [review]
patch, v3

I suppose this is a pretty minimal perf hit.  Please cite the bug number of the bug you filed about interface pointers in the comment though. r=jminta with that.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

For some reason, this seems to have regressed.  However, since we now prompt for deletion, I dont think this regression needs to block 0.1.  I noticed this before I landed bug 328597 just now, so that is not the cause.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

A bunch of different people have complained about this since 0.3RC1, if we can get a fix in time that's suitably low-risk, we should consider taking it.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

I suspect the combination of the patches that landed in bug 328597 and bug 296202 caused this to regress.  In particular, we now both unregister and delete calendars in Lightning, and the Lightning observers for each of those actions both try to remove the calendar from the tree.  That said, I'm not able to reproduce this bug in my trunk build at the moment, though I do see it in the RC1 branch build.  So there may be more to this yet.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

The regression is better documented in bug 352667.  Re-closing here.