Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

implement sync interval every 24 hours #756

Merged
merged 3 commits into from
Jun 14, 2019
Merged

implement sync interval every 24 hours #756

merged 3 commits into from
Jun 14, 2019

Conversation

sashei
Copy link
Contributor

@sashei sashei commented Jun 12, 2019

Fixes #735
Connected to #754

Testing and Review Notes

Currently, manual sync resets the 24 hour timer, so if you would like to see an automatic sync, don't sync manually!

To Do

  • double check the original issue to confirm it is fully satisfied
  • add testing notes and screenshots in PR description to help guide reviewers
  • add unit tests
  • make sure CI builds are passing (e.g.: fix lint and other errors)

@sashei sashei requested a review from a team as a code owner June 12, 2019 21:19
Copy link
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

High quality code. Well done.

@@ -90,7 +89,7 @@ open class DataStore(
.subscribe { state ->
when (state) {
is State.Locked -> clearList()
is State.Unlocked -> sync()
is State.Unlocked -> syncIfRequired()
Copy link
Contributor

Choose a reason for hiding this comment

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

OMG AT LAST 😻

// this will be triggered when the system time is very low due to device restarts
val currentTimeTooEarly = (syncTimerDate - currentSystemTime) > Constant.Common.twentyFourHours

return syncTimerDate < currentSystemTime || currentTimeTooEarly
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand the name and comment currentTimeTooEarly.

syncTimerDate > currentSystemTime + twentyFourHours

Oh, so if syncTimerDate (which should never be less than twenty four hours after the current time) is 24h after the current time, then we've had something weird happen, e.g. a reboot.

So, suggest: renaming currentTimeTooEarly to followingDeviceReboot.

val followingDeviceReboot = syncTimerDate > currentSystemTime + twentyFourHours

Copy link
Contributor

Choose a reason for hiding this comment

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

Incidentally, I'd put 24 hours as a property.

open fun storeNextAutoLockTime() {
settingStore.autoLockTime
.take(1)
.subscribe(this::updateNextLockTime)
.addTo(compositeDisposable)
}

open fun storeNextSyncTime() {
storeSyncTimerDate(systemTimingSupport.systemTimeElapsed + Constant.Common.twentyFourHours)
Copy link
Contributor

Choose a reason for hiding this comment

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

Set Constant.Common.twentyFourHours as a property of TimingSupport.

e.g.

private val syncInterval = Constant.Common.twentyFourHours

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stuck it in the constants with the expectation that it will eventually become a setting that users can configure per #757 .

}
}

private fun lockCurrentlyRequired(): Boolean {
val currentSystemTime = lockingSupport.systemTimeElapsed
val currentSystemTime = systemTimingSupport.systemTimeElapsed
return if (currentSystemTime <= Constant.Common.sixtySeconds) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why Constant.Common.sixtySeconds is not in a property — isn't autoLockInterval settable via SharedPreferences?

@sashei sashei merged commit b57d8d4 into master Jun 14, 2019
@sashei sashei deleted the 735-sync-interval branch June 14, 2019 16:24
This was referenced Jun 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App syncs every time it's brought to the foreground
2 participants