Closed Bug 389397 Opened 17 years ago Closed 17 years ago

Use async providers -> reload remote calendars doesn't work

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: andreas.treumann, Assigned: dbo)

References

Details

(Whiteboard: [gdata-0.3])

Attachments

(3 files)

STEPS TO REPRODUCE:
===================

- use only WCAP calendars
- try to reload this calendars

RESULT:
=======

- nothing happens, no view refresh 

EXPECTED RESULT:
================

- reload remote calendar should work.

REPRODUCIBLE:
=============

- always
The cause is that the remote providers (WCAP, CalDAV, gdata) don't send out an onLoad when being refreshed. The views react on onLoad(), thus IMO we should make it mandatory when calendars are explicitly refreshed. onLoad() states that a calendar does not know its detailed changes (which is the most defensive behaviour right now). In the future a provider might retrieve the diff of changes from its server and notify only those.
Status: NEW → ASSIGNED
Flags: blocking-calendar0.7+
Summary: Use only WCAP calendars -> reload remote calendars doen't work → Use async providers -> reload remote calendars doen't work
Target Milestone: --- → 0.7
- Optimizes composite, sending out a single onLoad() for the composite's refresh().
- While fixing composite and caldav, I added a listener bag implementation and consolidated the providers using it (using compareObjects for listener management).
- wcap fix
Attachment #273958 - Flags: review?(michael.buettner)
Attached patch caldavSplinter Review
Attachment #273960 - Flags: review?(browning)
Attached patch gdataSplinter Review
Attachment #273961 - Flags: review?(philipp)
- caldav, gdata untested(!)
Component: Provider: WCAP → General
QA Contact: wcap-provider → general
Summary: Use async providers -> reload remote calendars doen't work → Use async providers -> reload remote calendars doesn't work
Is this related to Bug 374972 and Bug 361520?
However I am wondering about why the toolbar reload button only refreshes the composite calendar (which has only the view visible ones in): it does not refresh alarms/todo-list etc. from calendars that are currently unchecked. IMO we should refresh always the whole set of calendars (like the auto update timer in calendarManager).
Opinions? Follow-up bug?
(In reply to comment #9)
> However I am wondering about why the toolbar reload button only refreshes the
> composite calendar (which has only the view visible ones in): it does not
> refresh alarms/todo-list etc. from calendars that are currently unchecked.
I think in light of this argument it makes sense to update all calendars. Feel free to add this to this patch or file a follow-up to your liking.
If we currently update all calendars (like auto update timer in calendar manager), this leads to a burst of onLoad notifications, leading to unnecessary view refreshes.
In the attached patch, I have at least optimized the composite calendar to send a single onLoad event after refreshing the contained calendars (ignoring onLoad events of the contained events during that time). But if any of the contained calendars will be refreshed from a third party, it logically leads to the composite calendar sending an onLoad (because one of its contained ones has been refreshed). Thus if we fix my comment #9, it will lead to unnecessary view refreshes again.
First of all, this patch contains the new calListenerBag (which is quite nifty code, btw) plus the actual fix for the problem. I think since the amount of code is not overwhelming we can leave this patch is it is for now, but for larger refactoring stuff I'd suggest to stick to the "one issue per bug" stigma.

Second, while trying to further break down the patch, I'm wondering whether or not the one line you added to the WCAP provider...
>  // notify about refreshed calendar:
>  this.notifyObservers("onLoad", [this]);
...is enough to fix the problem while all the rest of the patch is all about the optimization in order to not spam the views with tons of onLoad() calls. Am I right, or did I miss the obvious?

Another question is why you didn't call onLoad() in e.g. the ICS provider? Specifically, around the code loction at [1]. While thinking more about it, you're calling onLoad() after the composite calendar has been refreshed. So, what's the reason for the providers to call onLoad() after being refreshed at all? It would be great if you could elaborate a bit on that.

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/providers/ics/calICSCalendar.js#173
(In reply to comment #12)
> First of all, this patch contains the new calListenerBag (which is quite nifty
> code, btw) plus the actual fix for the problem. I think since the amount of
> code is not overwhelming we can leave this patch is it is for now, but for
> larger refactoring stuff I'd suggest to stick to the "one issue per bug"
> stigma.
There have been two reasons why I finally wanted to get rid of this (all over) duplicated listener code:
- I'd have needed to add more verbose code to caldav
- None of that code actually uses compareObjects (which they should to be safe from wrapper objects)
- it leads to less code
thus I thought it's better to review, actually. But I generally agree to have one bug per issue.

> Second, while trying to further break down the patch, I'm wondering whether or
> not the one line you added to the WCAP provider...
> >  // notify about refreshed calendar:
> >  this.notifyObservers("onLoad", [this]);
> ...is enough to fix the problem while all the rest of the patch is all about
> the optimization in order to not spam the views with tons of onLoad() calls. Am
> I right, or did I miss the obvious?
WCAP currently has a unified listener bag at the shared session object which I plan to consolidate soon.
- The core fix that all remote providers are loacking is to send an onLoad event when being refreshed (beause that refreshes the views).
- The composite calendar optimization however is about sending a single onLoad (which logically matches the single refresh called on it) and *not* sending a dozen onLoad events for a single refresh call on the composite.

> Another question is why you didn't call onLoad() in e.g. the ICS provider?
> Specifically, around the code loction at [1]. While thinking more about it,
> you're calling onLoad() after the composite calendar has been refreshed. So,
> what's the reason for the providers to call onLoad() after being refreshed at
> all? It would be great if you could elaborate a bit on that.
onLoad is sent out asynchronously in ICS (after the file has been loaded), because ICS cannot determine the detailed changes and notify those e.g. via onMotifyItem. So it just got the listener bag simplification.
Comment on attachment 273958 [details] [diff] [review]
composite, memory, storage, ics, wcap

>     refresh: function() {
>+        this.mObserverHelper.suppressOnLoad = true;
>+        try {
>+            for each (cal in this.mCalendars) {
>+                    try {
>+                        if (cal.canRefresh) {
>+                            cal.refresh();
>+                        }
>+                    } catch (e) { }
>+                }
>+        } finally {
>+            this.mObserverHelper.suppressOnLoad = false;
>+            // send out a single onLoad for this composite calendar:
>+            this.mObservers.notify("onLoad", [this]);
Basically, I was thinking about what happens if some specific provider answers asynchronously and sends the onLoad() after the 'for each' loop in the above code has been finished? You just block onLoad() calls that happen as long as you're iterating the calendars. It's not the end of the world, as you're not blocking all onLoad() calls that possibly can occur.

But while thinking about a possible solution I could imagine that the composite calendar could wait for all calendars having their onLoad() event broadcasted, before sending out the final onLoad(). But this would be bad for two reasons. 1) the views would have to wait for the slowest calendar and 2) in case a provider fails to send an onLoad() we're probably waiting in the composite calendar forever.

This observation leads me to the conclusion that the patch is just fine as it is now. By the way, the indentation is not correct in the above function (the closing brace of the for each statement is not correctly aligned). r=mickey.
Attachment #273958 - Flags: review?(michael.buettner) → review+
Comment on attachment 273960 [details] [diff] [review]
caldav

tested/working on CalDAV; my comments are mostly style nits

>-                thisCalendar.observeModifyItem(aNewItem, aOldItem.parentItem);
>+                thisCalendar.mObservers.notify("onModifyItem", [aNewItem, aOldItem.parentItem]);

I realize we're not religious about the 80-column rule, but this would be more readable and style-compliant if you'd wrap it after aINewItem


>-                this.mICSService = Cc["@mozilla.org/calendar/ics-service;1"].
>-                                   getService(Components.interfaces.calIICSService);
>-                var rootComp = this.mICSService.parseICS(calData);
>+                if (!thisCalendar.mICSService) {
>+                    thisCalendar.mICSService = Cc["@mozilla.org/calendar/ics-service;1"].
>+                                               getService(Components.interfaces.calIICSService);
>+                }

again, this could be wrapped better. At the very least, calIICSService should be on a new line


>-                    thisCalendar.observeModifyItem(item, aItem.parentItem);
>+                    thisCalendar.mObservers.notify("onModifyItem", [item, aItem.parentItem]);

please wrap after "onModifyItem"

Also, there is a redundant definition of refresh() at line 1377 of the (patched) file which can/should be gotten rid of.

r=browning with that

Drive-by comment on the main patch: I think it would be preferable to use Mozilla-style arg names (e.g. aIid, aListener instead of iid, listener).

I understand the reasoning behind always querying calendars whether or not they are checked in the calendar list, but I wonder if we don't have an issue there wrt users (say laptop users) with intermittent network connections. With the current UI, it would be I think reasonable for them to construe unchecking the calendar as disabling it completely rather than simply suppressing (mostly) its display - I know that's what I initially thought the checkbox did. So I initially tried to use it to disable remote calendars when using Sb on a laptop that happened to be off-net, to no good result. If we're going to alway query calendars we need to make sure that the async providers handle the no-net situation well. CalDAV is getting better in this regard, but it's not all the way there yet; I don't know about the others.
Attachment #273960 - Flags: review?(browning) → review+
(In reply to comment #15)
> I understand the reasoning behind always querying calendars whether or not
> they are checked in the calendar list, but I wonder if we don't have an
> issue there wrt users (say laptop users) with intermittent network
> connections.
It strikes me that this is related to our offline-story. In case we have a decent support for being offline, we should solve such issues in one fellow swoop. As I currently envision the offline support it's structured around synchronizing two versions of a calendar (the local one on the harddrive and the remote calendar). So in light of this patch, I think we should take it as Daniel suggested and wait until the offline support is finally available.
Comment on attachment 273961 [details] [diff] [review]
gdata

Looks fine, r=philipp
Attachment #273961 - Flags: review?(philipp) → review+
Whiteboard: [gdata-0.2.2]
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Verified in lightning 2007073103 -> task is fixed.
Status: RESOLVED → VERIFIED
Did anybody verify this in Sunbird? An up2date build still shows the wrong behavior in Sunbird.
In sunbird 'reload remote calendar' is always disabled. But after a view reload (week view/month view) the reload works.
Whiteboard: [gdata-0.2.2] → [gdata-0.3]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: