215971 - auto refresh remote calendar every x minutes

Status: VERIFIED FIXED

(Calendar :: General, enhancement)

Description

[k b]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030812
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030812

wish:
option to automatically refresh remote calendar every x minutes/hours.
global option will do fine (for me, others may say otherwise).

Reproducible: Didn't try

Steps to Reproduce:

Comment 1

[k b]

maybee i was not clear enough, i am thinking of only downloading event and task
from remote to local. no upload, since upload should happen when
edding/removing/changing task/event.

tihs feature would be good if you can expect someone else to add tings to your
calendar, and you do not want to manually check every x minutes.

Comment 2

[Aleksey Nogin]

Ideally I would like the following preferences:

- auto refresh remote calendars every x minutes
- auto refresh remote calendar y minutes before each alarm

The idea of a second pref is to "double-check" whether the alarm is still there
before acting on it. And IMHO it should really be "y minutes before", not "right
before" since when it is already time to fire an alarm, it it IMHO to late to be
refreshing the remote calendar (since that could take a long time over a slow
network).

Comment 3

[Justin Wood (:Callek)]

*** Bug 248093 has been marked as a duplicate of this bug. ***

Comment 4

[Jaco Kroon]

My person here working on this has already managed to add a button to the menu
bar for easier refresh, but he does not seem to be able to set up some kind of
timing event.  Ie, I suppose I'm asking whether there is documentation on adding
configuration options, and then also, does anyone know where the code for
checking mail at regular intervals is located cause I suspect this code would
probably be similar.  Any pointers in this regard would be *very* usefull, we
could possibly even have a patch ready by next week.

Unless mostafah is already working on a patch?

Comment 5

[David Hendricks]

Attachment: modified fireAlarm() calls refreshAllRemoteCalendars()

This is as far as I was able to get. Any time an alarm fires, the remote
calendars automatically reload.

I wanted to create an event with associated alarm that would repeat every
minute, but the smallest repeat interval supported by Sunbird and the
underlying library looks like one day.

I then tried to create an event programmatically that would fire an event in a
minute, upon which remote calendars would refresh and the alarm would be reset
to go off in another minute. The plan was to somehow flag this as a "hidden"
event, so that there would be no popup when it fires, no notation in the
calendar that it exists, and no entry in the .ics file.

The hitch is that I couldn't successfully create an event without bringing up
the New Event dialog. Someone else may be able to do it, feel free to try.

Looks to me like someone's going to have to do something in xpcom to create a
special event that fires an alarm at regular (configurable) intervals, and is
so flagged to be hidden from the user.

Comment 6

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

Comment on attachment 177864 [details]
modified fireAlarm() calls refreshAllRemoteCalendars() 

I think the intention of this patch is to abuse alarms for reloading. If it is,
it isn't the way to go.
If is isn't, it don't see how it fixes this bug.

Comment 7

[Joey Minta]

*** Bug 283169 has been marked as a duplicate of this bug. ***

Comment 8

[Joey Minta]

*** Bug 239682 has been marked as a duplicate of this bug. ***

Comment 9

[Lovell]

*** Bug 292651 has been marked as a duplicate of this bug. ***

Comment 10

[Joey Minta]

Attachment: auto-reload

With the new calendarManagement/providers, this is pretty easy.  Removes the
'reload remote calendars on startup' option, since that is obsolete and
replaces it with 'automatically reload remote calendars every X minutes'.

Comment 11

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

Comment on attachment 191471 [details] [diff] [review]
auto-reload

>+++ mozilla/calendar/resources/content/calendarManagement.js	3 Aug 2005 11:50:26 -0000

I'm not sure about this patch. My plan was to put the autoreload into the ics
provider, and combine it with syncing, to prevent dataloss and all
On the other hand, this patch is simple enough, and does most of the trick. We
would need a 'download before changing' though. Not sure how and where to do
that. Autopublish is already part of the ics provider (and can't be turned of)

Comment 12

[Lovell]

I would like to try this patch but I am unsure of how to download and install
it.  Can someone tell me how to do this?

Comment 13

[Joey Minta]

Sending back to default.  If the changes are going to be done in the providers,
I probably won't be writing it.

Comment 14

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

Attachment: work in progress

This is what i have so far. Still need to work on adding a pref.

Comment 15

[Simon Paquet [:sipaq]]

Can we try to drive this in for Sunbird 0.3 beta?
This is a very popular enhancement request from our users as can be seen by the votes and the dupes this bug has.

Comment 16

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

Attachment: reload from calendarmanager

The problem with the previous patch was that the prefs could not be read. Fixed that by putting the refresh in the calendar manager. That is better anyway, because other providers also might need/want to be reloaded.
But this patch still has a problem: the views redraws the eventboxes. This makes the boxes flicker quite visibly. It might also be slow. This should be prevented somehow.
Patch is also not finished. Still needs a user pref for the refresh time-out, and some UI to turn on auto-refresh. Attaching anyway, because i don't want to loose the patch :)

Comment 17

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

Attachment: UI work

This patch add some UI. It doesn't fully work yet, the prefs panel doesn't want to remember the new value.

But while working on this, I wondered if this is the right way. I wondered what the usecase was for disabling auto-reload. In which cases would you want outdated information? Shouldn't auto-reload be always on (for providers that support it)?

Comment 18

[Matthew (lilmatt) Willis]

(In reply to comment #17)
> I wondered what the usecase was for disabling auto-reload. In which cases
> would you want outdated information? Shouldn't auto-reload be always on (for
> providers that support it)?

Possible usecase ideas:
- Working offline in the same fashion as Tb 
- Users on dialup who don't want the app calling their isp every x minutes just because Sunbird is open
- If your published calendar is hosed or otherwise updated by another app somewhere else and you don't want to lose your cached data just yet (so you can print it out, etc.)

Comment 19

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

All those reasons sound like one global pref would do, instead of a per-calendar setting. It would remove the need for a new bit of UI in the edit-calendar dialog, and thus would reduce confusion. It would also reduce the number of code paths. So I'm in favor, hoping i'm not missing an use-case for disabling reload.

Comment 20

[Stefan Sitter [:ssitter]]

(In reply to comment #17)
> This patch add some UI. It doesn't fully work yet, the prefs panel
> doesn't want to remember the new value.

Don't forget to add the new pref to the <preferences> section too.

In my opinion it is sufficient if there is one bool pref to enable/disable auto-refresh globally and one int pref to set the refresh timeout globally.

Comment 21

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

Attachment: only a pref

This patch should do it all. Adds a pref to enable autoreload, and one to set the timeout. Makes all calendars that supprt reload, reload.

Asking lilmatt for review, mainly because of the preferences part of the patch. Asking dmose mainly for the backend changes.

Comment 22

[Matthew (lilmatt) Willis]

Comment on attachment 231477 [details] [diff] [review]
only a pref

+        dump("Changed to: "+autoRefreshPref+"\n");
Let's not ship the dump.

+            <caption label="&pref.refreshbox.label;"/>
+                <checkbox label="&pref.autorefresh.label;"
These entities don't seem to already exist, nor are they in your patch.

+                <textbox id="refreshtimeout"
+                         preference="calendar.autorefresh.timeout"
+                         maxlength="3"
+                         size="3"/>
Do we want to use the ensurePositiveInteger() here? We use it in the Alarms already I think.


Minusing on the missing entities...

Comment 23

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

Attachment: updated patch

Path now has l10n changes, plus the validateNaturalNums in the pref pane

Comment 24

[Matthew (lilmatt) Willis]

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

Index: locales/en-US/chrome/calendar/calendar.dtd
===================================================================
+<!ENTITY calendarproperties.autorefresh.label "Refresh calendar every N minutes">
Delete this line per IRC.


Index: locales/en-US/chrome/calendar/preferences/general.dtd
===================================================================
+<!ENTITY pref.refreshbox.label "Refresh Settings" >
+<!ENTITY pref.autorefresh.label "Refresh calendars every" >
These should be changed to "Reload" to match the current icon label and menuitems. (and Firefox)

r=lilmatt with that

Comment 25

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

to follow the rules, i need to ask ui-review.

Comment 26

[ma2412ma]

*** Bug 356383 has been marked as a duplicate of this bug. ***

Comment 27

[Matthew (lilmatt) Willis]

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

Moving r2 to jminta per dmose

Comment 28

[Joey Minta]

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

As I read this patch, the auto-refresh will result in the back-end and the front-end falling out of sync.  Because we've refreshed the back-end, our etag checks will turn out fine when writing the file, resulting in silent over-write of conflicts.  That seems to me like a step backwards in terms of dataloss, or am I misunderstanding something?

Comment 29

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

I'm not sure what you mean.
The front-end (views etc) should stay in sync, thanks to some observers.
And i don't know what you mean with etags. They don't have anything to do with the front-end.

Comment 30

[Joey Minta]

(In reply to comment #29)
> The front-end (views etc) should stay in sync, thanks to some observers.
Just refreshing the calendar fires the observers?  Which ones and when?

Comment 31

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

They have to be fired, otherwise our current UI to reload calenders would not work. That UI in the end calls the same code.

The code is as follows: the ics calendars add all the loaded items to the memory calendar: http://lxr.mozilla.org/seamonkey/source/calendar/providers/ics/calICSCalendar.js#334
Then the memory calendar notifies the observers: http://lxr.mozilla.org/seamonkey/source/calendar/providers/memory/calMemoryCalendar.js#208

A smart view might ignore those observations, because there is a batch mode. But I'm not sure if our current views are smart like that.

Comment 32

[Joey Minta]

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

+    this.mRefreshTimer = false;
How about null?

+    setUpPrefObservers: function() {
I'd really prefer if we didn't add any more anonymous functions to our code.  Here and elsewhere.

+        var refreshEnabled = 0;
Should default to |false|.
+        try {
+            var refreshEnabled = prefBranch.getBoolPref("calendar.autorefresh.enabled");
+        } catch (e) {
+        }
You're redeclaring a variable here, same thing below.

The way we're using observers with the memory calendar feels like an abuse to me, but that's not a battle for this bug.  Also, this code will similarly suffer from  Bug 282013, but I'd be ok with that being a followup (that we may want to fix at the same time as bug 357079). Lastly, we probably need a followup about leaking the prefobservers at shutdown.  r2=jminta with the above nits picked.

Comment 33

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

patch checked in

Comment 34

[Damian Szczepanik]

this issue needs more than one or two tests, eg what if event is edited and after refresh is not available any more or has changed? - integration

Comment 35

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

No, it does not need a bunch of testcases. The bug is all the summary says: "auto reload every x minutes" Nothing more.
Concurrent editing and all that are not included, and are known to be broken. All the patch in this bug does is making the time window in which dataloss can occur somewhat smaller. But it's not gone yet.

Comment 36

[Damian Szczepanik]

(In reply to comment #35)
> No, it does not need a bunch of testcases. The bug is all the summary says:
> "auto reload every x minutes" Nothing more.

Yes, I know - I wanted to say that we need focus what kind of bug this fix can produce in the future - so anyway testing this feature is a good way

Comment 37

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20061225 Calendar/0.4a1

Comment 39

[Stefan Sitter [:ssitter]]

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

The changes in preference UI from this patch (
mozilla/calendar/base/content/preferences/general.js and
mozilla/calendar/base/content/preferences/general.xul)
have not been checked in yet. Reopen bug?

Comment 40

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

Finally checked in the missing parts.

Comment 41

[Damian Szczepanik]

Litmus testcase 3000 created