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

integrate kronos-android ntp clock library into project #150

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

integrate kronos-android ntp clock library into project #150

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fe9df02
add Kronos ntp clock lib
rasprague Sep 2, 2020
bcc9950
make app context injectable, make NTPTime use it
rasprague Sep 2, 2020
c7f3c82
refactor so NtpTime is injectable, change VerifyPositiveDiagnosisFrag…
rasprague Sep 2, 2020
d04e542
remove unused function from NtpTime
rasprague Sep 2, 2020
385e199
remove unused import
rasprague Sep 2, 2020
fdff161
remove minor change to DiagnosisKeysToken.kt to remove from this commit
rasprague Sep 2, 2020
2f31082
remove DiagnonsisKeysToken.kt from commit
rasprague Sep 3, 2020
1a90ab6
removed "LocalTime" from NtpTime, add plus/minus days extenstion to I…
rasprague Sep 4, 2020
f2f9b99
force date picker to open on today's month
rasprague Sep 4, 2020
e626be8
minor changes / cleanup
rasprague Sep 7, 2020
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
6 changes: 5 additions & 1 deletion app/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,10 @@ dependencies {
testImplementation "io.mockk:mockk:1.10.0"
// Koin
testImplementation "org.koin:koin-test:$koin_version"

// Kronos ntp clock
def kronos_latest_version = "0.0.1-alpha09"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last dependencies are preserved for the test flavored so let's move this before the test dependencies.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dependency line moved

implementation "com.lyft.kronos:kronos-android:$kronos_latest_version"
}

apply plugin: 'com.google.gms.google-services'
apply plugin: 'com.google.gms.google-services'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the same formatter from AS in the future to avoid this kind of changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

4 changes: 4 additions & 0 deletions app/src/main/java/org/covidwatch/android/BaseCovidWatchApplication.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package org.covidwatch.android

import android.app.Application
import kotlinx.coroutines.GlobalScope
import org.covidwatch.android.data.NtpTime
import org.covidwatch.android.di.appModule
import org.covidwatch.android.di.flavorSpecificModule
import org.covidwatch.android.domain.ProvideDiagnosisKeysUseCase
Expand All @@ -18,6 +19,7 @@ open class BaseCovidWatchApplication : Application() {
private val updateRegionsUseCase: UpdateRegionsUseCase by inject()
private val removeOldExposuresUseCase: RemoveOldDataUseCase by inject()
private val provideDiagnosisKeysUseCase: ProvideDiagnosisKeysUseCase by inject()
private val ntpTime: NtpTime by inject()

override fun onCreate() {
super.onCreate()
Expand All @@ -36,5 +38,7 @@ open class BaseCovidWatchApplication : Application() {
launchUseCase(removeOldExposuresUseCase)
launchUseCase(updateRegionsUseCase)
}

ntpTime.syncInBackground()
}
}
19 changes: 19 additions & 0 deletions app/src/main/java/org/covidwatch/android/data/NtpTime.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package org.covidwatch.android.data

import android.content.Context
import com.lyft.kronos.AndroidClockFactory
import com.lyft.kronos.KronosClock
import java.time.Instant
import java.time.LocalDate
import java.time.ZoneId

class NtpTime (context : Context) {
private val kronosClock : KronosClock = AndroidClockFactory.createKronosClock(context)

private fun currentTimeMillis() : Long = kronosClock.getCurrentTimeMs()

fun syncInBackground() = kronosClock.syncInBackground()

fun nowAsLocalDate() : LocalDate =
Instant.ofEpochMilli(currentTimeMillis()).atZone(ZoneId.systemDefault()).toLocalDate()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ZoneId.systemDefault() introduces the same problem that we tried to eliminate -- it tights the time of the app to the users' settings. I was able to reproduce a one day shift with a timezone that have the next day relatively to mine. It's not that big as before but still a problem.

One way to resolve is just to remove atZone(ZoneId.systemDefault()).

Copy link
Collaborator Author

@rasprague rasprague Sep 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed notion of LocalDate from NtpTime to avoid confusion / simplify the code. Sticking to returning Instants instead

}
4 changes: 4 additions & 0 deletions app/src/main/java/org/covidwatch/android/di/AppModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,10 @@ val appModule = module {
)
}

single {
NtpTime(androidApplication())
}

factory {
ProvideDiagnosisKeysUseCase(
workManager = get(),
Expand Down
8 changes: 5 additions & 3 deletions app/src/main/java/org/covidwatch/android/ui/reporting/VerifyPositiveDiagnosisFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,21 @@ import com.google.android.material.datepicker.CalendarConstraints
import com.google.android.material.datepicker.MaterialDatePicker
import com.google.android.material.textfield.TextInputLayout
import org.covidwatch.android.R
import org.covidwatch.android.data.NtpTime
import org.covidwatch.android.databinding.FragmentVerifyPositiveDiagnosisBinding
import org.covidwatch.android.extension.observe
import org.covidwatch.android.extension.observeEvent
import org.covidwatch.android.extension.observeNullableEvent
import org.covidwatch.android.extension.toInstant
import org.covidwatch.android.ui.BaseViewModelFragment
import org.koin.android.ext.android.inject
import org.koin.androidx.viewmodel.ext.android.stateViewModel
import java.time.LocalDate

class VerifyPositiveDiagnosisFragment :
BaseViewModelFragment<FragmentVerifyPositiveDiagnosisBinding, VerifyPositiveDiagnosisViewModel>() {

override val viewModel: VerifyPositiveDiagnosisViewModel by stateViewModel()
private val ntpTime: NtpTime by inject()

override fun bind(
inflater: LayoutInflater,
Expand Down Expand Up @@ -119,13 +121,13 @@ class VerifyPositiveDiagnosisFragment :
val builder = MaterialDatePicker.Builder.datePicker()
val constraints = CalendarConstraints.Builder()

val twoWeeksAgo = LocalDate.now().plusDays(-14).toInstant()
val twoWeeksAgo = ntpTime.nowAsLocalDate().plusDays(-14).toInstant()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a problem with this solution.
Even if the dates that can be selected are correct the date picker shows the current month from the device's settings.

I tried to use selection to current day when we open the dialog but it doesn't help.
@rasprague Could you try to investigate if that somehow possible to fix with MaterialDatePicker?

Copy link
Collaborator Author

@rasprague rasprague Sep 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a setOpenAt() to the date picker constraints fixes this

.toEpochMilli()

// Day in the past or 14 days back
val fromWhen = dayInPast ?: twoWeeksAgo

val untilNow = LocalDate.now().toInstant().toEpochMilli()
val untilNow = ntpTime.nowAsLocalDate().toInstant().toEpochMilli()

constraints.setValidator(BaseDateValidator { it in fromWhen..untilNow })

Expand Down