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] AudioRecorder improvements #20915

Merged
merged 4 commits into from
May 31, 2024

Conversation

pantstamp
Copy link
Contributor

Fixes #
https://github.com/Automattic/wordpress-mobile/issues/93
https://github.com/Automattic/wordpress-mobile/issues/94
https://github.com/Automattic/wordpress-mobile/issues/95
https://github.com/Automattic/wordpress-mobile/issues/96

This PR introduces some minor improvements for the AudioRecorder like better error handling, recording params special values, threshold values to ensure that recording constraints are met.


To Test:

Please review the code and follow the steps below to ensure that nothing is broken.

  • Install and launch the app
  • Login into a site in which you have access to create posts
  • Navigate to Me > Debug Settings
  • Enable voice_to_content and restart the app
  • Navigate to My Site
  • Tap the FAB
  • Tap the microphone
  • Give the "Record audio" permission.
  • Say something
  • Tap the "stop" icon

✅ Verify the microphone disappears, "Assistant Feature Returned Successfully" text is shown quickly, and then the text changes to what you have said.


Regression Notes

  1. Potential unintended areas of impact

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

    • TODO
  3. What automated tests I added (or what prevented me from doing so)

    • TODO

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):

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

@pantstamp pantstamp force-pushed the issue/93-recorder-leftovers branch from 5ede5bc to 5fda76b Compare May 30, 2024 13:25
Copy link
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

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

@pantstamp - I like the improvements. 👏 I have two thoughts. These can definitely be overkill thoughts on my part and I am NOT requesting changes. Just curious to hear your thoughts.

💡
(1) Adding an initialization method that takes an interface for configurable parameters such as maxFileSize, maxDuration, storeInMemory, and recordingFileName. This approach follows the Strategy design pattern, allowing different recording configurations to be easily swapped in and out. Something like this:

interface RecordingStrategy {
    val maxFileSize: Long
    val maxDuration: Int
    val storeInMemory: Boolean
    val recordingFileName: String
} 

and then a VoiceToContentRecordingStrategy.

This could work with interfaces or sealed classes.

sealed class RecordingStrategy {
    data class VoiceToContentRecordingStrategy(
        val maxFileSize: Long = xxx,
        val maxDuration: Int = yyy,
        val recordingFileName: String = "asdfas",
        val storeInMemory: Boolean = true
    ) : RecordingStrategy()
}

(2) Using a sealed class to represent the different outcomes of the recording process instead of a plain String. This method allows for clear differentiation between success and error states and makes the code more maintainable and expressive. Ignore my too long method names and such.

sealed class AudioRecorderFinishedResponse {
    data class Success(val fileNameWithPath: String) : AudioRecorderFinishedResponse()
    data class Error(val errorMessage: String) : AudioRecorderFinishedResponse()
}

You can even use a flow to return the response

wdyt? Am I nuts?

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 30, 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
Versionpr20915-91a4f5b
Commit91a4f5b
Direct Downloadjetpack-prototype-build-pr20915-91a4f5b.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 30, 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
Versionpr20915-91a4f5b
Commit91a4f5b
Direct Downloadwordpress-prototype-build-pr20915-91a4f5b.apk
Note: Google Login is not supported on these builds.

@dangermattic
Copy link
Collaborator

dangermattic commented May 30, 2024

2 Warnings
⚠️ Class AudioRecorderResult is missing tests, but unit-tests-exemption label was set to ignore this.
⚠️ Class RecordingStrategy is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by 🚫 Danger

Copy link

sonarcloud bot commented May 31, 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

Copy link

codecov bot commented May 31, 2024

Codecov Report

Attention: Patch coverage is 1.40845% with 70 lines in your changes are missing coverage. Please review.

Project coverage is 40.94%. Comparing base (84df633) to head (91a4f5b).
Report is 13 commits behind head on trunk.

Files Patch % Lines
.../org/wordpress/android/util/audio/AudioRecorder.kt 0.00% 50 Missing ⚠️
...droid/ui/voicetocontent/VoiceToContentViewModel.kt 9.09% 10 Missing ⚠️
.../wordpress/android/util/audio/RecordingStrategy.kt 0.00% 6 Missing ⚠️
...org/wordpress/android/util/audio/IAudioRecorder.kt 0.00% 3 Missing ⚠️
...ress/android/ui/voicetocontent/RecordingUseCase.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #20915      +/-   ##
==========================================
- Coverage   40.96%   40.94%   -0.02%     
==========================================
  Files        1517     1518       +1     
  Lines       69516    69547      +31     
  Branches    11468    11473       +5     
==========================================
  Hits        28474    28474              
- Misses      38456    38487      +31     
  Partials     2586     2586              

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

@pantstamp
Copy link
Contributor Author

@pantstamp - I like the improvements. 👏 I have two thoughts. These can definitely be overkill thoughts on my part and I am NOT requesting changes. Just curious to hear your thoughts.

💡 (1) Adding an initialization method that takes an interface for configurable parameters such as maxFileSize, maxDuration, storeInMemory, and recordingFileName. This approach follows the Strategy design pattern, allowing different recording configurations to be easily swapped in and out. Something like this:

interface RecordingStrategy {
    val maxFileSize: Long
    val maxDuration: Int
    val storeInMemory: Boolean
    val recordingFileName: String
} 

and then a VoiceToContentRecordingStrategy.

This could work with interfaces or sealed classes.

sealed class RecordingStrategy {
    data class VoiceToContentRecordingStrategy(
        val maxFileSize: Long = xxx,
        val maxDuration: Int = yyy,
        val recordingFileName: String = "asdfas",
        val storeInMemory: Boolean = true
    ) : RecordingStrategy()
}

(2) Using a sealed class to represent the different outcomes of the recording process instead of a plain String. This method allows for clear differentiation between success and error states and makes the code more maintainable and expressive. Ignore my too long method names and such.

sealed class AudioRecorderFinishedResponse {
    data class Success(val fileNameWithPath: String) : AudioRecorderFinishedResponse()
    data class Error(val errorMessage: String) : AudioRecorderFinishedResponse()
}

You can even use a flow to return the response

wdyt? Am I nuts?

Both ideas are great @zwarm ! I pushed some extra commits that include these implementations. Thank you!!!

@pantstamp pantstamp merged commit 62d260a into trunk May 31, 2024
21 checks passed
@pantstamp pantstamp deleted the issue/93-recorder-leftovers branch May 31, 2024 09:18
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