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

Fixed minimizing and reopening the app while on enter name dialog #5888

Merged
merged 4 commits into from
Jan 17, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Jan 3, 2024

Closes #5549

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

When we reopen the app (with a started form) the onResume , method from FormFillingActivity is called and it checks if formController object s null:

if (formController == null) {
    // there is no formController -- fire MainMenu activity?
    Timber.w("Starting MainMenuActivity because formController is null/formLoaderTask is null");
    startActivity(new Intent(this, MainMenuActivity.class));
    exit();
    return;
}

if so it closes the form assuming something went wrong.
The thing is that if Enumerator identification is enabled we set formController only after passing username and navigating to the form. That's why such forms used to be closed after reopening the app and performing onResume.

I can't see any reason why we shouldn't set formController earlier to fix this bug. That's what I did!
Another solution would be to keep it as it is and add another check in onResume to prevent closing the form if formController is null when we are waiting for username. It should work as well but one another check is what I like less.

[EDIT] The first solution turned out to be buggy see: #5888 (comment) so I had to use the second one.

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?

Forms that use Enumerator identification should be tested because they are at risk. There should be no difference in the behavior of other forms (without that option enabled) so I believe they are safe. It's difficult to tell what might be broken (if anything) in the forms with Enumerator identification](https://docs.getodk.org/form-audit-log/#enumerator-identification) so please be creative.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • 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

@grzesiek2010 grzesiek2010 marked this pull request as ready for review January 3, 2024 14:40
@grzesiek2010 grzesiek2010 requested a review from seadowg January 3, 2024 14:40
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

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

I'm pretty worried moving formLoaded will cause logs to be different (we might end up with a form resume we don't want for instance) but I've not found a case as of yet. I do definitely prefer the new code though!

For the moment, I'm happy to accept, but let's not merge until we've got to "behaviour verified" for this one.

@dbemke
Copy link

dbemke commented Jan 10, 2024

I'm not sure what is the expected behavior - if I open the Identify User form and while on the dialog "Enter identity" I press the back button (there isn't a dialog in which I can discard changes) and the form is closed, in the file directory a file for this opened form is created (the file is empty). Is it expected that this type of form creates a file in the directory each time the form is opened? OR maybe the dialog to save or discard changes is missing?

@dbemke
Copy link

dbemke commented Jan 10, 2024

Another case if I minimize the app while on the "Enter identity" and go back to the app, the form is closed and the file in the directory is also created. What is the expected result after changes in the PR after minimizing the app - should the form close?

@grzesiek2010
Copy link
Member Author

I'm not sure what is the expected behavior - if I open the Identify User form and while on the dialog "Enter identity" I press the back button (there isn't a dialog in which I can discard changes) and the form is closed, in the file directory a file for this opened form is created (the file is empty). Is it expected that this type of form creates a file in the directory each time the form is opened? OR maybe the dialog to save or discard changes is missing?

We don't need to add any dialog. The empty directory is of course not needed but it doesn't cause any problems plus it's not even visible to a user. I think you can file a separate issue for removing it (or maybe rather creating later after passing the identity.

Another case if I minimize the app while on the "Enter identity" and go back to the app, the form is closed and the file in the directory is also created. What is the expected result after changes in the PR after minimizing the app - should the form close?

It should be restored. I've tested the fix again now and it looks like it works like that. Please double check maybe you were on master branch?

@dbemke
Copy link

dbemke commented Jan 12, 2024

Please double check maybe you were on master branch?

Yes, I used the master branch as now after the changes by default when there's label needs testing we test on the master branch. If for some reason you would like us to test a PR before merging please be specific which version you would like us to test and add this info in the risk section.

@grzesiek2010
Copy link
Member Author

Yes, I used the master branch as now after the changes by default when there's label needs testing we test on the master branch. If for some reason you would like us to test a PR before merging please be specific which version you would like us to test and add this info in the risk section.

This pr is not merged as you can see. @seadowg wanted to test it first:

but let's not merge until we've got to "behaviour verified" for this one.

so you have to test the pr using its branch.

@dbemke
Copy link

dbemke commented Jan 16, 2024

In the apk in this PR when don't keep activities setting is enabled minimizing the app when on "Enter identity" dialog and going back to the app results in being moved to the first question so the "Enter identity" isn't shown and possible to fill in. The issue doesn't occur on the store version.
Steps to reproduce the problem

  1. In the device settings don't keep activities is enabled.
  2. Go to Identify User form.
  3. When you see "Enter identity” dialog minimize the app.
  4. Go back to the app.

After going back to the app the user sees the first question

Expected result
After minimizing the app on "Enter identity" dialog and going back to it, the user should still be on the same dialog (not the first question)

@grzesiek2010
Copy link
Member Author

@dbemke please try now, I hope it should be fixed.

@dbemke
Copy link

dbemke commented Jan 17, 2024

After the change the dialog doesn't disappear. There's only one thing which I'm not sure if it can be prevented. When don't keep activities setting is enabled. When in the Identify user form while on the "Enter identity" dialog I enter something in the field, minimize the app and go back, the filled identity disappears (need to enter it again). It works the same on the store version so it's not the PR but I'm not sure if should file an issue for that or the way it works is fine?

@grzesiek2010
Copy link
Member Author

Yeah, I think a separate issue would be ok in this case.

@dbemke
Copy link

dbemke commented Jan 17, 2024

Tested with Success!

Verified on Androids: 10

Verified cases:

@srujner
Copy link

srujner commented Jan 17, 2024

Tested with Success!

Verified on Androids: 13

@grzesiek2010 grzesiek2010 merged commit 7c41ddd into getodk:master Jan 17, 2024
6 checks passed
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.

Form closes and two instances of Identify User form created after minimizng the app while on enter name dialog
4 participants