feat: add ViewModel progress infrastructure [DEV]#20729
feat: add ViewModel progress infrastructure [DEV]#20729criticalAY wants to merge 1 commit intoankidroid:mainfrom
Conversation
b6fabdb to
c278581
Compare
david-allison
left a comment
There was a problem hiding this comment.
This looks exceptional. One question on the race condition
c278581 to
d97aee6
Compare
Introduce a common pattern for progress notifications in ViewModels, decoupling progress dialog management from Activity/Fragment context.
d97aee6 to
97cb3d7
Compare
| /** The cancel callback for the currently active cancellable operation, if any */ | ||
| @Volatile | ||
| private var currentOnCancel: (() -> Unit)? = null |
There was a problem hiding this comment.
I feel we should store each onCancel.
This would require arbitrary access to the list of currently active methods, as they are removed based on the completion time.
| val mutex = Mutex() | ||
| var showJob: Job? = null | ||
| var dialogVisible = false | ||
| viewModel.progressManager.progress.collect { state -> |
There was a problem hiding this comment.
Consider using launchCollectionInLifecycleScope
| repeatOnLifecycle(Lifecycle.State.STARTED) { | ||
| val mutex = Mutex() | ||
| var showJob: Job? = null | ||
| var dialogVisible = false |
There was a problem hiding this comment.
Sorry... I was incorrect here. You need a lock when updating the progress state, but this isn't an update method.
There was a problem hiding this comment.
dialogVisible doesn't restore state from the FragmentManager
| activeCount.incrementAndGet() | ||
| if (onCancel != null) { | ||
| currentOnCancel = onCancel | ||
| } | ||
| updateState(message = message, amount = null, cancellable = onCancel != null) | ||
| try { | ||
| return ProgressScope(this).block() | ||
| } finally { | ||
| if (onCancel != null) { | ||
| currentOnCancel = null | ||
| } | ||
| if (activeCount.decrementAndGet() == 0) { | ||
| progress.value = ViewModelProgress.Idle | ||
| } | ||
| } |
There was a problem hiding this comment.
Concurrency bugs here: There's dependencies between currentOnCancel, activeCount and progress, but locks aren't held to serialise the mutations.
|
Thinking more: What's does progress/cancellation mean if there are two concurrent progress operations. Let's define this explicitly, both the UI and UX. |
Purpose / Description
Introduce a common pattern for progress notifications in ViewModels, decoupling progress dialog management from Activity/Fragment context.
Fixes
withProgressin ViewModels #19460Approach
How Has This Been Tested?
Unit test
Learning (optional, can help others)
NA
Checklist
Please, go through these checks before submitting the PR.