integrate kronos-android ntp clock library into project #150

rasprague · 2020-09-02T17:17:04Z

integrate ntp clock library into app, make "Share a Positive Diagnosis" date picker use ntp time

add Kronos ntp clock lib change DiagnosisKeysToken to use ntp time

make app context injectable, make NTPTime use it removed app context access hack from BaseCovidWatchApplicaiton

rasprague · 2020-09-02T17:22:13Z

asana task https://app.asana.com/0/1188301658055194/1188820238689794/f

Apisov

It's nice start but according to README from https://github.com/lyft/Kronos-Android
we need to init this lib in the onCreate of the Application class. So when the wrappers is done we would need to inject it in the Application class to call syncInBackground method.

Also, to fix the problem we need to limit the date pickers on the verification screen to real dates.

So we would to rework it here:

covidwatch-android-en/app/src/main/java/org/covidwatch/android/ui/reporting/VerifyPositiveDiagnosisFragment.kt

Lines 122 to 128 in 21cb2a7

    
           val twoWeeksAgo = LocalDate.now().plusDays(-14).toInstant() 
        
               .toEpochMilli() 
        
           // Day in the past or 14 days back 
        
           val fromWhen = dayInPast ?: twoWeeksAgo 
        
           val untilNow = LocalDate.now().toInstant().toEpochMilli()

Apisov · 2020-09-02T19:30:27Z

app/src/main/java/org/covidwatch/android/data/NTPTime.kt

+import org.koin.core.KoinComponent
+import org.koin.core.inject
+
+class NTPTime {


A separate wrapper is nice but having a static reference to a context is a bad thing that leads to memory leaks.

In Koin there is androidApplication() that returns applicationContext, have a look at any wrapper that use androidApplication() in appModule. For example Notifications -- we need something like that and then we will be injecting NtpTime anywhere we need it.

Also, to make it clear -- I suggest to rename this class to NtpTime because of the guidelines

removed static reference to context, changed so it's passed into the constructor and never stored

renamed file

Made NtpClock injectable. Injected into BaseCovidWatchApplication and started properly from onCreate()

modified VerifyPositiveDiagnosisFragment to use NtpTime for date pickers

Apisov · 2020-09-02T19:36:37Z

app/src/main/java/org/covidwatch/android/data/diagnosiskeystoken/DiagnosisKeysToken.kt

 import org.covidwatch.android.data.converter.ExposureConfigurationConverter

 @Entity(tableName = "diagnosis_keys_token")
 @TypeConverters(ExposureConfigurationConverter::class)
 data class DiagnosisKeysToken(
    @PrimaryKey val token: String,
    val exposureConfiguration: CovidExposureConfiguration,
-    val providedTime: Long = System.currentTimeMillis(),
+    val providedTime: Long = NTPTime.currentTimeMillis(),


We would to remove default value for this property and use injected NtpTime to use a proper time. But having here a proper world time is not critical, it's not even used anywhere for now.

So we could leave the System time here because this class might be deleted in the near future when we update EN version.

change reverted back to System call

Apisov · 2020-09-02T19:38:01Z

app/src/main/java/org/covidwatch/android/di/AppContext.kt

+import android.content.ContextWrapper
+
+// exists solely to make the Android applicationContext injectable
+class AppContext(context : Context) : ContextWrapper(context) {


Remove it please since we won't use the implementation of NTPTime with companion object.

file removed

…ment to use NtpTime for date pickers refactor so NtpTime is injectable, change VerifyPositiveDiagnosisFragment to use NtpTime for date pickers remove AppContext class properly start ntpTime in Application onCreate()

remove unused function from NtpTime

remove unused import

removed trailing newline (the only change to this file) to remove from this commit

rasprague · 2020-09-02T21:50:53Z

app/src/main/java/org/covidwatch/android/data/diagnosiskeystoken/DiagnosisKeysToken.kt

looks like the github editor enforces the final newline haha

remove DiagnonsisKeysToken.kt from commit

Apisov

Thanks for changes now it's better but still some issues need to be resolved.

I double checked the library and it's quite old but the version of it still still super alpha which suggest Lyft doesn't really support it. There are a few issues in their repo but we don't really need a high accuracy from the lib -- we need good enough to sync the days not minutes.
Another old lib that I remember is https://github.com/instacart/truetime-android
but it looks even more abandoned so no big difference I guess.

Also, I realized that introducing NTP time, in the ideal case, would need more changes than here...
We probably need to change most of Instant.now() or any now() requests but this is too big changes that would need to replace all java.Time API. We need to think on this before changing everything.

Apisov · 2020-09-04T09:06:42Z

app/build.gradle

@@ -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"


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

dependency line moved

Apisov · 2020-09-04T09:11:13Z

app/src/main/java/org/covidwatch/android/data/NtpTime.kt

+    fun syncInBackground() = kronosClock.syncInBackground()
+
+    fun nowAsLocalDate() : LocalDate =
+        Instant.ofEpochMilli(currentTimeMillis()).atZone(ZoneId.systemDefault()).toLocalDate()


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

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

Apisov · 2020-09-04T09:13:42Z

app/src/main/java/org/covidwatch/android/ui/reporting/VerifyPositiveDiagnosisFragment.kt

@@ -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()


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?

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

…nstant removed "LocalTime" from NtpTime add plus/minus days extenstion to Instant

force date picker to open on today's month

Apisov

Almost done, just a few suggestions. But the concern about this PR is still valid -- should we use this NTP adjusted time across the whole app or only in the date pickers.

Apisov · 2020-09-07T19:12:35Z

app/build.gradle

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

Apisov · 2020-09-07T19:22:30Z

app/src/main/java/org/covidwatch/android/extension/Instant.kt

+
+import java.time.Instant
+
+// 1d * (24h/d) * (60m/h) * (60s/m)


Creating extensions is nice but why then don't use existing API inside of them?
Like this:

minus(15, ChronoUnit.DAYS)

add(Duration.ofDays(15))

Apisov · 2020-09-07T19:22:58Z

app/src/main/java/org/covidwatch/android/ui/reporting/VerifyPositiveDiagnosisFragment.kt

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


Apisov · 2020-09-07T19:32:41Z

app/src/main/java/org/covidwatch/android/ui/reporting/VerifyPositiveDiagnosisFragment.kt


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

-        val untilNow = LocalDate.now().toInstant().toEpochMilli()
+        val untilNow = ntpTime.nowAsInstant().toEpochMilli()


It looks like the conversion is redundant because we use the milliseconds anyway. We could make NtpTime.currentTimeMillis public and use it directly here.

calling currentTimeMillis() directly

minor changes / cleanup

rasprague added 2 commits September 1, 2020 19:33

add Kronos ntp clock lib

fe9df02

add Kronos ntp clock lib change DiagnosisKeysToken to use ntp time

make app context injectable, make NTPTime use it

bcc9950

make app context injectable, make NTPTime use it removed app context access hack from BaseCovidWatchApplicaiton

Apisov requested changes Sep 2, 2020

View reviewed changes

rasprague added 4 commits September 2, 2020 14:28

remove unused function from NtpTime

d04e542

remove unused function from NtpTime

remove unused import

385e199

remove unused import

remove minor change to DiagnosisKeysToken.kt to remove from this commit

fdff161

removed trailing newline (the only change to this file) to remove from this commit

rasprague commented Sep 2, 2020

View reviewed changes

remove DiagnonsisKeysToken.kt from commit

2f31082

remove DiagnonsisKeysToken.kt from commit

Apisov requested changes Sep 4, 2020

View reviewed changes

rasprague added 2 commits September 4, 2020 12:48

removed "LocalTime" from NtpTime, add plus/minus days extenstion to I…

1a90ab6

…nstant removed "LocalTime" from NtpTime add plus/minus days extenstion to Instant

force date picker to open on today's month

f2f9b99

force date picker to open on today's month

Apisov requested changes Sep 7, 2020

View reviewed changes

minor changes / cleanup

e626be8

minor changes / cleanup

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrate kronos-android ntp clock library into project #150

integrate kronos-android ntp clock library into project #150

rasprague commented Sep 2, 2020 •

edited

Loading

rasprague commented Sep 2, 2020

Apisov left a comment

Apisov Sep 2, 2020

Apisov Sep 2, 2020

rasprague Sep 2, 2020

rasprague Sep 2, 2020

rasprague Sep 2, 2020

Apisov Sep 2, 2020

rasprague Sep 2, 2020

Apisov Sep 2, 2020

rasprague Sep 2, 2020

rasprague Sep 2, 2020

Apisov left a comment

Apisov Sep 4, 2020

rasprague Sep 4, 2020

Apisov Sep 4, 2020

rasprague Sep 4, 2020 •

edited

Loading

Apisov Sep 4, 2020

rasprague Sep 4, 2020 •

edited

Loading

Apisov left a comment

Apisov Sep 7, 2020

rasprague Sep 7, 2020

Apisov Sep 7, 2020

rasprague Sep 7, 2020

Apisov Sep 7, 2020

Apisov Sep 7, 2020

rasprague Sep 7, 2020

	val twoWeeksAgo = LocalDate.now().plusDays(-14).toInstant()
	.toEpochMilli()

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

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

integrate kronos-android ntp clock library into project #150

Are you sure you want to change the base?

integrate kronos-android ntp clock library into project #150

Conversation

rasprague commented Sep 2, 2020 • edited Loading

rasprague commented Sep 2, 2020

Apisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Apisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasprague Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasprague Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

Apisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rasprague commented Sep 2, 2020 •

edited

Loading

rasprague Sep 4, 2020 •

edited

Loading

rasprague Sep 4, 2020 •

edited

Loading