Calendar observer functions need to provide the actual calendar to callbacks

ASSIGNED
Assigned to

Status

Calendar
Internal Components
ASSIGNED
13 years ago
4 years ago

People

(Reporter: Joey Minta, Assigned: daiict218, Mentored)

Tracking

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(4 attachments)

(Reporter)

Description

13 years ago
Followup from bug 310503.  Because most of our errors actually go through the composite calendar, there's no easy way to determine which calendar actually had the error being reported to observers via the onError message.

Comment 1

13 years ago
When this happens, we should push the prompting of users from inside the calCalendarManager.js one layer outwards, since that code can't possibly know about what UI it's being called from (could be from sync code, the calendar code, an extension, etc).
OS: Linux → All
Hardware: PC → All
Unfortunately, I do not have cycles to work on Calendar stuff these days (just as it's getting to the good part!), so I am a bad owner for these bugs.  To delete the tragically-large chunk of bugspam, search for gregorianabdication.
Assignee: shaver → nobody
(Reporter)

Comment 3

12 years ago
Good first bug, simply modify the calIObserver interface and fix the consumers to accept the extra param.

onAdditem, onModifyItem, onDeleteItem, and onAlarm probably don't need them, since the calendar can be retrieved from the item param.  onStartBatch, onEndBatch, onLoad, and onError probably do need to be modified.
Whiteboard: [good first bug]

Comment 4

12 years ago
Created attachment 224782 [details] [diff] [review]
A first attempt for a patch

Following Joey Minta's advice, here's a first-attempt patch.

Comment 5

12 years ago
Created attachment 224786 [details] [diff] [review]
2nd try (missed some callers in the first try)

I found some callers that I forgot to patch using LXR.

Comment 6

12 years ago
I don't really know what to do for startBatch and endBatch in calICSCalendar.js, so that's missing from my patch right now.

Would it be okay to use 'this.calendar'?

Comment 7

12 years ago
Created attachment 224794 [details] [diff] [review]
3rd try - ready for review?

With some help from jminta on IRC, here's the 3rd iteration.

Things changed:

1) Use 'this' instead of 'this.mMemoryCalendar' in ICSCalendar.

2) Cleaned up remaining references to 'this.calendar' in errorAnnouncer. Since all calls to 'announceError' will come with the new aCalendar argument, removed 'this.calendar' and 'this.storedReadOnly' from the constructor as well.

3) Patched startBatch and endBatch in ICSalendar, by passing on 'this'.

This patch should now be ready for review, thanks.

Updated

12 years ago
Attachment #224794 - Flags: first-review?(jminta)
(Reporter)

Comment 8

12 years ago
Comment on attachment 224794 [details] [diff] [review]
3rd try - ready for review?

-    // We compare this to determine if the state actually changed.
-    this.storedReadOnly = calendar.readOnly;
We're still going to need to do this comparison, since we display different error message depending on whether or not the state switched.  (There's another bug on passing setToReadOnly as another argument to onError that would be a good follow-up here.)  For now, though, you need to keep this around (and probably still pass aCalendar).

-            this.mObserver.onError(calIErrors.CAL_UTF8_DECODING_FAILED, e.toString());
+            this.mObserver.onError(calIErrors.CAL_UTF8_DECODING_FAILED, e.toString(), null);

Passing null here doesn't look right.

It looks like you missed changes to calDav calendars and storage calendars (in calendar/providers/).
(Reporter)

Comment 9

12 years ago
Comment on attachment 224794 [details] [diff] [review]
3rd try - ready for review?

(cleaning out my review queue a bit).  Minusing per previous comment.
Attachment #224794 - Flags: first-review?(jminta) → first-review-

Comment 10

12 years ago
Created attachment 226047 [details] [diff] [review]
Add aSetToReadOnly to onError, include missing callers in caldav and storage 

Fixes the comments above.

Right now, for all onError calls, setToReadOnly=true. I don't know if that's correct or not.
Attachment #226047 - Flags: first-review?(jminta)
(Reporter)

Comment 11

12 years ago
Comment on attachment 226047 [details] [diff] [review]
Add aSetToReadOnly to onError, include missing callers in caldav and storage 

This is really coming along nicely.  The aSetToReadOnly stuff still needs some work, but it's close enough that I think we can continue with it here.  It's of course your call though.

The problem here is that we don't have defined semantics for what aSetToReadOnly means.  On the one hand, it could mean a severe error happened.  On the other, it could mean a severe error happened AND we weren't already in read-only mode.  Whichever way we decide that should go, it needs to be clearly documented in calICalendar.idl.

As I read your patch right now, it breaks the case where a severe error happens for a 2nd time, but I may be mis-reading.  The best way to test is to subscribe to an invalid ics calendar.  This should go into read-only right away.  Then refresh and you should see the still read only case.
(Reporter)

Updated

12 years ago
Assignee: nobody → duncanmak
(Reporter)

Comment 12

12 years ago
Comment on attachment 226047 [details] [diff] [review]
Add aSetToReadOnly to onError, include missing callers in caldav and storage 

minused, per previous comment, but very much looking forward to the next version.
Attachment #226047 - Flags: first-review?(jminta) → first-review-
(Reporter)

Comment 13

12 years ago
This needs to move fast if it's going to make 0.3.  Let me know if I need to do more to help push this forward.
Assignee: duncanmak → nobody
The first patch from bug 392561 contains the "onError" part of this bug.
Version: Trunk → unspecified
Whiteboard: [good first bug] → [good first bug][mentor=Fallen][lang=js]
(Assignee)

Comment 15

4 years ago
I would like to be assigned to this bug, Can someone assign it to me..
Sure, no problem! I think all that needs to be done for this bug is to pick up the patch, debitrot it and make sure all callers are adapted. Personally I don't see much use for the extra calendar yet, but I guess it won't hurt.

One more thing, if an IDL file is changed, the interface uuid needs to be changed.

Look for me on irc.mozilla.org #calendar if you need help getting set up. Build instructions are at:
https://developer.mozilla.org/En/Simple_Thunderbird_build
Assignee: nobody → daiict218
Status: NEW → ASSIGNED
Mentor: philipp
Whiteboard: [good first bug][mentor=Fallen][lang=js] → [good first bug][lang=js]
You need to log in before you can comment on or make changes to this bug.