libical / XbICalendar

Objective-C Wrapper for the libical library
Other
17 stars 19 forks source link

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

Open tikitu opened 7 years ago

tikitu commented 7 years ago

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:

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

Thanks for your contribution … I hope you stay engaged long enough to help me merge in your changes and suggestions.

On Mar 28, 2017, at 7:50 AM, Tikitu de Jager notifications@github.com wrote:

@ahalls https://github.com/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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/libical/XbICalendar/issues/47#issuecomment-289775781, or mute the thread https://github.com/notifications/unsubscribe-auth/AAt2TV5UlKblm2d87MZviG0qyPBWbgM-ks5rqRAmgaJpZM4MrW9h.

-- Andrew

Mobile Apps To Engage Your Audience

Eagle, Colorado | Mandurah, Western Australia +1 (301) 404 7263 |+61 (0) 484 339 261 andrew@galtsoft.com www.linkedin.com/in/ahalls

tikitu commented 7 years ago

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.