Skip to content
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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Sep 5, 2023
Merged

DatePicker. Fix today circle #782

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
82aac5b
apply locale before pattern on darwin
alexzhirkevich Aug 21, 2023
4e80c9e
Merge branch 'jb-main' of https://github.com/alexzhirkevich/compose-m…
alexzhirkevich Aug 21, 2023
274d556
Merge branch 'jb-main' of https://github.com/alexzhirkevich/compose-m…
alexzhirkevich Aug 30, 2023
f196d45
datepicker today circle fix
alexzhirkevich Aug 30, 2023
aae821a
utc
alexzhirkevich Aug 30, 2023
135cc22
day of week
alexzhirkevich Aug 30, 2023
9292441
weekdays
alexzhirkevich Aug 31, 2023
ad02921
comment
alexzhirkevich Aug 31, 2023
b472b97
apply suggestion
alexzhirkevich Aug 31, 2023
6a9b273
refactor atStartOfDay -> at midnight
alexzhirkevich Aug 31, 2023
5af54d1
midnight outside the class
alexzhirkevich Aug 31, 2023
37d4fca
midnight outside the class
alexzhirkevich Aug 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions ...material3/src/skikoMain/kotlin/androidx/compose/material3/KotlinxDatetimeCalendarModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,26 @@ import kotlinx.datetime.LocalDate
import kotlinx.datetime.Month
import kotlinx.datetime.TimeZone
import kotlinx.datetime.atStartOfDayIn
import kotlinx.datetime.atTime
import kotlinx.datetime.isoDayNumber
import kotlinx.datetime.plus
import kotlinx.datetime.toInstant
import kotlinx.datetime.toLocalDateTime

internal class KotlinxDatetimeCalendarModel : CalendarModel {

override val today: CalendarDate
get() = Clock.System.now()
.toLocalDateTime(systemTZ)
.date
.atStartOfDayIn(TimeZone.UTC)
.toCalendarDate(TimeZone.UTC)
get() {
val localDate = Clock.System.now().toLocalDateTime(systemTZ)
return CalendarDate(
year = localDate.year,
month = localDate.monthNumber,
dayOfMonth = localDate.dayOfMonth,
utcTimeMillis = localDate.date
.atStartOfDayIn(TimeZone.UTC)
Copy link
Member

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).

Copy link
Member

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.

Copy link
Author

@alexzhirkevich alexzhirkevich Aug 31, 2023 •
edited
Loading

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:

compose-multiplatform-core/compose/material3/material3/src/commonMain/kotlin/androidx/compose/material3/CalendarModel.kt

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 🥲🔫)

Copy link
Author

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

.toEpochMilliseconds()
)
}

override val firstDayOfWeek: Int
get() = PlatformDateFormat.firstDayOfWeek
Expand Down