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

Calendar: repeating event rrule & exdate fixes #1701

Merged
merged 4 commits into from
Jun 14, 2019

Conversation

mattdb
Copy link
Contributor

@mattdb mattdb commented Jun 13, 2019

Previously, if a repeating event had specific recurrences deleted (exdate) or modified (rrule), the calendar would not display those changes, instead still showing canceled events, or events with their originally scheduled times or titles. This PR makes handling these types of exceptions to calendars much more robust.

In order to do this, I:

  • updated calendar's vendored ical.js
  • made extensive changes to the calendar fetcher within the logic branch where an event has an rrule attached
  • extracted title-parsing logic into a function (getTitleFromEvent) since modified events may need to re-parse their title.

I believe this will solve and/or make progress on a slew of calendar-related issues, including:

The updated ical.js should also help with comma-separated EXDATEs, as noted in peterbraden/ical.js#84 and peterbraden/ical.js#76

@mattdb
Copy link
Contributor Author

mattdb commented Jun 13, 2019

Doh! That conflict just got pulled in as I was writing up the PR. I'll work on resolving.

@mattdb
Copy link
Contributor Author

mattdb commented Jun 13, 2019

Investigated the conflict-- #1699 made manual changes to the vendored ical.js/node-ical.js file, but similar improvements were already included in the upstream file that I am updating here. In my opinion, best to stay with the "official" upstream version as long as it resolves the issue the "unofficial" modification was addressing.

rrule-alt has not been updated in some time, while the main rrule has. ical.js is using updated rrule. Getting rid of rrule-alt would affect some third-party modules, however, so we should keep it around for now at least.
@mattdb
Copy link
Contributor Author

mattdb commented Jun 13, 2019

Updating to the up-to-date ical.js does offer one complication: it uses rrule, rather than rrule-alt, which MagicMirror manually switched to a while back. Since that switch, rrule-alt has not been updated, but rrule has seen quite a bit of updating.

I propose we hop back on the rrule train to handle bug fixes like all of the myriad ical formatting quirks, etc.

However, we can't remove rrule-alt from the package's dependencies because some 3rd party modules have come to expect it (MMM-CalendarExt is the one I ran into, at least).

Also log error on node side as well.
@MichMich
Copy link
Collaborator

MichMich commented Jun 14, 2019

I propose we hop back on the rrule train to handle bug fixes like all of the myriad ical formatting quirks, etc.

Agree.

However, we can't remove rrule-alt from the package's dependencies because some 3rd party modules have come to expect it (MMM-CalendarExt is the one I ran into, at least).

What's you suggested solution for this?

@MichMich MichMich merged commit d41afa0 into MagicMirrorOrg:develop Jun 14, 2019
@MichMich
Copy link
Collaborator

I've merged your PR for now. We can always add extra PR's to solve open or created issues.

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