Closed Bug 364580 Opened 13 years ago Closed 13 years ago

asynchronous provider -> fast traveling freezes the weekview

Categories

(Calendar :: Calendar Views, defect)

Lightning 0.3
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: andreas.treumann, Assigned: michael.buettner)

References

Details

Attachments

(1 file, 1 obsolete file)

REPRODUCTION:
=============

- switch to weekview
- use the arrow button for fast forward/reward traveling
- after some fast mouse clicks the calendar view hangs


RESULT:
=======

- after some fast mouse clicks the calendar view hangs
- selecting a day in the calender view unfreezes the view  

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

- no freeze 

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

- always
Andreas: Oh, I forgot that this happens only with asynchron calendars, like WCAP, sorry.
Summary: fast traveling freezes the weekview → asynchron provider -> fast traveling freezes the weekview
js console outputs:

Error: this.selectedDay has no properties
Source File: chrome://calendar/content/calendar-decorated-week-view.xml
Line: 183
Summary: asynchron provider -> fast traveling freezes the weekview → asynchronous provider -> fast traveling freezes the weekview
Duplicate of this bug: 365646
notas@gmx.net also discovered this problem when using only CalDav calendars too, which was reported in bug 365646
taking this one.
Assignee: nobody → michael.buettner
unfortunately this is a regression from bug #329035. traveling through different weeks results in calls to calendar-multiday-view::setDateRange() which calls calendar-multiday-view::refresh() which in turn rebuilds the internal list of dates that are requested to be displayed. bug #329035 introduced that rebuilding is delayed until any previously scheduled requests have been finished. thus findColumnForDate() can't find the requested date under these circumstances which leads to the above described issue.
Attached patch patch v1 (obsolete) — Splinter Review
This patch resolves the problem by calling relayout() whether or not there are pending refresh-requests instead of waiting for previous requests to finish. I also added the missing call to the proper location in calendar-month-view since it suffers from the same problem, even if it did not cause any trouble so far.
Attachment #250845 - Flags: first-review?(lilmatt)
Comment on attachment 250845 [details] [diff] [review]
patch v1

Adding ui-review request to make sure this is indeed the behaviour we want in this situation.
Attachment #250845 - Flags: ui-review?(mvl)
Why not just clear the mRefreshPending flag when setDateRange is called?  That seems to me like it would solve this problem and avoid the big performance hit with synchronous providers that this patch would require.
(In reply to comment #9)
> Why not just clear the mRefreshPending flag when setDateRange is called?  That
> seems to me like it would solve this problem and avoid the big performance hit
> with synchronous providers that this patch would require.
This would re-introduce bug #329035 as it would completely circumvent the request queue. But you're right that the proposed patch involves a performance hit for synchronous providers. It would be better to call relayout() only if there are request jobs in the queue, something like this:

            if(this.mRefreshQueue.length > 0) {
              this.relayout();
            }

This would update the mDateColumns-array before any request to send down to the providers and ignore calling relayout in case the request queue has been finished. Any comments on this?
I'm quite confused here. It sound to me that the problem is that data for old date ranges is coming in after the daterange has been changed. I would that that this data should just be ignored. I don't see the need to force it to redraw.
(In reply to comment #11)
> I'm quite confused here. It sound to me that the problem is that data for old
> date ranges is coming in after the daterange has been changed. I would that
> that this data should just be ignored. I don't see the need to force it to
> redraw.
Obviously I didn't make this clear enough ;-) The problem is not the old data is coming in after the daterange has been updated, but that the daterange hasn't been updated but new data is coming in expecting the daterange to be up to date. What actually went wrong is that the call to relayout() has been defered due to the fact that we need to take asynchronous providers into account.
Attached patch patch v2Splinter Review
addresses previous comments.
Attachment #250845 - Attachment is obsolete: true
Attachment #251929 - Flags: ui-review?(mvl)
Attachment #251929 - Flags: second-review?(jminta)
Attachment #251929 - Flags: first-review?(lilmatt)
Attachment #250845 - Flags: ui-review?(mvl)
Attachment #250845 - Flags: first-review?(lilmatt)
Comment on attachment 251929 [details] [diff] [review]
patch v2

What are the implications of this patch?
It's quite hard to tell from the code what this patch will do to both the sync case and the async case, from a viewpoint of the user.
Comment on attachment 251929 [details] [diff] [review]
patch v2

I'm letting jminta and mvl figure out the "if we want to take this patch part".

Only nit: add a space between the if and the (

r=lilmatt
</buck passed>
Attachment #251929 - Flags: first-review?(lilmatt) → first-review+
I really can't oversee the implications of this patch. So, i repeat my questions:
What does this do for the users of an sync calendar? Will it speed up the views or will it slow them down? And how about async calendars?
(In reply to comment #16)
> What does this do for the users of an sync calendar? Will it speed up the views
> or will it slow them down? And how about async calendars?
Obviously I didn't do a good job making myself clear. To answer this specific question, this patch doesn't affect performance at all. Bug #329035 and this one take care about the issue that methods get called in the right order, even in case asynchronous providers take part in the game.

I'm going to explain what this is all about, but i'll restrict this discussion to the multiday view since all of this applies to the other views as well.

Views get refreshed by calling refresh(). this happens under various circumstances, e.g. moveView() -> goToDay() -> setDateRange() -> refresh().

refresh() calls relayout() which in turn updates the internal components of the view. an important role plays the mDateColumns array which contains the specific dates that the view displays. after calling relayout() getItems() on the composite calendar gets called, thus each calendar receives a getItems() call. thus for each calendar mOperationListener.onGetResult() gets called plus mOperationListener.onOperationComplete() after the operation has been completed.

all of this works great until asynchronous providers enter the game. in this case onGetResult() could be called with an unspecified delay. it could well be possible that refresh() gets called again before past getItems() calls are finished and are notified through onGetResult(). to prevent this issue i introduced a queue in bug #329035. in case previous requests didn't finished before relayout() gets called again the request is put into a queue. in case the previous request finishes the next job is fetched from the queue and executed.

with this queue in place the re-entrancy problems are solved. unfortunately one problem remains in case relayout() gets called before the queue is empty (previous requests have been finished) - the mDateColumns array is not up to date. and this exactly what this patch is about to address. in case a request is pending (result has not been received) and there's at least another job waiting we're calling relayout(). this ensures that the mDateColumns array is up to date. this scenario happens immediately in case we're traveling from week to week with an asynchronous provider in place. in case the user switches to another week before the current request has been finished we're calling relayout() in order to update the mDateColumns to reflect the dates of the week we're switching to. the previously submitted request (which we didn't receive the result for) will eventually be received later but won't do any harm since those items won't find the date columns (since the mDateColumns array already reflects the new dates).

the conclusion of what i tried to explain is that with synchronous providers we don't see any difference whatsoever. with asynchronous providers we're just calling the methods delayed, no performance penalty is involved.
Comment on attachment 251929 [details] [diff] [review]
patch v2

Thanks for your explenation. Patch looks good.
ui-review+r2=mvl

One thing i do wonder is if instead of a queue, we need to cancel previous requests (or just ignore the results) if the user has already switched to a different timerange. But that's for a future bug.
Attachment #251929 - Flags: ui-review?(mvl)
Attachment #251929 - Flags: ui-review+
Attachment #251929 - Flags: second-review?(jminta)
Attachment #251929 - Flags: second-review+
Checked in on trunk and MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.