Closed
Bug 420228
Opened 16 years ago
Closed 16 years ago
Cannot dismiss alarms on CalDAV calendars
Categories
(Calendar :: Provider: CalDAV, defect)
Calendar
Provider: CalDAV
Tracking
(Not tracked)
RESOLVED
FIXED
0.8
People
(Reporter: browning, Assigned: browning)
Details
Attachments
(1 file, 1 obsolete file)
1.90 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
The (!aListener) test in the CalDAV provider's getUpdatedItem() is inappropriate, since the alarm service calls modifyItem() with a null listener.
Attachment #306438 -
Flags: review?(ctalbert)
Flags: wanted-calendar0.8?
Comment 1•16 years ago
|
||
You say that modifyItem calls this with a null listener, but if aItem is null, the listener is called regardless. I don't quite understand the interaction with modifyItem though.
Assignee | ||
Comment 2•16 years ago
|
||
Right you are, we need to handle the case where both aItem and aListener are null. What's happening is that the alarm service calls modifyItem when dismissing the alarm, which with CalDAV results in the item being PUT to the server. CalDAV servers often alter items upon PUT (with X-props and the like), so we always refetch the item immediately after a PUT. This is done by calling getUpdatedItem with the item and the listener from the original modifyItem() call. On an alarm dismissal, that listener will be null, so getUpdatedItem must not bail when being called with a null listener.
Assignee: nobody → browning
Attachment #306438 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #306592 -
Flags: review?(ctalbert)
Attachment #306438 -
Flags: review?(ctalbert)
Comment 3•16 years ago
|
||
Comment on attachment 306592 [details] [diff] [review] handle case where both aItem and aListener are null >Index: calendar/providers/caldav/calDavCalendar.js >=================================================================== >RCS file: /cvsroot/mozilla/calendar/providers/caldav/calDavCalendar.js,v >retrieving revision 1.45.2.69 >diff -p -8 -u -r1.45.2.69 calDavCalendar.js >--- calendar/providers/caldav/calDavCalendar.js 23 Feb 2008 20:53:52 -0000 1.45.2.69 >+++ calendar/providers/caldav/calDavCalendar.js 29 Feb 2008 21:32:31 -0000 >@@ -689,27 +689,29 @@ calDavCalendar.prototype = { > /** > * Retrieves a specific item from the CalDAV store. > * Use when an outdated copy of the item is in hand. > * > * @param aItem item to fetch > * @param aListener listener for method completion > */ > getUpdatedItem: function caldavGUI(aItem, aListener) { >- if (!aListener) { >- return; >- } > > if (aItem == null) { >- aListener.onOperationComplete(this.superCalendar, >- Components.results.NS_ERROR_FAILURE, >- aListener.GET, >- null, >- "passed in null item"); >- return; >+ if (aListener) { >+ aListener.onOperationComplete(this.superCalendar, >+ Components.results.NS_ERROR_FAILURE, >+ aListener.GET, >+ null, >+ "passed in null item"); >+ return; >+ } else { >+ LOG("passed in null item and null listener for getUpdatedItem"); >+ return; >+ } I'd just shorten this to if (aItem == null) { if (aListener) { aListener.onOp... } return; } No need for the double return, and I don't think a LOG message is needed since this is a quite common case. r=philipp with those changes (Clint, I hope its ok I'm taking your reviews, I was just available and since the rc is coming up on monday I thought I'd take over :) > } > > var itemType = "VEVENT"; > if (aItem instanceof Components.interfaces.calITodo) { > itemType = "VTODO"; > } > > var queryStatuses = new Array();
Attachment #306592 -
Flags: review?(ctalbert) → review+
Updated•16 years ago
|
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Keywords: checkin-needed
Assignee | ||
Comment 4•16 years ago
|
||
patch checked in on HEAD and MOZILLA_1_8_BRANCH with reviewer's changes ->FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: wanted-calendar0.8+ → wanted-calendar0.8?
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Updated•16 years ago
|
Keywords: checkin-needed
Target Milestone: --- → 0.8
You need to log in
before you can comment on or make changes to this bug.
Description
•