Events with the Europe/Dublin timezone are displayed on 23:00 on previous day

[ArnyminerZ]

Closes #94

[ArnyminerZ]

I've refactored ICalPreprocessor a bit so it supports both Rfc5545PropertyRule and Rfc5545ComponentRule. The abstract class ReplaceInvalidTzTimezoneRule aims to provide a "generic" parent class to add new replacements in the future if necessary. We can also add more timezones to ReplaceInvalidTzTimezoneRule.timeZoneReplacements if we want to. Maybe the instantiation of VTimeZone is not the most optimal one, but I believe it works fine.

Classes that use ReplaceInvalidTzTimezoneRule should implement repair by checking oldTzId.

@rfc2822 please take a look when possible, and tell me if this approach is correct or if should I change to something else 😄

[rfc2822]

Can you please have a look @sunkup ?

[sunkup]

Can you please have a look @sunkup ?

Yes, will check it out tomorrow.

[sunkup]

Here we are, finally! Had this ready since last thursday, but wanted to be available for questions - as I know you and Ricki tend to work in the weekends as well 😉

So, that's a lot of code ... I think we can do with less :)

Reading Rickis comment I find we want to remove this workaround as soon as Ben has fixed the issue in ical4j or we move on to ical4j v4 (in case the problem does not occur there). So in my opinion we do not need to add this much code - just to remove it again. I think a small and maybe a bit "dirty" workaround is fine and more appropriate - even if we need to replace/fix more timezones in the future - until ical4j is ready.

From Rickis comment:

In any case, we have to provide a solution for our users. Currently the only thing that crosses my mind is changing Europe/Dublin to Europe/London automatically:

• Create a test in Ical4jTest that checks whether the ical4j problem still occurs [...] This test will remind us to remove the workaround when it's not needed anymore (should be documented in the test KDoc).

The mentioned KDoc annotation is missing for the test. We should add it, so we understand we can remove the test alongside with any added preprocessing when the test suddenly fails.

For the preprocessing: I would simply add another stream preprocessor for now. The following, worked just fine for me with the tests you created (applying the stream preprocessors with preprocessStream() instead of preprocessCalendar() which applies the property rules.

object FixInvalidTimezonesPreprocessor: StreamPreprocessor() {

    private val TZOFFSET_REGEXP = Regex("Europe\\/Dublin",
        setOf(RegexOption.MULTILINE, RegexOption.IGNORE_CASE))

    override fun regexpForProblem() = TZOFFSET_REGEXP

    override fun fixString(original: String) =
        original.replace(TZOFFSET_REGEXP) {
            Ical4Android.log.log(Level.FINE, "Changing problematic Dublin timezone to London timezone", it.value)
            "Europe/London"
        }

}

Because just replacing "Europe/Dublin" is so simple, we would not even need any regex, which might speed the code up a bit, but I guess this is neglectible in favor of added clarity.

• Add a ReplaceInvalidTzDatetimeRule() to ICalPreprocessor.propertyRules that checks DATE-TIMEs and replaces Dublin by London.

I know Ricki proposed to create a preprocessor property rule - I am not sure whether this is what made your code so complex, but if it did, I feel we should add the stream preprocessor instead, like I did above. Otherwise feel free to implement a shorter version via preprocessor property rule.

In any case, try to add the documentation I mentioned on the test and maybe also the added preprocessing code. IE: "If test fails, check ical4j has fixed this issue [link reference]. If so, we can remove this test together with [FixInvalidTimezonesPreprocessor] (or any other other related impl.)"

And for FixInvalidTimezonesPreprocessor: "Remove once fixed in ical4j [link reference]"

[ArnyminerZ]

Yeah, I agree that a preprocessor is much simpler, using a rule surely made the code more complex. I just made it that way because @rfc2822 asked for it, and I thought it had bigger implications or something, but I surely can replace it with a preprocessor.

[sunkup]

Yeah, I agree that a preprocessor is much simpler, using a rule surely made the code more complex. I just made it that way because @rfc2822 asked for it, and I thought it had bigger implications or something, but I surely can replace it with a preprocessor.

I see. Well then let's hear Rickis call on this before you change anything. The changes should be quick anyways 👍

Or make the changes with new commits, not force pushing the branch. Then we can go back, in case Ricki wants the ReplaceInvalidTzDatetimeRule.

[rfc2822]

I have tried a completely different approach in #96 – what do you think?

The problems with a StreamPreprocessors are:

• This solution would replace every Europe/Dublin, even it for instance occurs in a SUMMARY or DESCRIPTION. And it could be a strange experience if you create an event with description "Let's meet in Europe/Dublin" and it's rewritten to "Let's meet in Europe/London".
• So it would be better to catch only the relevant TZIDs, and then it begins with RegExp juggling, and it won't work when lines are folded etc.

So StreamPreprocessors should be the last resort. I originally thought that a workaround with Rfc5545Rules would be possible, but they don't work in this case because the initial parsing already destroys the DtStart DateTime, so every attempt to fix it afterwards will fail.

This is how I came to the ical4j factories (because they work when the parameter values are initially created). I had a solution that provides a custom parameter factory to the CalendarBuilder, where the TZID parameter was processed (Europe/Dublin → Europe/London). It worked, however it still didn't looked like a proper solution to me because it doesn't work around the actual problem but instead rewrites every Europe/Dublin to Europe/London, even if it would not be problematic.

So I came to the TimezoneRegistry solution. It requires a bit of knowledge how ical4j internally works. Basically it has a map of time zones (ID → VTIMEZONE) with pre-defined VTIMEZONEs. If an iCalendar contains a custom VTIMEZONE for a specific ID, it's overwritten in the TimeZoneRegistry. Otherwise, the default stays active. We can however block the updates for certain unwanted time zone definitions, and the default Europe/Dublin doesn't cause problems.

[sunkup]

So StreamPreprocessors should be the last resort. I originally thought that a workaround with Rfc5545Rules would be possible, but they don't work in this case because the initial parsing already destroys the DtStart DateTime, so every attempt to fix it afterwards will fail.

Ah, yes. Good catch. Would have never thought of your approach, but it seems like its abusing the time zone registry of ical4j a bit 😁 But it's short and evades the drawbacks you mentioned. If that works I am all for it :)

[ArnyminerZ]

Looks good to me, seems more reasonable and cleaner