lightning pane doesn't repaint after calendar added or deleted

RESOLVED FIXED

Status

Calendar
Lightning Only
P2
normal
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: dmose, Assigned: dmose)

Tracking

Details

(Whiteboard: [patch needed])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 208918 [details] [diff] [review]
work in progress patch, v1
(Assignee)

Comment 2

13 years ago
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.
Dmose, is this related to Bug 305578? Not the same problem but maybe the same cause?
(Assignee)

Comment 4

13 years ago
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.
Blocks: 298366
(Assignee)

Comment 5

13 years ago
(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.

Updated

13 years ago
Depends on: 325350

Updated

13 years ago
No longer depends on: 325350
(Assignee)

Comment 6

13 years ago
Created attachment 210403 [details] [diff] [review]
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.
Attachment #208918 - Attachment is obsolete: true
Attachment #210403 - Flags: first-review?(jminta)

Comment 7

13 years ago
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.
(Assignee)

Comment 8

13 years ago
Created attachment 210501 [details] [diff] [review]
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.
Attachment #210403 - Attachment is obsolete: true
Attachment #210501 - Flags: first-review?(jminta)
Attachment #210403 - Flags: first-review?(jminta)

Comment 9

13 years ago
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.
Attachment #210501 - Flags: first-review?(jminta) → first-review+
(Assignee)

Comment 10

13 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

12 years ago
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.
No longer blocks: 298366
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

12 years ago
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.
Whiteboard: [patch needed]
Target Milestone: --- → Lightning 0.3
(Assignee)

Updated

12 years ago
Priority: -- → P2
(Assignee)

Comment 13

12 years ago
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.
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 14

12 years ago
The regression is better documented in bug 352667.  Re-closing here.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: Lightning 0.3 → ---
You need to log in before you can comment on or make changes to this bug.