bitfireAT / ical4android

Allows usage of iCalendar files with the Android calendar provider
GNU General Public License v3.0
19 stars 10 forks source link

Update ical4j to 3.2.17 #139

Closed rfc2822 closed 7 months ago

rfc2822 commented 7 months ago

A test fails when updating to 3.2.15

Depends on https://github.com/ical4j/ical4j/issues/676 Depends on https://github.com/ical4j/ical4j/issues/678

ArnyminerZ commented 7 months ago

Looks like there's an issue with TZ aliases in the latest release of ical4j. Ben updated the loading method of aliases, and it seems like it broke something. See the change:

https://github.com/ical4j/ical4j/compare/ical4j-3.2.14..ical4j-3.2.15#diff-ca0f72d0501ea912d0342c8d541e40aade5c74a11fb65580fb308993aa9a4973

Calling:

DateUtils.ical4jTimeZone("GMT")

doesn't work, but calling

DateUtils.ical4jTimeZone("Etc/GMT")

does. If I forcefully update the test to do:

@Test
fun testLoadSystemTimezones() {
    for (id in ZoneId.getAvailableZoneIds()) {
        val name = ZoneId.of(id).getDisplayName(TextStyle.FULL, Locale.US)
        val info = try {
            DateUtils.ical4jTimeZone(
                if (id == "GMT") "Etc/GMT"
                else id
            )
        } catch(e: Exception) {
            Assert.fail("Invalid system timezone $name ($id)")
        }
        if (info == null)
            assertNotNull("ical4j can't load system timezone $name ($id)", info)
    }
}

It passes for GMT, but then Israel fails. Then I add else if (id == "Israel") "Asia/Jerusalem" and Israel works, but Greenwich fails. Looks definitely like a bug on their side.

rfc2822 commented 7 months ago

It passes for GMT, but then Israel fails. Then I add else if (id == "Israel") "Asia/Jerusalem" and Israel works, but Greenwich fails. Looks definitely like a bug on their side.

Is it really a problem in ical4j or in Android compatibility / our Android timezone adapter? Could you please try to create a short test that illustrates the problem just with ical4j methods? If that's possible and shows a problem, we can report it at ical4j.

ArnyminerZ commented 7 months ago

Is it really a problem in ical4j or in Android compatibility / our Android timezone adapter?

I think it's a problem with ical4j, because these aliases are defined correctly, but it looks like they are not loaded correctly:

https://github.com/ical4j/ical4j/blob/ical4j-3.2.15/src/main/resources/net/fortuna/ical4j/model/tz.alias

rfc2822 commented 7 months ago

Ok then please create an issue at ical4j with a short test that illustrates the problem (just loads the TZ), then we can set this issue as dependency of that one.

ArnyminerZ commented 7 months ago

Ben has released a fix for it in 3.2.16, see.

ArnyminerZ commented 7 months ago

Looks like it's fixed 😄

rfc2822 commented 7 months ago

Great!

ArnyminerZ commented 7 months ago

@rfc2822 please can you take a look at the failing test? I'm not sure why it's there, there isn't anything at ICalPreprocessor that should replace that name afaik

rfc2822 commented 7 months ago

@rfc2822 please can you take a look at the failing test? I'm not sure why it's there, there isn't anything at ICalPreprocessor that should replace that name afaik

ICalPreprocessor applies DatePropertyRule and DateListPropertyRule which are provided by ical4j. They call TzHelper.correctTzParameterFrom, which call getCorrectedTimeZoneIdFrom, which should somehow replace (English) MS time zone times by IANA tzdata ones.

Seems that this fails in 3.2.16 – probably because there were changes regarding this, which is also why 3.2.16 was released. So I think this should again be reported to ical4j (with a test that applies DatePropertyRule to a DTSTART with MS timezone; should replace the name with ical4j 3.2.14, but not with 3.2.16).

ArnyminerZ commented 7 months ago

I've reported it to Ben at https://github.com/ical4j/ical4j/issues/678

github-actions[bot] commented 7 months ago

This PR/issue depends on:

rfc2822 commented 7 months ago

We can update to 3.2.17 :)