Replace java.time APIs with kotlin(x).(date)time#3001
Replace java.time APIs with kotlin(x).(date)time#3001MajorTanya wants to merge 1 commit intomihonapp:mainfrom
Conversation
kotlinx-datetime explicitly says in its README that it does not cover i18n, so we have to stick with using their toJavaX converters wherever we localise dates and times. Yes, some of these replacements are... questionable. But I wanted to try and maximise the replacement for now.
MajorTanya
left a comment
MajorTanya
left a comment
There was a problem hiding this comment.
Please take a look at some of my notes.
|
|
||
| # KotlinX Datetime | ||
| -keep,allowoptimization class kotlinx.datetime.** { public protected *; } |
There was a problem hiding this comment.
Not sure if this is needed, but I saw it exists for kotlinx.coroutines and kotlin.time, so I copied it over
| implementation(projects.domain) | ||
| implementation(projects.core.common) | ||
|
|
||
| implementation(kotlinx.datetime) |
There was a problem hiding this comment.
Note: Added kotlinx.datetime as a dependency for the data module.
|
|
||
| private val testTime = ZonedDateTime.parse("2020-01-01T00:00:00Z") | ||
| private val testZoneId = ZoneOffset.UTC | ||
| private val testTime = LocalDateTime.parse("2020-01-01T00:00:00") |
There was a problem hiding this comment.
The parser throws if zone information is present
| implementation(kotlinx.bundles.coroutines) | ||
| implementation(kotlinx.bundles.serialization) | ||
|
|
||
| implementation(kotlinx.datetime) |
There was a problem hiding this comment.
Note: Added kotlinx.datetime as a dependency for the domain module.
|
|
||
| implementation(kotlinx.immutables) | ||
|
|
||
| implementation(kotlinx.datetime) |
There was a problem hiding this comment.
Note: Added kotlinx.datetime as a dependency for the presentation-widget module.
| timeZone: TimeZone = TimeZone.currentSystemDefault(), | ||
| dateTime: LocalDateTime = Clock.System.now().toLocalDateTime(timeZone), | ||
| window: Pair<Long, Long> = fetchInterval.getWindow(dateTime.date, timeZone), |
There was a problem hiding this comment.
Not a mistake that these are in a different order to literally everywhere else. To avoid querying the timezone twice and to be able to use the parameter for the default value of dateTime, I had to make it initialise before that. Probably the biggest wart of these changes and could be swapped around, but losing the default parameter elegance.
| private var timeZone = TimeZone.currentSystemDefault() | ||
| private var now = Clock.System.now().toLocalDateTime(timeZone) | ||
| private var currentFetchWindow = fetchInterval.getWindow(now.date, timeZone) | ||
|
|
||
| init { | ||
| now = ZonedDateTime.now() | ||
| currentFetchWindow = fetchInterval.getWindow(now) | ||
| timeZone = TimeZone.currentSystemDefault() | ||
| now = Clock.System.now().toLocalDateTime(timeZone) | ||
| currentFetchWindow = fetchInterval.getWindow(now.date, timeZone) |
There was a problem hiding this comment.
This code confused me. The init block seems to set the vars to the same values as the initial values -- is this redundant or am I missing some finer details of the restore process? I didn't see any reasoning in the commit history for this section.
| .toLocalDateTime(from) | ||
| .toInstant(to) |
There was a problem hiding this comment.
Yes this converts between zones. Internally, the toInstant method uses atZone(to) on the JVM implementation.
|
|
||
| val emptyFieldCount = weekDays.indexOf(selectedYearMonth.atDay(1).dayOfWeek) | ||
| val daysInMonth = selectedYearMonth.lengthOfMonth() | ||
| val emptyFieldCount = weekDays.indexOf(selectedYearMonth.firstDay.dayOfWeek.toJavaDayOfWeek()) |
There was a problem hiding this comment.
Days of the week are an i18n matter, so we have to fall back to Java's stuff here (thanks to people who wrongly insist that the week starts on Sunday)
| implementation(platform(kotlinx.coroutines.bom)) | ||
| implementation(kotlinx.bundles.coroutines) | ||
|
|
||
| implementation(kotlinx.datetime) |
There was a problem hiding this comment.
Note: Added kotlinx.datetime as a dependency for the app module.
1 participant
kotlinx-datetime explicitly says in its README that it does not cover i18n, so we have to stick with using their toJavaX converters or plain
java.time.Xwherever we localise dates and times.Yes, some of these replacements are... questionable. But I wanted to try and maximise the replacement for now.
Closes #1947.
I'll add some comments for things I am not entire sure about.