-
Notifications
You must be signed in to change notification settings - Fork 81
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
DatePicker. Fix today circle #782
Conversation
@alexzhirkevich I started investigating before I noticed your PR.
Indeed, this seems to be the problem. |
...l3/material3/src/skikoMain/kotlin/androidx/compose/material3/KotlinxDatetimeCalendarModel.kt
Outdated
Show resolved
Hide resolved
month = localDate.monthNumber, | ||
dayOfMonth = localDate.dayOfMonth, | ||
utcTimeMillis = localDate.date | ||
.atStartOfDayIn(TimeZone.UTC) |
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.
https://kotlinlang.org/api/kotlinx-datetime/kotlinx-datetime/kotlinx.datetime/at-start-of-day-in.html says atStartOfDay
isn't necessarily at midnight, and we do want midnight here because of how isToday
is computed:
val dateInMillis = month.startUtcTimeMillis + (dayNumber * MillisecondsIn24Hours)
val isToday = dateInMillis == today.utcTimeMillis
that's why I suggested to use atTime(0,0)
.
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.
Or maybe both will be wrong, that is neither will match dateInMillis
? I'm not sure. The way isToday
is computed looks bad to me.
But Android uses
utcTimeMillis = systemLocalDate.atTime(LocalTime.MIDNIGHT)
.atZone(utcTimeZoneId).toInstant().toEpochMilli()
so maybe it's better to stick with that, unless you're sure atStartOfDayIn
is correct.
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.
Hmm, this is what CalendarModel interface comments (from canonical date method) says about it:
Lines 94 to 95 in 8f57b8c
* The returned date will hold milliseconds value that represent the start of the day, which may | |
* be different than the one provided to this function. |
So it does not exactly say it needs to be 00:00, but start of day 🤔
Maybe today
method should behave differently
Newer android calendar model takes it at midnight, lets do the same
(but canonical date it takes at start of day 🥲🔫)
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.
Aligned all methods with the Android implementation
...l3/material3/src/skikoMain/kotlin/androidx/compose/material3/KotlinxDatetimeCalendarModel.kt
Outdated
Show resolved
Hide resolved
@alexzhirkevich Would you like me to merge this? |
Yep if everything looks good to you |
Proposed Changes
Fix one more bug from Kotlinx datetime CalendarModel (support date picker on darwin and web) #717 🥲.
Today time millis should be at the midnight.
Added the last test from Android that checks the equality of legacy and new models. Forgot to do it in the first PR.
Prostite brakodela 🙏
Use narrow weekdays format instead of short (as in non-legacy model on Android).
It should be more consistent with Android 26+ (see screenshots from fixed issue).
Also removed hardcoded weekdays names as all platforms have localized ones.
Replace input format building with existing function from common (missed that it already exists). It should be more stable for unusual locales.
atStartOfDay
can produce non-midnight time. Some calendar model methods require midnight and some start of day. Aligned all methods with the newer android implementationTesting
Test:
EqualityOfCalendarModelsTest
. This should protect from other bugs i believeIssues Fixed
Fixes: JetBrains/compose-multiplatform#3591