Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

recur_expansion.js: fix iterator with RDATE-only recurrence #534

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dilyanpalauzov
Copy link
Contributor

When iterating with

  e is an event-component
  if (e.isRecurring() {
     let i = e.iterator();
     while (n = i.next()) {
       o = g.getOccurrenceDetails(obj)
       
     }
  }

on the first iteration the DTSTART instance shall be returned. It is returned, when there is RRULE. In the lack of this change, on the first iteration, when RDATE is present without RRULE, the first RDATE instance is returned.

@darktrojan how does Thunderbird handle VEVENTS with RDATE but without RRULE. These are presented correct in the UI. But the instances cannot be obtained by getting an iteratior of the event and then calling .next(). The reason is, that the very first iteration skips then the DTSTART-instance. The DTSTART instance is not skipped by the iterator.next() when the VEVENT has RRULE.

dilyanpalauzov added a commit to dilyanpalauzov/ical.js that referenced this pull request Sep 16, 2022
kewisch#534

When iterating with
  e is an event-component
  if (e.isRecurring() {
     let i = e.iterator();
     while (n = i.next()) {
       o = g.getOccurrenceDetails(obj)
       …
     }
  }

on the first iteration the DTSTART instance shall be returned.  It is returned,
when there is RRULE.  In the lack of this change, on the first iteration, when
RDATE is present without RRULE, the first RDATE instance is returned.
dilyanpalauzov added a commit to dilyanpalauzov/ical.js that referenced this pull request Sep 17, 2022
kewisch#534

When iterating with
  e is an event-component
  if (e.isRecurring() {
     let i = e.iterator();
     while (n = i.next()) {
       o = g.getOccurrenceDetails(obj)
       …
     }
  }

on the first iteration the DTSTART instance shall be returned.  It is returned,
when there is RRULE.  In the lack of this change, on the first iteration, when
RDATE is present without RRULE, the first RDATE instance is returned.
dilyanpalauzov added a commit to dilyanpalauzov/ical.js that referenced this pull request Sep 18, 2022
kewisch#534

When iterating with
  e is an event-component
  if (e.isRecurring() {
     let i = e.iterator();
     while (n = i.next()) {
       o = g.getOccurrenceDetails(obj)
       …
     }
  }

on the first iteration the DTSTART instance shall be returned.  It is returned,
when there is RRULE.  In the lack of this change, on the first iteration, when
RDATE is present without RRULE, the first RDATE instance is returned.
@coveralls
Copy link

coveralls commented Sep 18, 2022

Pull Request Test Coverage Report for Build 3076542534

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 95.473%

Totals Coverage Status
Change from base Build 3012316465: 0.03%
Covered Lines: 2932
Relevant Lines: 3023

💛 - Coveralls

When iterating with
  e is an event-component
  if (e.isRecurring() {
     let i = e.iterator();
     while (n = i.next()) {
       o = g.getOccurrenceDetails(obj)
       …
     }
  }

on the first iteration the DTSTART instance shall be returned.  It is returned,
when there is RRULE.  In the lack of this change, on the first iteration, when
RDATE is present without RRULE, the first RDATE instance is returned.
dilyanpalauzov added a commit to dilyanpalauzov/ical.js that referenced this pull request Sep 18, 2022
kewisch#534

When iterating with
  e is an event-component
  if (e.isRecurring() {
     let i = e.iterator();
     while (n = i.next()) {
       o = g.getOccurrenceDetails(obj)
       …
     }
  }

on the first iteration the DTSTART instance shall be returned.  It is returned,
when there is RRULE.  In the lack of this change, on the first iteration, when
RDATE is present without RRULE, the first RDATE instance is returned.
@darktrojan
Copy link
Collaborator

@darktrojan how does Thunderbird handle VEVENTS with RDATE but without RRULE. These are presented correct in the UI. But the instances cannot be obtained by getting an iteratior of the event and then calling .next(). The reason is, that the very first iteration skips then the DTSTART-instance. The DTSTART instance is not skipped by the iterator.next() when the VEVENT has RRULE.

Thunderbird doesn't use this code for RDATEs, it maintains a list of them and does iteration itself. So I'm not familiar with how this bit works at all.

Copy link
Collaborator

@darktrojan darktrojan left a comment

Choose a reason for hiding this comment

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

In my very limited understanding of how this particular bit works, this looks okay to me. But it needs updating for ES6.

dilyanpalauzov added a commit to dilyanpalauzov/ical.js that referenced this pull request Nov 29, 2022
kewisch#534

When iterating with
  e is an event-component
  if (e.isRecurring() {
     let i = e.iterator();
     while (n = i.next()) {
       o = g.getOccurrenceDetails(obj)
       …
     }
  }

on the first iteration the DTSTART instance shall be returned.  It is returned,
when there is RRULE.  In the lack of this change, on the first iteration, when
RDATE is present without RRULE, the first RDATE instance is returned.
dilyanpalauzov added a commit to dilyanpalauzov/ical.js that referenced this pull request Nov 29, 2022
kewisch#534

When iterating with
  e is an event-component
  if (e.isRecurring() {
     let i = e.iterator();
     while (n = i.next()) {
       o = g.getOccurrenceDetails(obj)
       …
     }
  }

on the first iteration the DTSTART instance shall be returned.  It is returned,
when there is RRULE.  In the lack of this change, on the first iteration, when
RDATE is present without RRULE, the first RDATE instance is returned.
@titanism titanism mentioned this pull request Feb 16, 2024
dilyanpalauzov added a commit to dilyanpalauzov/ical.js that referenced this pull request Apr 13, 2024
kewisch#534

When iterating with
  e is an event-component
  if (e.isRecurring() {
     let i = e.iterator();
     while (n = i.next()) {
       o = g.getOccurrenceDetails(obj)
       …
     }
  }

on the first iteration the DTSTART instance shall be returned.  It is returned,
when there is RRULE.  In the lack of this change, on the first iteration, when
RDATE is present without RRULE, the first RDATE instance is returned.
@kewisch
Copy link
Owner

kewisch commented Apr 13, 2024

@dilyanpalauzov looks like you are actively working on this one, anything I can do to help get this merged?

dilyanpalauzov added a commit to dilyanpalauzov/ical.js that referenced this pull request Apr 14, 2024
kewisch#534

When iterating with
  e is an event-component
  if (e.isRecurring() {
     let i = e.iterator();
     while (n = i.next()) {
       o = g.getOccurrenceDetails(obj)
       …
     }
  }

on the first iteration the DTSTART instance shall be returned.  It is returned,
when there is RRULE.  In the lack of this change, on the first iteration, when
RDATE is present without RRULE, the first RDATE instance is returned.
@dilyanpalauzov
Copy link
Contributor Author

This change for proposal is one and a half years old. I do not know what is missing to get it merged. I see now it contains a test, which is failing. Sorry, I do not know anymore what was going on.

@kewisch
Copy link
Owner

kewisch commented Apr 14, 2024

Ah no problem, I had seen a recently referenced commit which made me assume. If you do feel like picking this up again I'm happy to give reviews some priority.

@dilyanpalauzov dilyanpalauzov mentioned this pull request Apr 21, 2024
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.

4 participants