Skip to content
This repository was archived by the owner on Jun 9, 2025. It is now read-only.

Conversation

@rfc2822
Copy link
Member

@rfc2822 rfc2822 commented May 6, 2025

No description provided.

@rfc2822 rfc2822 self-assigned this May 6, 2025
@rfc2822 rfc2822 added the refactoring Quality improvement of existing functions label May 6, 2025
@rfc2822 rfc2822 requested a review from Copilot May 6, 2025 17:49
Copy link

Copilot AI left a 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 aims to remove unnecessary thread context loader checks from multiple components of the ical4j integration. Key changes include removal of Ical4Android.checkThreadContextClassLoader invocations from tests and production code, and the complete removal of the Ical4Android object with its associated context loader check.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
lib/src/test/kotlin/at/bitfire/ical4android/Ical4jServiceLoaderTest.kt Retains a test ensuring the functionality works with a null context class loader.
lib/src/main/kotlin/at/bitfire/ical4android/util/DateUtils.kt Removed context loader checks from the init block and parseVTimeZone function.
lib/src/main/kotlin/at/bitfire/ical4android/Task.kt Removed the context loader check from the write function.
lib/src/main/kotlin/at/bitfire/ical4android/JtxICalObject.kt Removed context loader checks from both getICalendarFormat and write functions; updated imports accordingly.
lib/src/main/kotlin/at/bitfire/ical4android/Ical4jVersion.kt Removed the Ical4Android object including its check function.
lib/src/main/kotlin/at/bitfire/ical4android/ICalendar.kt Removed context loader checks from fromReader and timezoneDefToTzId functions.
lib/src/main/kotlin/at/bitfire/ical4android/Event.kt Removed the context loader check from the write function.
Comments suppressed due to low confidence (9)

lib/src/main/kotlin/at/bitfire/ical4android/util/DateUtils.kt:24

  • The removal of the thread context loader check from the initialization block is consistent with the PR intent; ensure that all use cases that relied on a non-null contextClassLoader are covered by tests.
init { Ical4Android.checkThreadContextClassLoader() }

lib/src/main/kotlin/at/bitfire/ical4android/util/DateUtils.kt:127

  • Removal of the context loader check before building the calendar in parseVTimeZone is aligned with the intended design; double-check that timezone parsing behaves as expected when the contextClassLoader is null.
Ical4Android.checkThreadContextClassLoader()

lib/src/main/kotlin/at/bitfire/ical4android/Task.kt:195

  • The removal of the context loader check in the write method is appropriate; confirm that output generation and Calendar creation continue to work reliably without this check.
Ical4Android.checkThreadContextClassLoader()

lib/src/main/kotlin/at/bitfire/ical4android/JtxICalObject.kt:593

  • Removing the check from getICalendarFormat is in line with the PR goals; ensure that calendar creation is unaffected by a null contextClassLoader.
Ical4Android.checkThreadContextClassLoader()

lib/src/main/kotlin/at/bitfire/ical4android/JtxICalObject.kt:732

  • The removal of the check in the write method should be verified to not impact the output process of the iCalendar; relying on tests can help ensure this change is safe.
Ical4Android.checkThreadContextClassLoader()

lib/src/main/kotlin/at/bitfire/ical4android/Ical4jVersion.kt:10

  • The complete removal of the Ical4Android.checkThreadContextClassLoader function is acceptable given the new service loader behavior; verify that none of the code paths still depend on this check.
fun checkThreadContextClassLoader() { ... }

lib/src/main/kotlin/at/bitfire/ical4android/ICalendar.kt:80

  • Ensure that removing the context loader check from the fromReader method does not affect calendar parsing when the thread contextClassLoader is null.
Ical4Android.checkThreadContextClassLoader()

lib/src/main/kotlin/at/bitfire/ical4android/ICalendar.kt:229

  • The removal of the context loader check in timezoneDefToTzId should be tested to confirm that timezone definitions continue to be processed correctly.
Ical4Android.checkThreadContextClassLoader()

lib/src/main/kotlin/at/bitfire/ical4android/Event.kt:245

  • Removing the context loader check from the write method in Event is consistent with the overall changes; ensure the calendar output remains correct without relying on this check.
Ical4Android.checkThreadContextClassLoader()

@rfc2822 rfc2822 marked this pull request as ready for review May 7, 2025 08:26
@rfc2822 rfc2822 merged commit d077602 into main May 7, 2025
6 checks passed
@rfc2822 rfc2822 deleted the 197-ical4j-doesnt-need-per-thread-contextclassloader-for-serviceloader-anymore branch May 7, 2025 08:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

refactoring Quality improvement of existing functions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ical4j doesn't need per-thread contextClassLoader for ServiceLoader anymore

1 participant