378588 - Tasks-in-view is broken on CalDAV calendars

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Bruno Browning]

CalDAV queries can in general request information on only a single component type - thus VEVENTs *or* VTODOs but not both. As commented in the code, the CalDAV provider currently punts on this issue: when getItems() is called with an aFilter for both events and todos, it requests only events. This breaks (at least) tasks-in-view.

Comment 1

[Bruno Browning]

Attachment: issue multiple queries as needed

This patch causes getItems() to issue multiple CalDAV queries as needed, and ensures that reportListener.onOperationComplete is called only once as required by the IDL.

It also makes reportListener.onOperationComplete return a correct value to the listener, thus fixing bug 377489

Not requesting review at this time because of the 0.5 codefreeze. Will request review once the freeze has been lifted.

Comment 2

[Daniel Boelzle [:dbo]]

Patch looks good to me. Just a thought: what about reworking reportInternal() to get passed an array of query strings instead of a single one and issuing all those queries? That way we can get rid of the count parameter.

Comment 3

[Bruno Browning]

(In reply to comment #2)
> Patch looks good to me. Just a thought: what about reworking reportInternal()
> to get passed an array of query strings instead of a single one and issuing all
> those queries? That way we can get rid of the count parameter.
> 

I plan on doing something like that in another bug. getItems() needs to first fetch etags only, and then fetch (to reportInternal) calendar-data solely for new/changed items (currently the CalDAV provider refetches calendar-data on every view change etc). I have some draft code that does this, but doing it in a scalable way is going to want to use a CalDAV:multiget query, which is not yet supported by all the servers that I think we want to maintain interop with. I'll polish and submit that patch when it won't break e.g. RSCDS. 

Since you've already looked at the patch, and following a comment by mvl on irc yesterday, I'll go ahead and request review. I realize the patch can't go in until after the 0.5 release.

Comment 4

[Daniel Boelzle [:dbo]]

Comment on attachment 262631 [details] [diff] [review]
issue multiple queries as needed

r=dbo

BTW: I recognized two more minor things in the code:
- mPendingStartupRequests ought to be initialized in ctor
- uri setter ought to return uri

Would you mind to fix those two when checking in? IMO it hardly justifies a separate bug.

Comment 5

[Bruno Browning]

Attachment: additional minor changes, per reviewer

This patch adds the changes requested by reviewer in comment #4, and is the version that should be checked in, post-0.5

Comment 6

[Bruno Browning]

removing 'checkin needed' as Daniel and I are still discussing this patch.

Comment 7

[Bruno Browning]

Attachment: further minor changes, per daniel

This version adds some changes that Daniel suggested via email after CalConnect (I'm leaving other of those suggestions, regarding notification and locationpath handling, for other patches). The only changes since the last, r+ed, version of the patch are ones suggested by reviewer, I think this can go in without further review.

Comment 8

[Stefan Sitter [:ssitter]]

checkin-needed for attachment 270921 [details] [diff] [review] (TRUNK and MOZILLA_1_8_BRANCH)

Comment 9

[Daniel Boelzle [:dbo]]

Comment on attachment 270921 [details] [diff] [review]
further minor changes, per daniel

carrying r+

Comment 10

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH.