-
Notifications
You must be signed in to change notification settings - Fork 3
Don't use global TimeZoneRegistry
#36
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.