Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the global TimeZoneRegistry usage and deprecated DateUtils.ical4jTimeZone calls by introducing explicit TimeZoneRegistryFactory.getInstance().createRegistry() usage and passing registries into key functions.
- Replaced global
DateUtils.ical4jTimeZonelookups withTimeZoneRegistry.getTimeZone(...)calls. - Updated APIs (e.g.,
toIcal4jDateTime,androidifyTimeZone,androidStringToRecurrenceSet) to accept aTimeZoneRegistryparameter. - Removed deprecated registry helpers and updated all affected tests.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
EventValidatorTest.kt |
Fixed extension-call syntax and removed unused import |
TimeApiExtensions.kt |
Added tzRegistry parameter to toIcal4jDateTime |
DateUtils.kt |
Removed global registry and ical4jTimeZone helper |
AndroidTimeUtils.kt |
Updated timezone helpers to require TimeZoneRegistry |
Multiple *.kt (main & tests) |
Replaced DateUtils.ical4jTimeZone imports/calls with registry |
Comments suppressed due to low confidence (4)
lib/src/main/kotlin/at/bitfire/synctools/icalendar/ICalendarParser.kt:36
- The KDoc lists a
tzRegistryparameter but theparsefunction signature only acceptsreader; please update or remove the outdated@param tzRegistrytag to match the actual signature.
* @param tzRegistry time zone registry where VTIMEZONE definitions of the iCalendar will be put
lib/src/main/kotlin/at/bitfire/ical4android/util/AndroidTimeUtils.kt:89
- [nitpick] Defining a new
TimeZoneRegistryinside this function on each call can lead to repetitive instantiation; consider passing a sharedtzRegistryinto the helper or hoisting it to the object scope to reduce duplication.
val tzRegistry by lazy { TimeZoneRegistryFactory.getInstance().createRegistry() }
lib/src/main/kotlin/at/bitfire/ical4android/util/DateUtils.kt:110
- The
parseVTimeZonebuilder no longer receives aTimeZoneRegistry, which may prevent VTIMEZONE definitions from resolving correctly; restoreCalendarBuilder(tzRegistry)or inject a registry instance.
val builder = CalendarBuilder()
lib/src/main/kotlin/at/bitfire/ical4android/DmfsTask.kt:77
- [nitpick] A per-instance lazy registry works but each task subclass also directly calls
createRegistry()elsewhere; consider centralizing registry creation (e.g., in a shared singleton) to avoid scattered instantiation.
protected val tzRegistry by lazy { TimeZoneRegistryFactory.getInstance().createRegistry() }
sunkup
left a comment
There was a problem hiding this comment.
Looks ok. I don't understand why we comment on duration = null instead of deleting it though?
Closes #35.
With this the tests of #32 seem to work fine.