Skip to content
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

Prevent crashes in form filling when restoring process #5602

Merged
merged 8 commits into from Jun 16, 2023

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented May 22, 2023

Closes #5240

As described in the issue:

For the moment, we're going to avoid restoring view state when we do a process restore

What has been done to verify that this works as intended?

New tests.

Why is this the best possible solution? Were any other approaches considered?

There's probably a bunch of other tricks we could use here, but I'm pretty happy with what I've gone for: write a UUID out to the outState and to application level state during onSaveInstanceState, and then compare later to determine whether the Activity is being recreated within the same or a new process.

I'm feeling pretty unhappy with some of the code required to test this, but I've left a comment about that inline.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

As well as checking the issue is resolved, it'd be good to test filling/editing and resuming (from save points) forms, forms with audit logs and the form hierarchy.

Before submitting this PR, please make sure you have:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@@ -418,7 +423,14 @@ public void onCreate(Bundle savedInstanceState) {
.forClass(SelectOneFromMapDialogFragment.class, () -> new SelectOneFromMapDialogFragment(viewModelFactory))
.build());

savedInstanceState = savedInstanceStateProvider.getState(savedInstanceState);
Copy link
Member Author

@seadowg seadowg May 24, 2023

Choose a reason for hiding this comment

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

I don't feel great about this solution of forcing in a provider for savedInstanceState, but there's no existing way to start an Activity with previous state using ActivityScenario, and this is the best I could come up with (without forking AndroidX Test).

It currently is possible to write pure Robolectric code that that creates an Acivity with previous state so it should be possible to add that to ActivityScenario for "local" tests (that under the hood use the Robolectric backed LocalActivityInvoker), but I don't have enough knowledge of Android internals to know if it'd be doable for instrumentation tests. I'll file an issue over at AndroidX Test for this (and update this comment with a link).

Copy link
Member Author

Choose a reason for hiding this comment

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

Link to issue: android/android-test#1825.

@seadowg seadowg marked this pull request as ready for review May 24, 2023 13:48
@seadowg seadowg requested a review from grzesiek2010 May 24, 2023 13:48
@srujner
Copy link

srujner commented Jun 16, 2023

@dbemke Tested this PR earlier this week.

Tested with Success!

Verified on device with Android: 10,12,13

Verified Cases:

  • Issue Crash when restoring to some form entry screens #5240 is no longer reproducing;
  • Sending finalized Forms, Editing saved Forms, Viewing sent Forms, Deleting saved Forms;
  • Restoring Forms when Editing, Viewing, Sending, in Hierarchy View;
  • Regression checks in Audit file;
  • Regression check in Hierarchy View;
  • In device settings - no background processes;
  • Don’t keep activities enabled/disabled;
  • Minimize the app and rotate the screen;
  • Light and Dark Mode.

There's a few problems with ActivityScenario for the way the
test was previously written:

- `recreate()` requires Activity to recreate to `RESUMED` state
- No way to set/clear state between `onSaveInstanceState` and `onCreate` when calling `recreate()`
- No way to start an Activity with `savedInstanceState`
@grzesiek2010 grzesiek2010 merged commit 419844e into getodk:master Jun 16, 2023
6 checks passed
@seadowg seadowg deleted the process-restore branch June 16, 2023 14:44
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.

Crash when restoring to some form entry screens
3 participants