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

[Voice to Content] Clear recorder resources #20983

Merged
merged 6 commits into from
Jun 17, 2024
Merged

Conversation

zwarm
Copy link
Contributor

@zwarm zwarm commented Jun 14, 2024

This PR addresses an issue with the AudioRecorder not clearing it's resources. With this implementation the process now captures the bottom sheet close event, the close tap and sends the endRecordingSession event to the usecase. This is turn will release the resources of the recorder.

Note

The following have not yet been implemented
recording timer
orientation changes
Overall polishing of the UI
Launching Edit Post


To Test:

There is not much to test

  • Install and launch the app
  • Navigate to Me > Debug Settings and enable the voice_to_content flag (restart the app)
  • Navigate to My Site and tap the FAB
  • Select the Post with Audio option
  • Start the recording session by tapping on the mic icon
  • Tap the close button or swipe down to close the bottom sheet
  • ✅ Verify the app doesn't crash or do anything weird

Regression Notes

  1. Potential unintended areas of impact
    The resources are not released

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing

  3. What automated tests I added (or what prevented me from doing so)
    N/A


PR Submission Checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing Checklist (strike-out the not-applying and unnecessary ones): N/A

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@zwarm zwarm requested a review from irfano June 14, 2024 13:35
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 14, 2024

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr20983-9aa4aec
Commit9aa4aec
Direct Downloadjetpack-prototype-build-pr20983-9aa4aec.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 14, 2024

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr20983-9aa4aec
Commit9aa4aec
Direct Downloadwordpress-prototype-build-pr20983-9aa4aec.apk
Note: Google Login is not supported on these builds.

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 11.11111% with 8 lines in your changes missing coverage. Please review.

Project coverage is 40.75%. Comparing base (29f9541) to head (9aa4aec).

Files Patch % Lines
...droid/ui/voicetocontent/VoiceToContentViewModel.kt 16.66% 5 Missing ⚠️
.../org/wordpress/android/util/audio/AudioRecorder.kt 0.00% 2 Missing ⚠️
...ress/android/ui/voicetocontent/RecordingUseCase.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #20983      +/-   ##
==========================================
- Coverage   40.75%   40.75%   -0.01%     
==========================================
  Files        1528     1528              
  Lines       70165    70174       +9     
  Branches    11607    11607              
==========================================
+ Hits        28595    28596       +1     
- Misses      38980    38988       +8     
  Partials     2590     2590              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@irfano irfano left a comment

Choose a reason for hiding this comment

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

I tested whether AudioRecorder.clearResources() is called every time the bottom sheet is closed. It is not called when dismissing the bottom sheet:

  • I opened the "Post from audio" bottom sheet. I tapped outside the prompt to dismiss it, and AudioRecorder.clearResources() was not invoked.

It should have been invoked in this case as well, right?

Copy link

sonarcloud bot commented Jun 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@zwarm
Copy link
Contributor Author

zwarm commented Jun 14, 2024

It is not called when dismissing the bottom sheet:

Good catch. Nope, I didn't think of this scenario. I added the logic in 9aa4aec. I don't want an accidental outside touch to stop the process when in recording or processing mode, so I am setting the isCancelableOutsideTouch dynamically.

np: // Handle other states if necessary and // Handle the slide offset if needed comments do not look useful to me. Maybe they remained after copy&paste. If you make them on purpose, you can ignore this and resolve the comment

I haven't prioritized cleaning up the code yet. My final PR will do that and clean up log lines, etc. I want to get the flow working properly first.

Thanks. :)

Copy link
Member

@irfano irfano left a comment

Choose a reason for hiding this comment

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

Thanks for the update!
I have one more question. Currently, we're calling recordingUseCase.endRecordingSession() when the bottom sheet is closed, even if the MediaRecorder has not started. Shouldn't .endRecordingSession() be called only if the recording has actually started?
If you plan to handle this in the following PRs, I can approve this one.

@zwarm
Copy link
Contributor Author

zwarm commented Jun 17, 2024

Thanks for the update! I have one more question. Currently, we're calling recordingUseCase.endRecordingSession() when the bottom sheet is closed, even if the MediaRecorder has not started. Shouldn't .endRecordingSession() be called only if the recording has actually started? If you plan to handle this in the following PRs, I can approve this one.

@irfano Handled this in a future branch ... had to wait to actually have the isRecording or isPaused values available in the VM first.

Copy link
Member

@irfano irfano left a comment

Choose a reason for hiding this comment

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

LGTM! 👍🏻

@irfano irfano merged commit f2ad356 into trunk Jun 17, 2024
20 checks passed
@irfano irfano deleted the issue/v2c-clear-resources branch June 17, 2024 15:39
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.

4 participants