Closed Bug 420749 Opened 12 years ago Closed 12 years ago

CalDAV calendars must refresh twice to display server-side changes

Categories

(Calendar :: Provider: CalDAV, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: browning, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

STR:
1) Start Sb with a CalDAV calendar in profile
2) Using some other client, modify an item on that calendar
3) Refresh in Sb. [at this point, Sb queries the server for etags, notes the changed etag, and downloads/caches the changed item]
4) Note updated item not displayed in UI
5) Refresh in Sb again [Sb queries the server for etags, finds no deltas, and does not request calendar-data]
6) Note updated item displayed in UI
Find the daterange for the item and compare it to the view. If it's in the view, refresh view from cache, if not, do nothing?
This appears to be another issue with caching. Backing out the patch for bug 418542 fixes it at the cost of making it impossible to leave CalDAV calendars in the unchecked/undisplayed state. It also seems to be fixable by doing the "onLoad" notification to observers *twice* at the end of a refresh, at the cost of a double flicker in the display and the commission an obvious hack. Still, I'm inclined toward the latter solution and will post a patch if further testing doesn't uncover problems with that approach. 
Attached patch fire onLoad twice (obsolete) — Splinter Review
This is really pretty ugly, and doubtless someone who understood the interactions between these various caches better than I do now could find a better solution. Whether we can find that fix in time for 0.8 I don't know. But if we can't, I would think we need to either do this or back out the patch in bug 418542: to my mind must-refresh-twice is a show-stopper whereas can't-keep-calendars-invisible is merely immensely annoying.
Attachment #307371 - Flags: review?(philipp)
Drat. Need to double up the onLoad in getUpdatedItems() as well, to catch items that have been deleted from the server by another client.
(Note also that this patch includes the two-line deletion from bug 420854)
Attachment #307371 - Attachment is obsolete: true
Attachment #307377 - Flags: review?(philipp)
Attachment #307371 - Flags: review?(philipp)
I really should have flagged this earlier, as I really don't think we should release 0.8 with this CalDAV refresh() behavior. As I see it, our options, in descending order of preference, are:
1. Find a proper fix. Might be hard to do in time for 0.8
2. Take this ugly patch
3. Roll back the patch from bug 418542 and release with that bug in place

We need to do #1 at some point; the question is whether we do that before or after the 0.8 release. I'm open-minded about the ordering of #2 and #3.
Flags: blocking-calendar0.8?
(In reply to comment #5)
> I really should have flagged this earlier, as I really don't think we should
> release 0.8 with this CalDAV refresh() behavior. As I see it, our options, in
> descending order of preference, are:
> 1. Find a proper fix. Might be hard to do in time for 0.8
> 2. Take this ugly patch
> 3. Roll back the patch from bug 418542 and release with that bug in place

Bruno, we're pretty late in the game and the RC1 builds are probably already in the making. To choose between these three options, it would be good to know how risky each of these three options are.

I would suggest to talk with Sebo and/or Philipp about option 1 to see if they have any suggestions. For option 2 or 3 I would defer to your judgement, but would personally prefer option 3.
I would like to insist on the fact that if the bug 418542 is not resolved for the 0.8 release, lightning become absolutely unusable. the severity of bug 418542 is set to normal but IMHO it should have been set at "Blocker" because without this, I can not start Lightning at all and can not test it (see : https://bugzilla.mozilla.org/page.cgi?id=fields.html#bug_severity)

IMHO #2 is a good option, maybe this patch is ugly, but if it resolve bug 418542 and this one, the end user will not see the ugly stuff. 
for the 0.9 I hope that there will be time to make #1

Sorry to give my opinion and to interfere, but I really think that bug 418542 is  a blocker one
I'm with Maxime unless of corse there's a proper fix
I don't know what changed, but I can no longer reproduce this bug with Sunbird 0.8 RC1 on Linux, calendar on Bedework. Given the severity of the bug and some of the chatter on irc today, I'd like to have confirmation from others before closing this bug worksforme.
Duplicate of this bug: 421478
bug 421478 was someone seeing another side of this problem, where newly created events also need to be refreshed twice in order to appear in the views, though they are created properly on the server. That reporter was using RC1 against a Scalix server.
Some additional context wrt comment #7: Maxime has more than a dozen CalDAV calendars in-profile and needs to keep most of them disabled at any given time. Bug 418542 prevented disabling them, so at startup Lightning would attempt to download and parse all of them simultaneously, pegging the CPU. This is a rare-but-valid use case, and is the only reason I ordered options as I did in comment #5.

I regard the patch for bug 418542 as a temporary fix. I strongly suspect that that bug has the same underlying cause as bug 412963, which unfortunately is not slated to be fixed for 0.8. A fix for that would probably fix 418542 and this (420749) as well.
I have seen the create/need-to-refresh problem on the Oracle CalDAV server as well, but here is a little bit more info:

Upon starting the RC1, I see created events immediately, I only start to see the problems described in bug 421478 after I hit "Ctrl-R" a few times, after that I need to re-hit ctrl-r to see the events I have just created, it seems that what the UI displays and what's in the server gets out of sync after reloading the calendar.

Ths patch seems to fix the problem for me. It does so by backing out the original fix for bug 418542 and fixing that bug in a way that does not cause the current one. It's still hackish, though, and is more a demonstration of where the problem is than a proper fix. The problem is that for some reason we are calling ensureCalendarVisible() every time we add or modify an item. I don't really understand why we're doing this: adding/modifying items on an invisible calendar sounds like a common use-case to me (example: I have a shared, remote, automatically-refreshed calendar whose events I don't need to see at the moment, but whose alarms I want. refresh() needs to be able to fetch deltas and put them in the calendar without forcing it visible).

In bug 412963 comment #4 Philipp seemed to think that ensureCalendarVisible was getting called only in sync scenarios (though I may be reading the comment wrong). Given that offline is still experimental, I think that having bug 418542 only with caching enabled would be acceptable/relnotable for 0.8, and this patch could be changed to check for !cache instead of !caldav. But again, I really don't understand why ensureCalendarVisible was initially considered necessary for addItem/modifyItem, so I really can't assess the risk here.
Attachment #307377 - Attachment is obsolete: true
Attachment #308136 - Flags: review?(philipp)
Attachment #307377 - Flags: review?(philipp)
> The problem is that for some reason we are calling
> ensureCalendarVisible() every time we add or modify an item.

If I recall correctly this was done to fix the following issue many users experienced: They have some hidden calendars and one of them is selected. They create a new event, enter all the information and save it. But there is no new event visible (because the calendar is hidden). They try again and again and finally curse on Sunbird because it seems that it's not possible to create events. Therefore a hidden calendar is made visible if the user creates an event in it.
(In reply to comment #15)
> Therefore a hidden calendar is made visible if the user creates an
> event in it.
> 

That makes sense, thanks for the explanation. Strikes me that the operative phrase there is "the user creates", and that we have differing needs depending on whether  the new/modified item is from user action or, say, refresh(). Should we maybe be doing ensureCalendarVisible in the event/task dialog (and I suppose import) rather than on every add/modify? The CalDAV provider recently got a lot smarter about network usage, and now fetches only new/changed items on refresh(). It needs to be able to get those items into its memoryCalendar cache without triggering ensureCalendarVisible.
(In reply to comment #16)
> 
> That makes sense, thanks for the explanation. Strikes me that the operative
> phrase there is "the user creates", and that we have differing needs depending
> on whether  the new/modified item is from user action or, say, refresh().
> Should we maybe be doing ensureCalendarVisible in the event/task dialog (and I
> suppose import) rather than on every add/modify?
That sounds good. It would also solve a problem with alarms, where dismissing/snoozing makes hidden calendars visible which is also not wanted, imo.
(In reply to comment #17)

> That sounds good. It would also solve a problem with alarms, where
> dismissing/snoozing makes hidden calendars visible which is also not wanted,
> imo.
> 

I'll start working on a patch that will do that, but in the meantime I'm leaving the review request in place. Assuming that the only reason we're using ensureCalendarVisible there is the one Stefan explained in comment #15, I think that a) a relnote along the lines of "Adding an item to a CalDAV calendar that is unchecked in the calendar list does not cause that calendar to become visible" is acceptable in a 0.8 release, and b) almost anything else we do will be higher-risk, and at this point in the release cycle it's best to avoid riskier patches to the extent that we can. Of course, opinions on both those points will likely vary. :-)
 
Comment on attachment 308136 [details] [diff] [review]
don't ensureCalendarVisible

>-        ensureCalendarVisible(aItem.calendar);
>+        if (aItem.calendar.type != "caldav") {
>+            ensureCalendarVisible(aItem.calendar);
>+        }
I'm not sure this is the best solution, but I guess I can live with it for now. This means that if you add an item to a caldav calendar that is not visible, we will run into the same problems we had before, that users will wonder where their events they just created went. Then when the calendar is shown, they might wonder where all the events came from.

Please at least add an XXX or TODO comment for the above change. 

If you have a better solution, I'd be happy to review (although I have limited internet access at the moment).


I'll assume the caldav changes have their meanings, I'm not quite sure on this observer jungle :)

r=philipp with above comments
Attachment #308136 - Flags: review?(philipp) → review+
Patch (with added comments as requested) checked in on HEAD, MOZILLA_1_8_BRANCH, and SUNBIRD_0_8_BRANCH

I agree that this is not an ideal solution - though I think removing the observer jungle is a big plus 8-)  I'll file a follow-on bug about the adding-items-to-invisible-CalDAV-calendar issue, which can be relnoted for 0.8, since I doubt I'll find a better solution before release. We certainly need a better one for 0.9.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: blocking-calendar0.8?
I checked this issue with the latest lightning build 2008031118 and sunbird 20080311 but it seems for me that this bug isn't fixed.

My STR:

- create a new profile
- create a calDAV calendar account http://rscds.zathras.lss.wisc.edu/caldav.php/mozilla/home/  User/Password=mozilla/calendar
- create a event via d&d or new event dialog -> no event in the calendar view
- refresh the view twice -> event is visible 
Status: RESOLVED → REOPENED
Flags: blocking-calendar0.8?
Resolution: FIXED → ---
needs investigation
Flags: blocking-calendar0.8? → blocking-calendar0.8+
I see this happening on that Zathras server but not on other servers, so this may be peculiar to that older RSCDS install on a slow&overloaded machine. Even there, I see it need one refresh, not two. Will investigate.
Flags: blocking-calendar0.8+ → blocking-calendar0.8?
Whups, mid-air collision there that bugzilla didn't catch; I did not mean to roll back Daniel's grant of blocking-0.8. Resetting. 

As long as I am commenting, I see RSCDS returning a 422 after the PUT, so I strongly suspect that this is just an older RSCDS that does not support the query-by-uid that we do after a PUT. Unfortunately it will most likely be tomorrow before I can look into this further.
Flags: blocking-calendar0.8? → blocking-calendar0.8+
I checked this issue with the latest lightning build 2008031120
against RSCDS 0.9.1 and I can not reproduce this on an already present profile
- create a event via d&d or new event dialog -> event is visible
Hope it's help
I rechecked with the current nightly (12-Mar-2008) and a Scalix Caldav server:

New events are not displayed. 
I now have to reload the calendar _once_ to have the new event displayed, not twice as before (bug 421478).



 
(In reply to comment #26)
> I rechecked with the current nightly (12-Mar-2008) and a Scalix Caldav server:
> 
> New events are not displayed. 
> I now have to reload the calendar _once_ to have the new event displayed, not
> twice as before (bug 421478).
> 
This sounds like bug 418050, and thus a Scalix problem, to me. 

I should be able to upgrade that zathras test server to current DAViCal later today to confirm my suspicions.
I updated that zathras machine to DAViCal 0.9.4 and no longer see the must-refresh-once behavior, so I'm going to go ahead and re-close this bug. Thanks to Andreas for using zathras to test CalDAV, and now that I know someone is doing so I'll be more careful about keeping it current with DAViCal.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
I do some additional testing:

cosmo 0.6.0.1 server (http):
- create an event, wait ~10 secs (without manual reload) -> event gets visible

cosmo 0.6.0.1 server (https):
- create an event, wait ~4 secs (without manual reload) -> event gets visible

zathra RSCDS 0.80:
-create an event, wait *3 secs (without manual reload) -> event gets visible
Target Milestone: --- → 0.8
You need to log in before you can comment on or make changes to this bug.