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

Use (NS)DateComponents instead of (NS)Date for DATE-TIME and DATE values #47

Open
tikitu opened this issue Mar 28, 2017 · 5 comments
Open

Comments

@tikitu
Copy link

tikitu commented Mar 28, 2017

Hi folks,

I'm wondering why XbICalendar uses NSDate to represent DATE-TIME and DATE properties, and whether NSDateComponents might be a better choice. In particular:

  • An NSDateComponents instance with hours/minutes/seconds unset is an apt representation for a "calendar date" (DATE).
  • An NSDateComponents instance with an explicit timezone is a better representation for a DATE-TIME, since the timezone component can be important for date-time arithmetic.

I'm not aware of any issues caused by the first point, it just "fits". The second, more important, point is what got me started on this. I'm working on a bug over in my fork which makes RRULE properties misbehave across a DST boundary: https://github.com/minddistrict/XbICalendar/tree/preserve-timezone-strings

The root cause is that when an DTSTART property makes a round-trip from icalproperty to XbICProperty and back to icalproperty it loses the original timezone, because XbICProperty represents it as NSDate which forgets its timezone information. (This makes sense. An NSDate represents an instant in time: two NSDate objects created from different timezones but identifying the same instant should compare equal, which implies forgetting the originating timezone.)

So I'm thinking that the "right" fix for that bug would be to convert to using NSDateComponents for DTSTART, which implies using them for all DATE-TIME properties, and then I might as well go ahead and convert DATE properties too.

My question for y'all is: am I missing some design considerations that make this a terrible idea? And if not, how do you rate the chances of landing a PR with these changes? It's a change to the external API of the library, so needs some release management.

@tikitu
Copy link
Author

tikitu commented Mar 28, 2017

Welp, the answer might just be "yes that's a terrible idea". At least, I think it will not solve the original problem: that round-tripping a DATE-TIME property through XbICalendar loses timezone information. The new wrinkle I've dug up is: NSTimeZone itself isn't a tight match for icaltimezone, with conversions in both directions potentially going wrong. (An icaltimezone might use a timezone name that's defined locally in the calendar file, which NSTimeZone won't recognise. In that case we can't make a corresponding NSTimeZone object without translating all the internal details of the icaltimezone, which doesn't sound fun. I think something similar can happen going the other direction; I'm also having trouble loading builtin (Olson) timezones in libical but that might just be a setup mistake on my part.)

Since NSDateComponents uses NSTimeZone for its timezone info, that means a round-trip is not guaranteed.

In fact the only way I see to guarantee the roundtrip is to keep the TZID string around somewhere. Which means a custom type, and I start wondering if it's all worth my while (compared to a more direct Swift wrapper around the types-and-operations defined in libical).

@ahalls
Copy link
Contributor

ahalls commented Mar 28, 2017

He tikitu ... I haven't worked on the project that used this over a year now. I'm wondering if you would like to take over management of this project ?

@tikitu
Copy link
Author

tikitu commented Mar 28, 2017

@ahalls if you'd asked me this morning I would have said "yes", but since my last comment I'm starting to think I'm better off sidestepping the ObjC layer entirely in my project. In which case I'd better not take this one on as well, sorry.

@ahalls
Copy link
Contributor

ahalls commented Mar 28, 2017 via email

@tikitu
Copy link
Author

tikitu commented Mar 28, 2017

Sure thing! On balance I don't think the NSDateComponents change will help (because it doesn't solve the underlying problem) but I hope landing at least the framework-building PR #46 will be useful.

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

No branches or pull requests

2 participants