386502 - dtstart is always included in recurrence set, even when it's out of the query range

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

When using calIRecurrenceInfo.getOccurrences(), the dtstart of the event is always included in the set, no matter if it is inside or outside the daterange that was passed in.
This is a due to the fix in bug 381415

Not sure what the real-world implication of this is. At least the storage provider returns incorrect items, but i think the views filters them out. But anyway, it's not correct.

Comment 1

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

Attachment: naive patch

This patch is mainly for my own reference. I think that it's incomplete, because it ignores event that start before the range of the query, and end inside the query (for example). Compare to the checks in the foreach below.

Comment 2

[Stefan Sitter [:ssitter]]

(In reply to comment #0)
> Not sure what the real-world implication of this is.

Bug 386559 and Bug 386563 seem to be occurrences of the issue described in this bug. Adding dependency so that the bugs can be retested once this issue resolved.

Comment 3

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

I won't get around to this any time soon. Feel free to take it.

Comment 4

[Christoph Kaulich]

Hello,

I have seen Bug 386563 in my installation here, and it got fixed by the patch which is added in comment #1.

So maybe someone else can test ist also?

Comment 5

[Philipp Kewisch [:Fallen][☁️📆]]

Please leave granting blocking flags to the release drivers.

Comment 6

[Sebastian Schwieger]

(In reply to comment #1)

> This patch is mainly for my own reference. I think that it's incomplete,
> because it ignores event that start before the range of the query, and end
> inside the query (for example). Compare to the checks in the foreach below.
> 
Wouldn't comparing to the modified start range (aka searchStart) solve this problem?

Comment 7

[Sebastian Schwieger]

Attachment: use shifted start range

Thats what I had in mind. It seams to fix the bug how I saw it (displaying all recurring birthday entries in agenda and considering all alarm entries thereof).

Comment 8

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

Comment on attachment 271851 [details] [diff] [review]
use shifted start range

The code inside the foreach block (a few lines later) has three checks: the start is in the range, the end is in the range, or the event starts before the range and ends after it.
The code in the patch only has the first check. I think that's not quite right, and that we need all three checks. There might be an event that starts before the range, but extends into the range. That event should be included too I think (we do the same for all the recurrences, so why not for the 'original' item?)

(I actually think that all three checks can be condensed into one: the event starts before the end of the range, and ends after the start of the range. I think we do that in other places too. But I'd like somebody to check my logic before applying it)

Comment 9

[Sebastian Schwieger]

(In reply to comment #8)
> (From update of attachment 271851 [details] [diff] [review])
> The code inside the foreach block (a few lines later) has three checks: the
> start is in the range,

That was included in your patch and still is in mine

> the end is in the range, 

By shifting the start of the range by the amount of duration before the original start, we catch those events.

> or the event starts before the range and ends after it.

same applies here. 

This all depends on us knowing the duration of the event. If this is always the case, I don't know.

Comment 10

[Daniel Boelzle [:dbo]]

Attachment: factoring out range check

Good catch; IMO mvl is right, we need to check like we check exceptional items.
This patch factors out the range check. Sebo, could you please verify the patch?

Comment 11

[Sebastian Schwieger]

Yes, it seems to fix the bug and also bug 386559. I still think it is more checking than actually needed. Since I have started to deal with this bug, I would appreciate if someone could shed some light on me, why my approach should not catch all mentioned cases.

Also mvl's idea to only check if "start" is before "endRange" and "end" after "startRange" sounds reasonable and more elegant than the current implementation to me.

Comment 12

[Daniel Boelzle [:dbo]]

(In reply to comment #11)
I think mvl's optimization is correct, too, except for the special case of todos without DUE: those need a check whether their DTSTART falls into the range.
Sebo's patch looks correct to me, too, although I'd favor a general checkRange() function being used for both the baseItem and the exception set. I think that makes reading the code easier.
Sebo: Do you volunteer to further optimize checkRange()?

Comment 13

[Sebastian Schwieger]

Attachment: modification of Daniels patch

Ok, this is the modification of Daniels patch, that uses searchStart. There is one thing, that might make this approach problematic: If exceptions can have different durations than the parent item, then this is not taken into account. Daniel, is this a possible use-case? In this case the searchStart definition must be put into checkRange() and then the whole patch is probably not an optimization any more.

Let me state that I don't care what patch is going in. My main concern was to know if the other approaches are also possible in theory, since I didn't see why they shouldn't. If you wish, I can also prepare a patch that applies mvl's approach. Let me know.

Comment 14

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

If there are any exceptions, those can have different durations, yes. But that's not a problem for this bug, because it is only about the 'base' item, which is not an exception.
So I think that the searchStart approach works in the form of the first patch, which only applied to the base item, but does not work in the last form, because there it also touches recurrences and exceptions.

Comment 15

[Sebastian Schwieger]

Attachment: second modification

This is a modification of Daniels patch that uses mvl's approach (see comment 8).

Comment 16

[Daniel Boelzle [:dbo]]

Comment on attachment 273334 [details] [diff] [review]
second modification

>+            var dtstart = item.getProperty("DTSTART");
>+            //tasks may have a due date set or no duration at all
>+            var end = item.getProperty("DTEND") || item.getProperty("DUE") || item.getProperty("DTSTART");
except for this minor opt:
  var end = item.getProperty("DTEND") || item.getProperty("DUE") || dtstart;

the patch looks good; thanks sebo!

Comment 17

[Sebastian Schwieger]

Attachment: second modification v2

Here is the second version with minor opt as requested. Nothing else changed.

Comment 18

[Daniel Boelzle [:dbo]]

Comment on attachment 273410 [details] [diff] [review]
second modification v2

r=dbo

Comment 19

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH.