Skip to content

feat: add ViewModel progress infrastructure [DEV]#20729

Open
criticalAY wants to merge 1 commit intoankidroid:mainfrom
criticalAY:feat/withprogress
Open

feat: add ViewModel progress infrastructure [DEV]#20729
criticalAY wants to merge 1 commit intoankidroid:mainfrom
criticalAY:feat/withprogress

Conversation

@criticalAY
Copy link
Copy Markdown
Contributor

@criticalAY criticalAY commented Apr 12, 2026

Purpose / Description

Introduce a common pattern for progress notifications in ViewModels, decoupling progress dialog management from Activity/Fragment context.

Fixes

Approach

  • The ViewModel exposes a state flow. The Activity/Fragment calls observeProgress(viewModel) once, and the LoadingDialogFragment is shown/dismissed automatically based on state changes.
  • Cancellation follows the same pattern as the old API (pass an onCancel callback to withProgress). The observer wires it to the dialog's cancel listener. (I actually followed a different pattern here then I saw David's comment on original issue)

How Has This Been Tested?

Unit test

Learning (optional, can help others)

NA

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@criticalAY criticalAY force-pushed the feat/withprogress branch 2 times, most recently from b6fabdb to c278581 Compare April 12, 2026 14:47
Copy link
Copy Markdown
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

This looks exceptional. One question on the race condition

Comment thread AnkiDroid/src/main/java/com/ichi2/anki/progress/ProgressManager.kt Outdated
Comment thread AnkiDroid/src/main/java/com/ichi2/anki/progress/ProgressObserver.kt Outdated
Comment thread AnkiDroid/src/main/java/com/ichi2/anki/progress/ProgressObserver.kt Outdated
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Apr 15, 2026
@criticalAY criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Apr 15, 2026
@criticalAY criticalAY added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Apr 17, 2026
Introduce a common pattern for progress notifications in ViewModels,
decoupling progress dialog management from Activity/Fragment context.
@criticalAY criticalAY added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Apr 18, 2026
Comment on lines +37 to +39
/** The cancel callback for the currently active cancellable operation, if any */
@Volatile
private var currentOnCancel: (() -> Unit)? = null
Copy link
Copy Markdown
Member

@david-allison david-allison Apr 19, 2026

Choose a reason for hiding this comment

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

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 ->
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider using launchCollectionInLifecycleScope

repeatOnLifecycle(Lifecycle.State.STARTED) {
val mutex = Mutex()
var showJob: Job? = null
var dialogVisible = false
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry... I was incorrect here. You need a lock when updating the progress state, but this isn't an update method.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

dialogVisible doesn't restore state from the FragmentManager

Comment on lines +56 to +70
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
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Concurrency bugs here: There's dependencies between currentOnCancel, activeCount and progress, but locks aren't held to serialise the mutations.

@david-allison
Copy link
Copy Markdown
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants