Skip to content

Add an equivalent of TemporalAdjusters #235

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

Closed
Nek-12 opened this issue Oct 15, 2022 · 10 comments
Closed

Add an equivalent of TemporalAdjusters #235

Nek-12 opened this issue Oct 15, 2022 · 10 comments

Comments

@Nek-12
Copy link

Nek-12 commented Oct 15, 2022

TemporalAdjusters are a useful family of classes in the java.time library
They allow to change a specific field value of a datetime according to a specific pattern commonly used by humans, avoiding the complications of the computation of such a value.

Common use cases (basically th list of adjusters from java.time):

  • Getting a date, but with the day of month set to the first one
  • Getting a date with day of month set to a last one in the current month
  • In the same fashion, first day of next month
  • First day of next / previous year / week
  • returning a date, with the day set to a specific number of a given day of week. For example "second tuesday of March"
  • Getting a date with the next given day of week (whatever date that day of week would result in) E.g., next monday will result in a wrap of the day of week if you call this on "tuesday" date, adding total 6 days to the date.
  • Getting a date with previous day of week of a given value

My personal use case is this:
I want to set an alarm that fires on given days of week each week, and each time an alarm is fired, I schedule an alarm for the next day of week that alarm is enabled on. I want to get the next date for my specific day of week to trigger the next alarm.

I propose implementing these as extension functions on LocalDate / LocalDateTime

@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented Oct 17, 2022

Thanks! This looks like an interesting proposal, and we received several requests in this vein already, though we didn't yet manage to find a good API shape for this.

A question about your use case though: let's say an alarm is enabled on Monday, Tuesday, and Thursday. On Tuesday, the alarm goes off, and your code needs to schedule an alarm for Thursday (not Monday or another Tuesday). How would it know that it needs Thursday and not something else? Could you write the full code if we pretend that we have a function like LocalDate.nextDateWithWeekDay(dayOfWeek: DayOfWeek): LocalDate?

EDIT: actually, let's not even pretend, here's the function:

public fun LocalDate.nextDateWithWeekDay(newDayOfWeek: DayOfWeek): LocalDate =
    plus((newDayOfWeek.isoDayNumber - dayOfWeek.isoDayNumber).mod(7), DateTimeUnit.DAY)

@Nek-12
Copy link
Author

Nek-12 commented Dec 3, 2022

Hello, here are the functions I'm using right now. These are essentially copy-pasted from the java.time library source code:

fun LocalDateTime.withNextDayOfWeek(
    dayOfWeek: DayOfWeek,
    zone: TimeZone,
): LocalDateTime {
    val daysDiff = this.dayOfWeek.value - dayOfWeek.value
    val daysToAdd = if (daysDiff >= 0) 7 - daysDiff else -daysDiff
    return plusDays(daysToAdd, zone)
}

fun LocalDateTime.previousOrSameDayOfWeek(
    dayOfWeek: DayOfWeek,
    zone: TimeZone,
): LocalDateTime {
    val daysDiff = dayOfWeek.value - this.dayOfWeek.value
    if (daysDiff == 0) return this

    val daysToAdd = if (daysDiff >= 0) 7 - daysDiff else -daysDiff
    return minusDays(daysToAdd, zone)
}

(I know you won't like my api of these, it's my personal choice)

@dkhalanskyjb
Copy link
Collaborator

Please reread my last message. I already provided a function similar to your withNextDayOfWeek, so the question is not how to implement this, but why. How is it used in the code, and how does it help with your use case?

@Nek-12
Copy link
Author

Nek-12 commented Dec 4, 2022

Okay, I'm sorry I misunderstood you.
Here's the actual code used:

fun Ritual.getNextAlarmTime(now: LocalDateTime, zone: TimeZone): LocalDateTime? {
    if (!shouldBeNotified) return null

    val days = repeat.toSet()

    val withTime = now.withTime(time!!.copy(second = 0), 0) //adjusts LocalDateTime to specified Time

    return if (now.dayOfWeek in days && withTime > now) {
        // alarm can go off today, and the time has not passed yet
        // (alarms for less than 1 min do not count)
        withTime
    } else {
        // if no day is next in the week, take closest to the start of the week
        val dayOfWeek = days.firstOrNull { it > now.dayOfWeek } ?: days.first()
        withTime.withNextDayOfWeek(dayOfWeek, zone)
    }
}

fun LocalDateTime.asStartOfWeek(zone: TimeZone) =
    withPreviousOrSameDayOfWeek(DayOfWeek.MONDAY, zone).asMidnight()

P.S. There is no support for Sunday as the first day of week yet, but it will take minor adjustments

@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented Dec 4, 2022

Great, thank you! This is clearly a solid use case.

An unrelated note: assuming plusDays converts the LocalDateTime to Instant and back to do the arithmetic, this code contains a subtle bug. Here's what could hypothetically happen:

  • In Berlin, a fireman, nurse, or in general someone who often doesn't sleep at night sets the alarm for Wednesdays and Sundays for 02:30 A.M.
  • On 22nd March 2023, the alarm goes off at 02:30 A.M., as expected. The alarm is scheduled for 26th March 2023.
  • On 26th March 2023, there's no 02:30 A.M. due to the time gap: the clocks are moved forward from 02:00 to 03:00. Okay, the alarm goes off at 03:30 A.M. However…
  • On 29nd March 2023, the alarm goes off also at 03:30, due to a bug. You look at the LocalDateTime at the moment of the alarm ringing, but that LocalDateTime itself can change suddenly.
val zone = TimeZone.of("Europe/Berlin")
val wednesday = LocalDateTime(2023, Month.MARCH, 22, 2, 30).toInstant(zone)
val sunday = wednesday.plus(4, DateTimeUnit.DAY, zone)
val anotherWednesday = sunday.plus(3, DateTimeUnit.DAY, zone)
println(anotherWednesday.toLocalDateTime(zone)) // 2023-03-29T03:30

Instead, you should explicitly store the LocalTime at which the alarm should fire and, on each date, combine it with the date to form a LocalDateTime. This way, LocalTime won't get corrupted by such transitions.

@Nek-12
Copy link
Author

Nek-12 commented Dec 4, 2022

Do I understand you correctly that the bug happens because the LocalDateTime is reused? Here's how my system works:

  1. The user sets a specific time for their alarm. That alarm will always fire at a local time set on the device. This time value is tz-unaware and persistently stored.
  2. The system schedules an alarm for that time and next weekday
  3. If the system timezone has changed, the system scraps all alarms and reschedules them again, using the new system LocalDateTime (I can see we assume that the localDateTime is already updated at this point, but it's not up to me, it's the OS to specify correctly)
  4. As soon as the alarm for a particular Ritual rings, the system removes the next alarm for that Ritual and schedules a new one using newly-calculated LocalDateTime based on the tz-unaware time value the user has set. The LocalDateTime is obtained anew and then merged with the time value.

I think that this behavior should work correctly, although unit-testing it would be a pain so I haven't...

@dkhalanskyjb
Copy link
Collaborator

Do I understand you correctly that the bug happens because the LocalDateTime is reused?

Yep.

If the system timezone has changed

The timezone itself doesn't change in my example, it's still Europe/Berlin, only the daylight-saving time state does. If you check only the timezone itself, then this check is irrelevant to the bug.

the system removes the next alarm for that Ritual and schedules a new one using newly-calculated LocalDateTime based on the tz-unaware time value the user has set

Then I don't understand how Ritual.getNextAlarmTime is used. You take the current Ritual, get the new LocalDateTime, then strip out the LocalTime part and replace it with the proper one? If so, okay, that works, but why not just return a LocalDate instead? This would even allow you to eliminate the zone: TimeZone parameter.

@Nek-12
Copy link
Author

Nek-12 commented Dec 4, 2022

No, getNextAlarmTime is used any time there is a need to schedule an alarm. This includes reboots, timezone changes, changing time of the alarm, and when the alarm rings. At any point in time, only one alarm is scheduled - the closest next one. The function is invoked each time an alarm rings. This was done precisely to avoid alarms broken by daylight time savings changes, tz changes, device reboots and other cases. The only part that is persistent is the hour and minute of the alarm.

@dkhalanskyjb
Copy link
Collaborator

Ah, ok! Everything should be good then.

@dkhalanskyjb
Copy link
Collaborator

Closing in favor of #325

@dkhalanskyjb dkhalanskyjb closed this as not planned Won't fix, can't repro, duplicate, stale Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants
@Nek-12 @dkhalanskyjb and others