[MS-939] Sync UI/UX update#1282
Conversation
…ed values needed for new UI/UX
…ng down-sync when needed for new UI/UX
…ons needed for new UI/UX
…odel of the new sync UI/UX. These follow the idea of the functions in the Kotlin's standard library.
alex-vt
requested review from
BurningAXE,
alexandr-simprints,
luhmirin-s and
meladRaouf
| syncInfo: SyncInfo, | ||
| config: SyncInfoFragmentConfig, | ||
| ) { | ||
| // note: ".isGone = not" is preferred to ".isVisible =" below for non-ambiguity of the no-show state |
There was a problem hiding this comment.
".isGone = not" is preferred
This is not true if you have to reverse all of the conditions. isVisible behaviour is how the Android framework has handled view visibility from the very beginning; it is not ambiguous.
There was a problem hiding this comment.
Some of the places where you do not have to reverse the boolean are valid cases for isGone, tho.
There was a problem hiding this comment.
At this point and context, the current solution is adequate.
There was a problem hiding this comment.
Let's agree to disagree on this one, it has moved into nitpick territory :D
| ?.takeIf { | ||
| timeNowMs - cachedEventCountToDownloadTimestamp < COUNT_EVENTS_CACHE_LIFESPAN_MILLIS | ||
| }?.let { | ||
| return it |
There was a problem hiding this comment.
IIRC, there was some news of local returns coming in one of the upcoming Kotlin versions, and this could bite us. I would suggest rewriting to avoid global returns within scope functions:
if (
cachedEventCountToDownload != null &&
timeNowMs - cachedEventCountToDownloadTimestamp < COUNT_EVENTS_CACHE_LIFESPAN_MILLIS
) {
return cachedEventCountToDownload
}
There was a problem hiding this comment.
Smart cast won't work here.
val currentlyCachedEventCountToDownload = cachedEventCountToDownload
if (
currentlyCachedEventCountToDownload != null &&
timeNowMs - cachedEventCountToDownloadTimestamp < COUNT_EVENTS_CACHE_LIFESPAN_MILLIS
) {
return currentlyCachedEventCountToDownload
}is quite verbose. I'd suggest to keep using the language as it currently allows, and we can migrate if the future changes actually arrive. Anyway return@let it trips the unit test, so we are protected in this particular potential case.
There was a problem hiding this comment.
Smart cast won't work here.
You can always do !!, cases like this (right after null check) are probably the only safe time to use that operator.
Anyway return@let it trips the unit test,
Because return@let only returns from the lambda, while plain return stops the whole function.
we can migrate if the future changes actually arrive
I was initially worried about it because it seemed that we might not easily spot the change, and it would cause a subtle bug in the future. I have reviewed the upcoming changes, and it seems that I was actually thinking about the non-local break/continue feature, which does not affect the returns. So while I don't think that this is a good use of the let{} function, there is no risk of that bug creeping in.
luhmirin-s
left a comment
luhmirin-s
left a comment
There was a problem hiding this comment.
Noticed a couple more potential improvements.
| pb: ProgressBar, | ||
| private fun renderProgress( | ||
| progress: SyncInfoProgress, | ||
| progressBar: com.google.android.material.progressindicator.LinearProgressIndicator, |
| import kotlinx.coroutines.flow.Flow | ||
|
|
||
| @Keep | ||
| interface Timer { |
There was a problem hiding this comment.
Not sure if this is the correct name since it does not time anything. Ticker would make more sense.
| val sampleReferences = localDataSource | ||
| .listImages(projectId) | ||
| var sampleIndex = 0 | ||
| val sampleReferenceBatches = sampleReferences |
There was a problem hiding this comment.
This might be splitting hairs a bit, but I think keeping the sampleReferences just for the size unintentionally doubles the memory footprint.
While the operations are the same, in the "before" version, the full list was freed for GC after it had been split into batches; in the "after" version, it is kept in memory next to the batched sub-lists.
I would recommend caching only the size in a "side-effect" call instead:
var sampleIndex = 0
var samplesSize = 0
val sampleReferenceBatches = localDataSource
.listImages(projectId)
.also { samplesSize = it.size }
.chunked(batchSize)
| fun getMillisSinceLastImageSync(): Long? = securePrefs | ||
| .getLong(IMAGE_SYNC_COMPLETION_TIME_MILLIS, 0) | ||
| .takeIf { | ||
| securePrefs.contains(IMAGE_SYNC_COMPLETION_TIME_MILLIS) |
There was a problem hiding this comment.
Could we use getLong(IMAGE_SYNC_COMPLETION_TIME_MILLIS, -1) as a "no-value" marker and avoid doing another read?
Or it would make more sense to do contains first:
fun getMillisSinceLastImageSync(): Long? = if (securePrefs.contains(IMAGE_SYNC_COMPLETION_TIME_MILLIS)) {
timeHelper.now().ms - securePrefs.getLong(IMAGE_SYNC_COMPLETION_TIME_MILLIS, 0)
} else {
null
}
luhmirin-s
left a comment
luhmirin-s
left a comment
There was a problem hiding this comment.
There are a couple of things that we should address for the sake of maintainability in the future, but none of those are blocking in my opinion.
I would suggest waiting for at least 1-2 additional approvals before merging, given the scale of the changes.
| import kotlinx.coroutines.flow.map | ||
| import kotlinx.coroutines.flow.scan | ||
|
|
||
| fun <T1, T2, T3, T4, T5, T6, T7, T8, T9, R> combine9( |
There was a problem hiding this comment.
This is getting out of hand. Can you add a TODO comment/ticket to review ways to simplify this in future? Potentially, by doing some nested combining of parts of the state instead.
There was a problem hiding this comment.
The standard "combine" doesn't go all the way to 9 args that we need in this rather complex case of interconnected UI that updates depending on 9 inputs. "combine9" is still aimed to have a similar, easy to use signature: array manipulation is hidden from the domain-specific use case. It is well worth having this, rather than falling back all the way to the procedural style of UI calculation. Added tickets for these.
There was a problem hiding this comment.
As I understand, it doesn't go over 5 because it becomes harder to follow/read. While there is nothing inherently wrong with combining 9 flows in one large function, there should be a way to split it into several smaller (more focused) sub-state combinations purely for maintainability.
| } | ||
| } | ||
|
|
||
| private fun Flow<List<WorkInfo>>.associateWithIfSyncing() = transformLatest { workInfos -> |
There was a problem hiding this comment.
I would suggest adding at least a TODO with a detailed description of the issues that this "pulse" fixes, and attempting to fix it in a separate PR.
| } | ||
|
|
||
| @Test | ||
| fun `should disable sync button when CommCare permission is missing`() = runTest { |
There was a problem hiding this comment.
Should the sync button be shown if CommCare permission is missing but device is online and can upsync?
There was a problem hiding this comment.
Makes sense! Updated the sync availability logic.
…but CoSync up-sync possible
| } | ||
|
|
||
| @Test | ||
| fun `sync button should be disabled when this is logout screen and no sync to Simprints`() = runTest { |
There was a problem hiding this comment.
Is this situation possible in practice? Shouldn't SID proceed to do logout automatically?
There was a problem hiding this comment.
The automatic logout isn't instant so the UI is calculated for it as well. But indeed, this is superseded by the sync button being hidden altogether, because logout sync is automatic in most cases.
| } | ||
|
|
||
| @Test | ||
| fun `sync button should be disabled when there is neither Simprints nor ComCare down-sync`() = runTest { |
There was a problem hiding this comment.
Currently not a valid configuration but doesn't hurt.
| } | ||
|
|
||
| @Test | ||
| fun `sync button should be disabled when only CommCare down-sync allowed but there is CoSync permission error`() = runTest { |
There was a problem hiding this comment.
CommCare permission error
BurningAXE
left a comment
BurningAXE
left a comment
There was a problem hiding this comment.
Great job with this monster of a PR!
No more comments from me, think all is good now. That said - we should E2E test all the various configuration and state combinations on different screens as it's too complex to just reason about the code statically and be confident all conditions are covered correctly.
|
JIRA ticket
Will be released in: 2025.3.0
Notable changes
Example of Sync Information in Settings
Example of sync card on Dashboard
Example of sync card at Logout
Additional work checklist