Closed
Bug 420749
Opened 16 years ago
Closed 16 years ago
CalDAV calendars must refresh twice to display server-side changes
Categories
(Calendar :: Provider: CalDAV, defect)
Calendar
Provider: CalDAV
Tracking
(Not tracked)
RESOLVED
FIXED
0.8
People
(Reporter: browning, Unassigned)
References
Details
Attachments
(1 file, 2 obsolete files)
6.45 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•16 years ago
|
||
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?
Reporter | ||
Comment 2•16 years ago
|
||
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.
Reporter | ||
Comment 3•16 years ago
|
||
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)
Reporter | ||
Comment 4•16 years ago
|
||
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)
Reporter | ||
Comment 5•16 years ago
|
||
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?
Comment 6•16 years ago
|
||
(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.
Comment 7•16 years ago
|
||
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
Comment 8•16 years ago
|
||
I'm with Maxime unless of corse there's a proper fix
Reporter | ||
Comment 9•16 years ago
|
||
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.
Reporter | ||
Comment 11•16 years ago
|
||
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.
Reporter | ||
Comment 12•16 years ago
|
||
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.
Comment 13•16 years ago
|
||
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.
Reporter | ||
Comment 14•16 years ago
|
||
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)
Comment 15•16 years ago
|
||
> 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.
Reporter | ||
Comment 16•16 years ago
|
||
(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.
Comment 17•16 years ago
|
||
(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.
Reporter | ||
Comment 18•16 years ago
|
||
(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 19•16 years ago
|
||
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+
Reporter | ||
Comment 20•16 years ago
|
||
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: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: blocking-calendar0.8?
Comment 21•16 years ago
|
||
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 → ---
Reporter | ||
Comment 23•16 years ago
|
||
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?
Reporter | ||
Comment 24•16 years ago
|
||
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+
Comment 25•16 years ago
|
||
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
Comment 26•16 years ago
|
||
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).
Reporter | ||
Comment 27•16 years ago
|
||
(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.
Reporter | ||
Comment 28•16 years ago
|
||
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: 16 years ago → 16 years ago
Resolution: --- → FIXED
Comment 29•16 years ago
|
||
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
Updated•16 years ago
|
Target Milestone: --- → 0.8
You need to log in
before you can comment on or make changes to this bug.
Description
•