285018 - reload remote calendar doesn't work

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

file->reload remote calendars:

JavaScript strict warning: chrome://calendar/content/calendar.xul, line 1:
reference to undefined property
gCalendarWindow.calendarManager.refreshAllRemoteCalendars
JavaScript error: chrome://calendar/content/calendar.xul, line 1:
gCalendarWindow.calendarManager.refreshAllRemoteCalendars is not a function

Now if reloading remote calendar is useful open for debate. I think it should
happen automaticly every n minutes, except when offline. But then the UI should
be removed.

Comment 1

[Stuart Parmenter]

For this, we should add a refresh() method to calICalendar and simply enumerate
over each calendar and call refresh.  It would then be up to each provider to
update its calendar in the best way.

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

My idea is that the user shouldn't have to worry about reloading. It should
always be automatic. and if a provider can't do it automatic, it should have a
timer and make it look automatic. (that's what i'm planning for remote ics)
This means that the UI can be removed.

But until we have that, a .reload() might do.

Comment 3

[Mike Shaver (:shaver emeritus)]

I think we'll probably want some explicit reload mechanism.  People who are
working to test their remote calendar setup will often want to see the new
changes _now_, and not in N minutes when the timer fires.  Maybe if we get it
really solidly right, though, we can move into such a brave new world.

But I agree that a user should not _need_ to reload a remote calendar in the
course of normal operation, in which they don't have "out of system" knowledge
about the arrival pending changes to the remote store.  Even so, we will need
some programmatic way to get the providers up-to-date in preparation for an
explicit offline transition or calendar export, so the interface addition seems
wise.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: initial patch v1

Patch has a suggested interface, plus a very simple impl for the ics provider.
This is initial work. In an ideal world, the ics provider only submits
notifications on the changed events. (we need that anyway for a cached ics, so
it will come one day)

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 188867 [details] [diff] [review]
initial patch v1

>         streamLoader.init(channel, this, this);
>+        
>+        return false;
>     },

Why return false here?	refresh is defined as void-return.

I don't see you implementing refresh on the composite calendar, but your
reloadCalendars function seems like it'll call it there, which would do nothing
but throw an exception.

Can you implement it for composite and storage (a no-op), and stub it to throw
NYI for CalDAV?

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: updated patch

Updated patch. Fixes the review comments.

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 190330 [details] [diff] [review]
updated patch

>+    refresh: function() {
>+        for (cal in this.mCalendars) {
>+            try {
>+                this.mCalendars[cal].refresh();
>+            } catch(e) {}
>+        }
>+    },

for each (cal in this.mCalendars) {
    try { cal.refresh(); } catch (e) { }
}

r=shaver.

Comment 8

[Stuart Parmenter]

Do we really want a refresh() method on calICalendar? I would think it should be
on something like calIRemoteCalendar instead.

Comment 9

[Mike Shaver (:shaver emeritus)]

I think it's fine there -- it's at worst a no-op, and I think it's better to
have there than on another interface that will be hard to name.  (Is the
composite a remote one?)

I regret that we split that stuff in RDF land, so long ago.

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

checked in

Comment 11

[Joey Minta]

-  <command id="reload_remote_calendars"
oncommand="gCalendarWindow.calendarManager.refreshAllRemoteCalendars()"/>
+  <command id="reload_remote_calendars" oncommand="reloadCalendars()"/>

Please don't forget about changing Sunbird's commands too...(I don't really feel
like re-opening bug 298772 as well, so maybe just fix them both here, please?)

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

ok, i updated sunbird

Comment 13

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: Sunbird Only. Be afraid for your sanity!