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 saving audio answers #5979

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Fixed saving audio answers #5979

merged 3 commits into from
Feb 23, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Feb 21, 2024

Closes #5977

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

The implementation of passing new answers to the audio widget was incorrect. The issue stemmed from immediately saving the answer after recording audio, as seen here: https://github.com/getodk/collect/pull/5979/files#diff-06b81c70d38f41b152a55fd3c656029170b518aa952058c54e7b8d68041150b7L62

This approach diverges from the standard procedure followed by other widgets. Typically, answers are passed to the widget, with saving occurring later when answers for the current screen are saved.

Adopting the same process as with other widgets ensures consistency. When the answer is saved prematurely, as with the audio widget, it complicates the handling of question updates, especially on screens with multiple questions. This early saving prevents proper comparison of questions before and after saving answers for the current screen see: https://github.com/getodk/collect/blob/master/collect_app/src/main/java/org/odk/collect/android/activities/FormFillingActivity.java#L2301

Consequently, if a question depended on the audio widget and was displayed conditionally, it might not have been added to the list upon audio recording. Subsequently, attempting to remove such a question, which wasn't displayed, led to a crash when the audio was deleted.

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?

Apart from testing the issue itself, it would be good to verify that the audio widget works well with no regressions. I would take a look at the similar issues we have had recently:

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

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 February 21, 2024 13:36
@grzesiek2010 grzesiek2010 requested a review from seadowg February 21, 2024 13:36
@grzesiek2010 grzesiek2010 added the high priority Should be looked at before other PRs/issues label Feb 21, 2024
@seadowg
Copy link
Member

seadowg commented Feb 23, 2024

@srujner @dbemke I think doing a deep dive on the audio widget is a good idea here (as @grzesiek2010 already suggests) as we're fundamentally changing how the audio file gets saved. I think playing around with rotation/"Don't keep activities" and using device file explorer to verify what files end up on disk would be good things to look at.

@seadowg seadowg merged commit 8c72f97 into getodk:master Feb 23, 2024
6 checks passed
@srujner
Copy link

srujner commented Feb 27, 2024

Tested with Success!

Verified on device with Android 12,13

Verified cases:

@dbemke
Copy link

dbemke commented Feb 27, 2024

Tested with Success!

Verified on device with Android 10,14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior verified high priority Should be looked at before other PRs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting a recorded audio in Audio widget on a field-list results in an error and closing the form
4 participants