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

bug: X-WR-TIMEZONE not supported #71

Closed
niccokunzmann opened this issue Dec 29, 2021 · 11 comments
Closed

bug: X-WR-TIMEZONE not supported #71

niccokunzmann opened this issue Dec 29, 2021 · 11 comments
Labels
bug Something isn't working enhancement New feature or request help wanted Extra attention is needed question Further information is requested

Comments

@niccokunzmann
Copy link
Owner

niccokunzmann commented Dec 29, 2021

Describe the bug

It seems that several people encounter a behaviour which is unexpected: When X-WR-TIMEZONE is present, Google and other calendars show a events at a different time in different time zones.

To Reproduce

See Issues #53, #59, #61

Expected behavior

The expected behaviour of this module is to handle X-WR-TIMEZONE as these issues clearly show.

Version:

Additional context

Suggested implementation

This varies:

  1. This library can handle X-WR-TIMEZONE by default.
  2. A new library is reated to handle X-WR-TIMEZONE
  3. The user handles it, conclusion from bug: UTC (Zulu) events seem to not be converted to local time (X-WR-TIMEZONE) #53, but not part of this Issue.

An untested implementation is discussed here.


We're using Polar.sh so you can upvote and help fund this issue. We receive the funding once the issue is completed & confirmed by you. Thank you in advance for helping prioritize & fund our work. Fund with Polar
@niccokunzmann
Copy link
Owner Author

niccokunzmann commented Jan 12, 2022

For choosing way 2, I created this: https://github.com/niccokunzmann/x-wr-timezone
Collaboration welcome! We need examples. The idea is that people are not bound to this library but can still fix the issue.

My idea is to integrate the x-wr-timezone library once it works into this one by default.

@NicoHood @sirdan69 @asimonson1125 @martinm76 Notifying you as you helped in this regard. Also inviting you to collaborate!

For reference, a discussion in icalendar: collective/icalendar#343

@NicoHood
Copy link

I think the idea is to optionally depend on your new library and if it is available, it will normalize the input ical file? Or maybe you could just add the normalize to the example and its all good, no need to change a single line of this library?

@niccokunzmann
Copy link
Owner Author

Hm. My thought would be that Users of this library want it easy and not Test it out and run into a x-wr-timezone calendar later, untested. I would integrate it and use it by default, so that the meaning of x-wr-timezone is supported by default. This means also changing the major Version number. You would be able to opt out, though.

It only affects results of no timezone is used in the arguments.

What do you think of that? It's probably good to list pro and con of each Option.

@martinm76
Copy link

Very interesting. I'll try it out on my work calendar soon and see if there is any difference from the modification I made inline when parsing it afterwards.

@NicoHood
Copy link

Pro (integrating):

  • There is no reason in not normalizing it. If you don't, the events are "broken"
  • You cannot do anything wrong, as the library implements it for you
  • No additional code in the user code, just a new dependency
  • If the calendar was already normalized, it would still be compatible

Con (not integrating)

  • If you (for some unknown reason) use the rest of the calendar not normalized, you have different timezones. So there must be a switch to disable that feature
  • It would work with old versions of this library. Not the best argument, but trust me, those setups exist in the wild.
  • Not backward compatible (major version bump)
  • New, hard dependency
  • You (very likely) need to normalize the calendar before the recurring filtering, if you also want to use it on different places. This means, that the library call to x-wr-timezone is useless
  • There are also other librarys that do not use x-wr-timezone as dependency, and there you need to normalize first anyways. It would be more freedome to the user which normalize app to use
  • Not all calendars use x-wr-timezone. Having this (hard) dependency would make it more complicated for those users, even if they do not need it. For example linux distributions now MUST package both librarys.

Suggestion:
Make a minimal change in your example and see how much the integration costs. I'd say it is an import, a function call and maybe an explanation comment. I'd go for the dependent solution first.

@niccokunzmann
Copy link
Owner Author

@martinm76 thanks for checking!

@NicoHood Thanks for the big list.

I created #77 so you can have a look.

For some reason, I still have problems with the tests - I think, having a few more example calendars would be useful. All tests run when I integrate x_wr_timezone - I did not expect that.

So: a good example would be a calendar using

  • X-WR-TIMEZONE that is Ameria/Asia, as far away from UTC as possible
  • one event with dtstart, dtend
  • one event with repetitions of which one was moved in time

If you can provide me with an ics file that does that, would be awesome! At the moment, I cannot create that myself - would take me hours and frozen fingers.

@niccokunzmann
Copy link
Owner Author

v1.0.0b supports X-WR-TIMEZONE. Please test it:) Thanks for your support!

@niccokunzmann
Copy link
Owner Author

I decided to include the small library. It does not really change much, only if you use at and between without timezone as argument. The tests indicated that no behavior that was tested needed to change.

@martinm76
Copy link

Sounds good! I did a quick test on my work calendar and running my script on the original and the one piped though x-wr-tinezone, with ny astimezone calls intact, and the final result was identical. The calendars differed a fair bit, but most of it seemed to be down to the order of the fields. I only checked for a week, but it parsed the entire calendar without issue, so pretty sure it is doing exactly what it should 😀👍🏻🙏🏻

@sirdan69
Copy link

Hi Nico, I tried my test case against your latest changes around x-wr-timezone and it works where it used to crash. This was for the case where Confluence-created calendar contained only all-day events. Thanks for all your work on this issue!

@niccokunzmann
Copy link
Owner Author

thanks for your feedback! I will close this. It looks like it does about what it should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants