-
-
Notifications
You must be signed in to change notification settings - Fork 48
Run form-entry Next navigation off the main thread #3667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
1b52471
08dc293
aa39848
f5aab51
2ec2e6e
a35839d
90530eb
21f76cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package org.commcare.activities | ||
|
|
||
| import androidx.lifecycle.LifecycleOwner | ||
| import androidx.lifecycle.lifecycleScope | ||
| import kotlinx.coroutines.Dispatchers | ||
| import kotlinx.coroutines.Job | ||
| import kotlinx.coroutines.launch | ||
| import kotlinx.coroutines.withContext | ||
|
|
||
| /** | ||
| * Runs form-navigation stepping on a background thread and delivers the | ||
| * resulting renderable event back on the main thread. | ||
| */ | ||
| class AsyncFormNavigator( | ||
| private val lifecycleOwner: LifecycleOwner, | ||
| private val stepWork: StepWork | ||
| ) { | ||
|
|
||
| fun interface StepWork { | ||
| fun step(): NavResult | ||
| } | ||
|
|
||
| fun interface StartCallback { | ||
| fun onStart() | ||
| } | ||
|
|
||
| fun interface ResultCallback { | ||
| fun onResult(result: NavResult) | ||
| } | ||
|
|
||
| private var job: Job? = null | ||
| private var currentNavId: Long = 0 | ||
|
|
||
| fun navigate(resuming: Boolean, onStart: StartCallback, onResult: ResultCallback) { | ||
| val navId = ++currentNavId | ||
| onStart.onStart() | ||
| job = lifecycleOwner.lifecycleScope.launch { | ||
| val result = withContext(Dispatchers.Default) { stepWork.step() } | ||
| if (navId != currentNavId) { | ||
| return@launch | ||
| } | ||
| onResult.onResult(result) | ||
| } | ||
| } | ||
|
|
||
| fun cancel() { | ||
| currentNavId++ | ||
| job?.cancel() | ||
| job = null | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,9 +81,12 @@ public class FormEntryActivityUIController implements CommCareActivityUIControll | |
| private BlockingActionsManager blockingActionsManager; | ||
|
|
||
| private boolean formRelevanciesUpdateInProgress = false; | ||
| private boolean navigationInFlight = false; | ||
|
|
||
| private static final String KEY_LAST_CHANGED_WIDGET = "index-of-last-changed-widget"; | ||
| private TextView finishText; | ||
| private FormLoadingOverlay formLoadingOverlay; | ||
| private AsyncFormNavigator asyncFormNavigator; | ||
|
|
||
| enum AnimationType { | ||
| LEFT, RIGHT, FADE | ||
|
|
@@ -144,6 +147,13 @@ public void setupUI() { | |
|
|
||
|
|
||
| mViewPane = activity.findViewById(R.id.form_entry_pane); | ||
| formLoadingOverlay = new FormLoadingOverlay( | ||
| activity.findViewById(R.id.form_entry_loading_overlay), | ||
| activity.findViewById(R.id.form_entry_loading_label), | ||
| Localization.get("form.entry.loading.next.question")); | ||
| asyncFormNavigator = new AsyncFormNavigator( | ||
| activity, | ||
| this::stepToRenderableEvent); | ||
|
|
||
| activity.requestMajorLayoutUpdates(); | ||
|
|
||
|
|
@@ -417,71 +427,90 @@ private void showNextView(boolean resuming) { | |
| // Any info stored about the last changed widget is useless when we move to a new view | ||
| resetLastChangedWidget(); | ||
|
|
||
| if (FormEntryActivity.mFormController.getEvent() != FormEntryController.EVENT_END_OF_FORM) { | ||
| int event; | ||
| if (FormEntryActivity.mFormController.getEvent() == FormEntryController.EVENT_END_OF_FORM) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| group_skip: | ||
| do { | ||
| event = FormEntryActivity.mFormController.stepToNextEvent(FormEntryController.STEP_OVER_GROUP); | ||
| switch (event) { | ||
| case FormEntryController.EVENT_QUESTION: | ||
| QuestionsView next = createView(); | ||
| if (!resuming) { | ||
| showView(next, LocalePreferences.isLocaleRTL() ? AnimationType.LEFT : AnimationType.RIGHT); | ||
| } else { | ||
| showView(next, AnimationType.FADE, false); | ||
| } | ||
| break group_skip; | ||
| case FormEntryController.EVENT_END_OF_FORM: | ||
| // auto-advance questions might advance past the last form quesion | ||
| // In special case when questionsView is null (there is no question), | ||
| // to avoid exit dialog without saving option, shown from refreshCurrentView(), | ||
| // save and exit method called. | ||
| if (questionsView != null) { | ||
| refreshCurrentView(); | ||
| } else { | ||
| activity.triggerUserFormComplete(); | ||
| } | ||
| break group_skip; | ||
| case FormEntryController.EVENT_PROMPT_NEW_REPEAT: | ||
| createRepeatDialog(); | ||
| break group_skip; | ||
| case FormEntryController.EVENT_GROUP: | ||
| //We only hit this event if we're at the _opening_ of a field | ||
| //list, so it seems totally fine to do it this way, technically | ||
| //though this should test whether the index is the field list | ||
| //host. | ||
| if (FormEntryActivity.mFormController.indexIsInFieldList() | ||
| && FormEntryActivity.mFormController.getQuestionPrompts().length != 0) { | ||
| QuestionsView nextGroupView = createView(); | ||
| if (!resuming) { | ||
| showView(nextGroupView, LocalePreferences.isLocaleRTL() ? AnimationType.LEFT : AnimationType.RIGHT); | ||
| } else { | ||
| showView(nextGroupView, AnimationType.FADE, false); | ||
| } | ||
| break group_skip; | ||
| } | ||
| // otherwise it's not a field-list group, so just skip it | ||
| break; | ||
| case FormEntryController.EVENT_REPEAT: | ||
| Log.i(TAG, "repeat: " + FormEntryActivity.mFormController.getFormIndex().getReference()); | ||
| // skip repeats | ||
| break; | ||
| case FormEntryController.EVENT_REPEAT_JUNCTURE: | ||
| Log.i(TAG, "repeat juncture: " | ||
| + FormEntryActivity.mFormController.getFormIndex().getReference()); | ||
| // skip repeat junctures until we implement them | ||
| break; | ||
| default: | ||
| Log.w(TAG, | ||
| "JavaRosa added a new EVENT type and didn't tell us... shame on them."); | ||
| break; | ||
| } | ||
| } while (event != FormEntryController.EVENT_END_OF_FORM); | ||
| } catch (XPathException e) { | ||
| new UserfacingErrorHandling<>().logErrorAndShowDialog(activity, e, FormEntryConstants.EXIT); | ||
| navigationInFlight = true; | ||
| asyncFormNavigator.navigate( | ||
| resuming, | ||
| formLoadingOverlay::show, | ||
| result -> { | ||
| navigationInFlight = false; | ||
| formLoadingOverlay.hide(); | ||
| renderNavResult(result, resuming); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Walks the form forward via {@link FormEntryController#stepToNextEvent} until a renderable | ||
| * event is reached. Pure controller-state access; no view creation or UI side effects. | ||
| */ | ||
| private NavResult stepToRenderableEvent() { | ||
| try { | ||
| while (true) { | ||
| int event = FormEntryActivity.mFormController.stepToNextEvent(FormEntryController.STEP_OVER_GROUP); | ||
| switch (event) { | ||
| case FormEntryController.EVENT_QUESTION: | ||
| return NavResult.Question.INSTANCE; | ||
| case FormEntryController.EVENT_END_OF_FORM: | ||
| return NavResult.EndOfForm.INSTANCE; | ||
| case FormEntryController.EVENT_PROMPT_NEW_REPEAT: | ||
| return NavResult.PromptNewRepeat.INSTANCE; | ||
| case FormEntryController.EVENT_GROUP: | ||
| //We only hit this event if we're at the _opening_ of a field | ||
| //list, so it seems totally fine to do it this way, technically | ||
| //though this should test whether the index is the field list | ||
| //host. | ||
| if (FormEntryActivity.mFormController.indexIsInFieldList() | ||
| && FormEntryActivity.mFormController.getQuestionPrompts().length != 0) { | ||
| return NavResult.FieldListGroup.INSTANCE; | ||
| } | ||
| // otherwise it's not a field-list group, so just skip it | ||
| break; | ||
| case FormEntryController.EVENT_REPEAT: | ||
| Log.i(TAG, "repeat: " + FormEntryActivity.mFormController.getFormIndex().getReference()); | ||
| // skip repeats | ||
| break; | ||
| case FormEntryController.EVENT_REPEAT_JUNCTURE: | ||
| Log.i(TAG, "repeat juncture: " | ||
| + FormEntryActivity.mFormController.getFormIndex().getReference()); | ||
| // skip repeat junctures until we implement them | ||
| break; | ||
| default: | ||
| Log.w(TAG, | ||
| "JavaRosa added a new EVENT type and didn't tell us... shame on them."); | ||
| break; | ||
| } | ||
| } | ||
| } catch (XPathException e) { | ||
| return new NavResult.Error(e); | ||
| } | ||
| } | ||
|
Comment on lines
+449
to
+489
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only
Consider widening the catch to 🤖 Prompt for AI Agents |
||
|
|
||
| private void renderNavResult(NavResult result, boolean resuming) { | ||
| if (result instanceof NavResult.Question || result instanceof NavResult.FieldListGroup) { | ||
| QuestionsView next = createView(); | ||
| if (!resuming) { | ||
| showView(next, LocalePreferences.isLocaleRTL() ? AnimationType.LEFT : AnimationType.RIGHT); | ||
| } else { | ||
| showView(next, AnimationType.FADE, false); | ||
| } | ||
| } else if (result instanceof NavResult.PromptNewRepeat) { | ||
| createRepeatDialog(); | ||
| } else if (result instanceof NavResult.EndOfForm) { | ||
| // auto-advance questions might advance past the last form question | ||
| // In special case when questionsView is null (there is no question), | ||
| // to avoid exit dialog without saving option, shown from refreshCurrentView(), | ||
| // save and exit method called. | ||
| if (questionsView != null) { | ||
| refreshCurrentView(); | ||
| } else { | ||
| activity.triggerUserFormComplete(); | ||
| } | ||
| } else if (result instanceof NavResult.Error) { | ||
| new UserfacingErrorHandling<>().logErrorAndShowDialog( | ||
| activity, ((NavResult.Error)result).getException(), FormEntryConstants.EXIT); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -598,7 +627,16 @@ protected void next() { | |
| } | ||
|
|
||
| protected boolean shouldIgnoreNavigationAction() { | ||
| return blockingActionsManager.isBlocked(); | ||
| return blockingActionsManager.isBlocked() || navigationInFlight; | ||
| } | ||
|
|
||
| protected void cancelNavigation() { | ||
| if (!navigationInFlight) { | ||
| return; | ||
| } | ||
| asyncFormNavigator.cancel(); | ||
| navigationInFlight = false; | ||
| formLoadingOverlay.hide(); | ||
| } | ||
|
|
||
| protected boolean shouldIgnoreSwipeAction() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| package org.commcare.activities | ||
|
|
||
| import android.os.Handler | ||
| import android.os.Looper | ||
| import android.view.View | ||
| import android.widget.TextView | ||
|
|
||
| /** | ||
| * Wraps the form-entry loading overlay view. [show] posts a delayed | ||
| * visibility change so fast navigations don't flash the overlay; [hide] | ||
| * cancels any pending show and hides the view immediately. | ||
| */ | ||
| class FormLoadingOverlay @JvmOverloads constructor( | ||
| private val overlayView: View, | ||
| labelView: TextView, | ||
| labelText: String, | ||
| private val showDelayMillis: Long = DEFAULT_SHOW_DELAY_MILLIS | ||
| ) { | ||
|
|
||
| private val handler = Handler(Looper.getMainLooper()) | ||
| private val showRunnable = Runnable { overlayView.visibility = View.VISIBLE } | ||
|
|
||
| init { | ||
| labelView.text = labelText | ||
| } | ||
|
|
||
| fun show() { | ||
| handler.removeCallbacks(showRunnable) | ||
| handler.postDelayed(showRunnable, showDelayMillis) | ||
| } | ||
|
|
||
| fun hide() { | ||
| handler.removeCallbacks(showRunnable) | ||
| overlayView.visibility = View.GONE | ||
| } | ||
|
|
||
| companion object { | ||
| const val DEFAULT_SHOW_DELAY_MILLIS: Long = 150 | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| package org.commcare.activities | ||
|
|
||
| import org.javarosa.xpath.XPathException | ||
|
|
||
| /** | ||
| * Outcome of stepping the form to the next renderable event. | ||
| */ | ||
| sealed class NavResult { | ||
| object Question : NavResult() | ||
| object FieldListGroup : NavResult() | ||
| object PromptNewRepeat : NavResult() | ||
| object EndOfForm : NavResult() | ||
| data class Error(val exception: XPathException) : NavResult() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the AsyncFormNavigator.kt file fd -t f AsyncFormNavigator.ktRepository: dimagi/commcare-android
Length of output: 120
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 1723
🏁 Script executed:
# Check the entire class to understand the context better wc -l app/src/org/commcare/activities/AsyncFormNavigator.ktRepository: dimagi/commcare-android
Length of output: 123
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 146
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 550
🏁 Script executed:
# Look for FormEntryActivity and how cancelNavigation is called fd -t f FormEntryActivity.ktRepository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 269
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 127
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 92
🏁 Script executed:
# Get the full diff for AsyncFormNavigator to see the context git diff HEAD~1 -- app/src/org/commcare/activities/AsyncFormNavigator.ktRepository: dimagi/commcare-android
Length of output: 97
🏁 Script executed:
# Check the commit details git show --stat 21f76cc29Repository: dimagi/commcare-android
Length of output: 50380
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 253
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 2905
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 2778
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 5293
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 1545
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 234
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 6475
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 143
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
Repository: dimagi/commcare-android
Length of output: 1190
cancel()is not effective for blockingstepWork.step()and leaves a window for stale results.Two related issues:
stepWork.step()runs viawithContext(Dispatchers.Default)but is pure blocking Java with no cooperative cancellation checks.job?.cancel()will only preventonResultfrom firing (already guarded bynavId), it will not stop theFormEntryControllermutations on the background thread. Callers that assume a synchronous post-cancel state (e.g.,FormEntryActivity.onPause()→cancelNavigation()→saveAnswersForCurrentScreen) will race with the still-running step on the shared, non-thread-safe controller.lifecycleScope.launchdispatches asynchronously even for the main body; betweenonStart.onStart()and entering the coroutine, anothernavigate()can be invoked, potentially overlapping work. Consider whether invocations must be serialized.At minimum, consider
job?.cancelAndJoin()from arunBlocking/suspend context at cancel time, or add anAtomicBoolean"navigating" guard and block newnavigatecalls until the previous step returns. Also consider capturingjobbeforelaunchreturns in a way that guaranteescancel()observes the latest job.🤖 Prompt for AI Agents