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

fixed calendar, issue 1696, ics file start date is not date type #1699

Merged
merged 4 commits into from
Jun 13, 2019

Conversation

Hg347
Copy link

@Hg347 Hg347 commented Jun 11, 2019

Fixes issue #1696
Symptom: calendar does not sync. But has not relation to authentication. The start date was simply not a date so toISOString() failed.
Same fix applied to exdates[i]

Tested with this calendar:
personal.zip

On the other hand might be that the code above, L33-38, failed, unfortunately I can't debug yet:

    if (curr.start.length === 8) {
        var comps = /^(\d{4})(\d{2})(\d{2})$/.exec(curr.start);
        if (comps) {
         curr.start = new Date (comps[1], comps[2] - 1, comps[3]);
        }
      }

@MichMich
Copy link
Collaborator

Thanks.
Unfortunately the build has failed due to lifting errors of the changelog. Are you able to fix this?

@Hg347
Copy link
Author

Hg347 commented Jun 13, 2019

Sure. I didn't notice, that you even have an integration build set up. That's great.

@MichMich MichMich merged commit cf2723a into MagicMirrorOrg:develop Jun 13, 2019
@MichMich
Copy link
Collaborator

Thanks!

@mattdb
Copy link
Contributor

mattdb commented Jun 13, 2019

My PR #1701 (that I was writing just as this was merged) also updates node-ical.js, but uses the "official version" which I believe offers equivalent levels of error handling, but also fixes several other calendar-related issues.

I resolved the conflict on my PR by simply using peterbraden's updated version, but want to make sure that I'm not missing anything or stepping on any toes. Thanks!

@Hg347
Copy link
Author

Hg347 commented Jun 13, 2019

My PR #1701 (that I was writing just as this was merged) also updates node-ical.js, but uses the "official version" which I believe offers equivalent levels of error handling, but also fixes several other calendar-related issues.

I resolved the conflict on my PR by simply using peterbraden's updated version, but want to make sure that I'm not missing anything or stepping on any toes. Thanks!

Indeed, looks much better. I just wonder what happened to curr.exdates, isn't that previous logic required anymore?

@mattdb
Copy link
Contributor

mattdb commented Jun 14, 2019

My read on why the exdates logic is no longer needed is this (but as with all things, especially date-related things, I heartily endorse a decent probability I'm off-base):

These objectHandler overrides at the end of node-ical.js are cleaning up / smoothing over some rough edges in ical inputs, specifically removing any milliseconds (. followed by 3 digits) added on to dates in DTSTART and EXDATE fields.

(Potential sidebar): Because the DTSTART and EXDATE dates are used as indexes in the resultant parsed ical object, being overly precise isn't particularly helpful and potentially a source of strange bugs. And I am fairly certain there isn't a use case for events repeating more than once a second.

As to why EXDATEs no longer need to be cleaned up: There was a recent-ish change in ical.js that stores EXDATE date keys as just the date part of the event, all hour/minute/second delineations are dropped. This means events that repeat more than once a day can't both have exceptions (and/or exceptions might get applied to the wrong one of the repeating events on that EXDATE??), but I don't think that events repeating more than once a day is at all common, as far as I know (at least for me, neither Fantastical nor Apple's calendar would allow a frequency smaller than daily). So, since all time information is dropped anyways, no need to partially clean it up beforehand.

@Hg347 Hg347 deleted the calendar-1696 branch June 15, 2019 09:09
@Hg347
Copy link
Author

Hg347 commented Jun 15, 2019

@mattdb I just checked the ical project and it looks not that active. There are open pull requests of 2013! I'm not that sure if it's a good idea to rely on the "official repo". The vows unit tests of master don't work either: "callback not fired".

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.

3 participants