Skip to content

Conversation

@rotdrop
Copy link

@rotdrop rotdrop commented Jun 20, 2025

This PR address #347 and its duplicate #700. The commit also adds a test. The idea is to maintain an array of iterators, compare their current values and pick the smallest one in next(). Then all iterators matching the timestamp value of the chosen DateTime are advanced, which the others which may be invalid or have their individual current values still ahead of time are kept at their position.

I have added a test (with one RRULE and RDATES which are placed "out of order").

rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Jun 20, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Jun 20, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Jun 20, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Jun 20, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Jun 20, 2025
@rotdrop
Copy link
Author

rotdrop commented Jun 20, 2025

Actually, the changes to the RDateIterator.php would not be needed; I started with the PHP AppendIterator, but as this was not enough: duplicated event dates are handled by the EventIterator anyway, which now behaves like an "OrderedUniqueAppendIterator".

@rotdrop
Copy link
Author

rotdrop commented Jun 20, 2025

Actually, the changes to the RDateIterator.php would not be needed; I started with the PHP AppendIterator, but as this was not enough: duplicated event dates are handled by the EventIterator anyway, which now behaves like an "OrderedUniqueAppendIterator".

Nope, it ain't that easy. However, this is also due to the current code base. If the current event DTSTART refers to a date after the dates defined by an RDATE rule, then the ordering of the values returned by the current EventIterator is garbled: it will always return the DTSTART value of the master event as first item which is wrong. RDATE exception may very well refer to events which are in the past w.r.t. DTSTART of the master event.

This also means that further changes would be needed. The RDate iterator has to take care of this.

RDATE is an execption. We do not have control over the point in time
where the exception is scheduled and hence should not assume that is is
always after DTSTART.
rotdrop added a commit to rotdrop/sabre-vobject that referenced this pull request Jun 21, 2025
rotdrop added a commit to rotdrop/sabre-vobject that referenced this pull request Jun 21, 2025
…TART is the earliest date.

It should also not assume that the list of dates is sorted at all.
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Jun 21, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Aug 19, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Sep 5, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Sep 6, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Sep 10, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Sep 13, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Sep 17, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Oct 5, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Nov 2, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Nov 13, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Nov 19, 2025
rotdrop added a commit to rotdrop/nextcloud-3rdparty that referenced this pull request Nov 25, 2025
Comment on lines +177 to +182
foreach ($this->masterEvent->RDATE as $rDate) {
$this->recurIterators[] = new RDateIterator(
$rDate->getParts(),
$this->startDate,
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of creating an array of iterators would it be easier to combine all the rdate instances in to a single iterator?

$rdateValues = [];
foreach ($this->masterEvent->RDATE as $rdate) {
    $rdateValues = array_merge($rdateValues, $rdate->getParts());
}
$this->recurIterator = new RDateIterator(
    $this->masterEvent->RDATE->getParts(),
    $rdateValues,
    $this->startDate
);

This would reduce the complexity of iterating through RRULE and RDATE significantly, you would only need to rewind, forward, and check 2 iterators, instead of looping through an array of iterators.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not look into this for a long time now. I am no longer using upstream Sabre, but rather my own forks with my bug fixes, due to lack of responsiveness of the maintainers of Sabre. No flame intended, but we all have only a limited amount of time available, this applies as well to the maintainers of a software package as to potential contributors (which might be at the same time over-loaded maintainers of other software packages). I'll post a more useful response as soon as I find the time to look into your suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants