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

fix: audio message filter toggle (WPB-6406) #2847

Merged
merged 7 commits into from
Apr 3, 2024

Conversation

alexandreferris
Copy link
Contributor

@alexandreferris alexandreferris commented Apr 2, 2024

StoryWPB-6406 [Android] Apply Filters to Audio Recording


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Wrong handling of checkbox to apply filters

Causes (Optional)

Misunderstood of ACs caught only after design review

Solutions

  • Checkbox on last state (ready to send audio file) is now enabled.
  • Checkbox is also enabled whilst playing the preview audio, which will also change the preview audio played.
  • Moved AudioEffect.applyEffectM4A(..) to a separate use case in order to fully test the view model (as we cannot mock/test this class)

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

How to Test

  • Record Audio (can check or not the apply filters options in the beginning)
  • on the last state (ready to send audio), can listen to recorded audio in original form (no selected checkbox) or with effects (selected checkbox)

Note: audio will continue from last position when selecting/unselecting the checkbox (meaning if audio is 50% listen and checkbox is selected, then it will continue from 50% of it)

@@ -118,7 +118,7 @@ class RecordAudioMessagePlayer @Inject constructor(
suspend fun playAudio(
audioFile: File
) {
if (currentAudioFile != null) {
if (currentAudioFile != null && audioFile.name == currentAudioFile?.name) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If current file name is different from what we receive, then we reset the media player and continue with the new received file

Comment on lines +76 to +81
fun setApplyEffectsAndPlayAudio(enabled: Boolean) {
setShouldApplyEffects(enabled = enabled)
if (state.audioState.audioMediaPlayingState is AudioMediaPlayingState.Playing) {
onPlayAudio()
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created in order to continue playing the audio if it was being played before, otherwise we just ignore the player and just update the checkbox value

Comment on lines -170 to -184
if (state.shouldApplyEffects) {
val result = AudioEffect(context)
.applyEffectM4A(
state.originalOutputFile!!.path,
state.effectsOutputFile!!.path,
AudioEffect.AVS_AUDIO_EFFECT_VOCODER_MED,
true
)

if (result > -1) {
appLogger.i("[$tag] -> Audio file with effects generated successfully.")
} else {
appLogger.w("[$tag] -> There was an issue with generating audio file with effects.")
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to usecase so this viewmodel could continue being tested throughout, as we can't mock AVS AudioEffect

@alexandreferris alexandreferris requested review from a team, vitorhugods, mchenani, Garzas, ohassine and saleniuk and removed request for a team April 2, 2024 14:15
Copy link
Contributor

github-actions bot commented Apr 2, 2024

Test Results

878 tests   878 ✅  12m 10s ⏱️
120 suites    0 💤
120 files      0 ❌

Results for commit 18e8368.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Apr 2, 2024

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 3907 succeeded.

The build produced the following APK's:

@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2024

Codecov Report

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

Project coverage is 43.76%. Comparing base (f24c968) to head (18e8368).
Report is 5 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2847      +/-   ##
===========================================
+ Coverage    43.73%   43.76%   +0.03%     
===========================================
  Files          422      423       +1     
  Lines        14086    14066      -20     
  Branches      2544     2501      -43     
===========================================
- Hits          6160     6156       -4     
+ Misses        7203     7193      -10     
+ Partials       723      717       -6     
Files Coverage Δ
...oid/media/audiomessage/RecordAudioMessagePlayer.kt 1.38% <0.00%> (ø)
...essagecomposer/recordaudio/RecordAudioViewModel.kt 63.35% <62.50%> (+4.84%) ⬆️
...recordaudio/GenerateAudioFileWithEffectsUseCase.kt 9.09% <9.09%> (ø)

... and 12 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f71d13e...18e8368. Read the comment docs.

@AndroidBob
Copy link
Collaborator

Build 3916 succeeded.

The build produced the following APK's:

@alexandreferris alexandreferris added this pull request to the merge queue Apr 3, 2024
Copy link
Contributor

github-actions bot commented Apr 3, 2024

APKs built during tests are available here. Scroll down to Artifacts!

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 3, 2024
@alexandreferris alexandreferris added this pull request to the merge queue Apr 3, 2024
Merged via the queue into develop with commit 7b1afb5 Apr 3, 2024
14 checks passed
@alexandreferris alexandreferris deleted the fix/audio_message_filter_toggle branch April 3, 2024 07:20
@echoes-hq echoes-hq bot added the echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. label Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants