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.

Sign up for GitHub

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

Jump to bottom

Make occurrences in chronological order even if BYMONTHDAY or BYYEARDAY is not in order #666

Merged
merged 2 commits into from
Apr 18, 2024
Merged

Make occurrences in chronological order even if BYMONTHDAY or BYYEARDAY is not in order #666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
427f3e4
Make occurrences in chronological order even if BYMONTHDAY or BYYEARD…
darktrojan Apr 15, 2024
e54e391
Added some more tests for things that could go wrong here
darktrojan Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 111 additions & 80 deletions lib/ical/recur_iterator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ class RecurIterator {
this.last.second = this.setup_defaults("BYSECOND", "SECONDLY", this.dtstart.second);
this.last.minute = this.setup_defaults("BYMINUTE", "MINUTELY", this.dtstart.minute);
this.last.hour = this.setup_defaults("BYHOUR", "HOURLY", this.dtstart.hour);
let dayOffset = this.last.day = this.setup_defaults("BYMONTHDAY", "DAILY", this.dtstart.day);
this.last.day = this.setup_defaults("BYMONTHDAY", "DAILY", this.dtstart.day);
this.last.month = this.setup_defaults("BYMONTH", "MONTHLY", this.dtstart.month);

if (this.rule.freq == "WEEKLY") {
Expand Down Expand Up @@ -262,77 +262,79 @@ class RecurIterator {
this._nextByYearDay();
}

if (this.rule.freq == "MONTHLY" && this.has_by_data("BYDAY")) {
let tempLast = null;
let initLast = this.last.clone();
let daysInMonth = Time.daysInMonth(this.last.month, this.last.year);

// Check every weekday in BYDAY with relative dow and pos.
for (let bydow of this.by_data.BYDAY) {
this.last = initLast.clone();
let [pos, dow] = this.ruleDayOfWeek(bydow);
let dayOfMonth = this.last.nthWeekDay(dow, pos);

// If |pos| >= 6, the byday is invalid for a monthly rule.
if (pos >= 6 || pos <= -6) {
throw new Error("Malformed values in BYDAY part");
}
if (this.rule.freq == "MONTHLY") {
if (this.has_by_data("BYDAY")) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Github's UI has failed to figure out that all I've done to this block is indent it. I'm pretty sure the block below (else if (this.has_by_data("BYMONTHDAY")) should also only be used if the rule is monthly.

let tempLast = null;
let initLast = this.last.clone();
let daysInMonth = Time.daysInMonth(this.last.month, this.last.year);

// Check every weekday in BYDAY with relative dow and pos.
for (let bydow of this.by_data.BYDAY) {
this.last = initLast.clone();
let [pos, dow] = this.ruleDayOfWeek(bydow);
let dayOfMonth = this.last.nthWeekDay(dow, pos);

// If |pos| >= 6, the byday is invalid for a monthly rule.
if (pos >= 6 || pos <= -6) {
throw new Error("Malformed values in BYDAY part");
}

// If a Byday with pos=+/-5 is not in the current month it
// must be searched in the next months.
if (dayOfMonth > daysInMonth || dayOfMonth <= 0) {
// Skip if we have already found a "last" in this month.
if (tempLast && tempLast.month == initLast.month) {
continue;
// If a Byday with pos=+/-5 is not in the current month it
// must be searched in the next months.
if (dayOfMonth > daysInMonth || dayOfMonth <= 0) {
// Skip if we have already found a "last" in this month.
if (tempLast && tempLast.month == initLast.month) {
continue;
}
while (dayOfMonth > daysInMonth || dayOfMonth <= 0) {
this.increment_month();
daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
dayOfMonth = this.last.nthWeekDay(dow, pos);
}
}
while (dayOfMonth > daysInMonth || dayOfMonth <= 0) {
this.increment_month();
daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
dayOfMonth = this.last.nthWeekDay(dow, pos);

this.last.day = dayOfMonth;
if (!tempLast || this.last.compare(tempLast) < 0) {
tempLast = this.last.clone();
}
}

this.last.day = dayOfMonth;
if (!tempLast || this.last.compare(tempLast) < 0) {
tempLast = this.last.clone();
this.last = tempLast.clone();

//XXX: This feels like a hack, but we need to initialize
// the BYMONTHDAY case correctly and byDayAndMonthDay handles
// this case. It accepts a special flag which will avoid incrementing
// the initial value without the flag days that match the start time
// would be missed.
if (this.has_by_data('BYMONTHDAY')) {
this._byDayAndMonthDay(true);
}
}
this.last = tempLast.clone();

//XXX: This feels like a hack, but we need to initialize
// the BYMONTHDAY case correctly and byDayAndMonthDay handles
// this case. It accepts a special flag which will avoid incrementing
// the initial value without the flag days that match the start time
// would be missed.
if (this.has_by_data('BYMONTHDAY')) {
this._byDayAndMonthDay(true);
}

if (this.last.day > daysInMonth || this.last.day == 0) {
throw new Error("Malformed values in BYDAY part");
}
} else if (this.has_by_data("BYMONTHDAY")) {
// Attempting to access `this.last.day` will cause the date to be normalised.
// So it will never be a negative value or more than the number of days in the month.
// We keep the value in a separate variable instead.

// Change the day value so that normalisation won't change the month.
this.last.day = 1;
let daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
if (this.last.day > daysInMonth || this.last.day == 0) {
throw new Error("Malformed values in BYDAY part");
}
} else if (this.has_by_data("BYMONTHDAY")) {
// Change the day value so that normalisation won't change the month.
this.last.day = 1;

if (dayOffset < 0) {
// A negative value represents days before the end of the month.
this.last.day = daysInMonth + dayOffset + 1;
} else if (this.by_data.BYMONTHDAY[0] > daysInMonth) {
// There's no occurrence in this month, find the next valid month.
// The longest possible sequence of skipped months is February-April-June,
// so we might need to call next_month up to three times.
if (!this.next_month() && !this.next_month() && !this.next_month()) {
throw new Error("No possible occurrences");
// Get a sorted list of days in the starting month that match the rule.
let normalized = this.normalizeByMonthDayRules(
this.last.year,
this.last.month,
this.rule.parts.BYMONTHDAY
).filter(d => d >= this.last.day);

if (normalized.length) {
// There's at least one valid day, use it.
this.last.day = normalized[0];
this.by_data.BYMONTHDAY = normalized;
} else {
// There's no occurrence in this month, find the next valid month.
// The longest possible sequence of skipped months is February-April-June,
// so we might need to call next_month up to three times.
if (!this.next_month() && !this.next_month() && !this.next_month()) {
throw new Error("No possible occurrences");
}
}
Comment on lines +319 to 337
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've thrown out an earlier hack and just used the valid days in the starting month if there is at least one of them.

} else {
// Otherwise, reset the day.
this.last.day = dayOffset;
}
}
}
Expand Down Expand Up @@ -513,7 +515,10 @@ class RecurIterator {
let rule;

for (; ruleIdx < len; ruleIdx++) {
rule = rules[ruleIdx];
rule = parseInt(rules[ruleIdx], 10);
if (isNaN(rule)) {
throw new Error('Invalid BYMONTHDAY value');
}
Comment on lines +518 to +521
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found that if the rule is a string, weird things happen.


// if this rule falls outside of given
// month discard it.
Expand Down Expand Up @@ -747,6 +752,9 @@ class RecurIterator {
if (this.by_indices.BYMONTHDAY >= this.by_data.BYMONTHDAY.length) {
this.by_indices.BYMONTHDAY = 0;
this.increment_month();
if (this.by_indices.BYMONTHDAY >= this.by_data.BYMONTHDAY.length) {
return 0;
}
}

let daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
Expand All @@ -762,7 +770,6 @@ class RecurIterator {
} else {
this.last.day = day;
}

} else {
this.increment_month();
let daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
Expand Down Expand Up @@ -835,7 +842,6 @@ class RecurIterator {
}

next_year() {

if (this.next_hour() == 0) {
return 0;
}
Expand All @@ -844,6 +850,13 @@ class RecurIterator {
this.days_index = 0;
do {
this.increment_year(this.rule.interval);
if (this.has_by_data("BYMONTHDAY")) {
this.by_data.BYMONTHDAY = this.normalizeByMonthDayRules(
this.last.year,
this.last.month,
this.rule.parts.BYMONTHDAY
);
}
Comment on lines +853 to +859
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the important bit (and the same code just below) – go back to the rule as specified and recalculate the valid days for the new month. This deals with negative rules and puts them all in order.

this.expand_year_days(this.last.year);
} while (this.days.length == 0);
}
Expand All @@ -854,19 +867,19 @@ class RecurIterator {
}

_nextByYearDay() {
let doy = this.days[this.days_index];
let year = this.last.year;
if (doy < 1) {
// Time.fromDayOfYear(doy, year) indexes relative to the
// start of the given year. That is different from the
// semantics of BYYEARDAY where negative indexes are an
// offset from the end of the given year.
doy += 1;
year += 1;
}
let next = Time.fromDayOfYear(doy, year);
this.last.day = next.day;
this.last.month = next.month;
let doy = this.days[this.days_index];
let year = this.last.year;
if (doy < 1) {
// Time.fromDayOfYear(doy, year) indexes relative to the
// start of the given year. That is different from the
// semantics of BYYEARDAY where negative indexes are an
// offset from the end of the given year.
doy += 1;
year += 1;
}
let next = Time.fromDayOfYear(doy, year);
this.last.day = next.day;
this.last.month = next.month;
}

/**
Expand Down Expand Up @@ -953,9 +966,19 @@ class RecurIterator {
this.increment_year(years);
}
}

if (this.has_by_data("BYMONTHDAY")) {
this.by_data.BYMONTHDAY = this.normalizeByMonthDayRules(
this.last.year,
this.last.month,
this.rule.parts.BYMONTHDAY
);
}
}

increment_year(inc) {
// Don't jump into the next month if this.last is Feb 29.
this.last.day = 1;
this.last.year += inc;
}

Expand Down Expand Up @@ -1177,6 +1200,14 @@ class RecurIterator {
} else {
this.days = [];
}

let daysInYear = Time.isLeapYear(aYear) ? 366 : 365;
this.days.sort((a, b) => {
if (a < 0) a += daysInYear + 1;
if (b < 0) b += daysInYear + 1;
return a - b;
});
Comment on lines +1204 to +1209
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And this does the same for BYYEARDAY rules.


return 0;
}

Expand Down
Loading