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.

Sign up for GitHub

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

Jump to bottom

fix: release mic from audio component #1974

Merged
merged 4 commits into from
Jul 19, 2023
Merged

fix: release mic from audio component #1974

merged 4 commits into from
Jul 19, 2023

Conversation

alexandreferris
Copy link
Contributor

@alexandreferris alexandreferris commented Jul 19, 2023 •
edited
Loading

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

If user is removed from the group or any other action that disabled the user to manually stop the recording audio, the microphone would stay on.

Solutions

Now on onDispose of Audio Recording Component we are also calling stopRecording to fully release device microphone

@alexandreferris alexandreferris self-assigned this Jul 19, 2023
@codecov
Copy link

codecov bot commented Jul 19, 2023 •
edited
Loading

Codecov Report

Merging #1974 (407cd33) into develop (8a9dbdf) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             develop    #1974      +/-   ##
=============================================
+ Coverage      38.49%   38.50%   +0.01%     
  Complexity       892      892              
=============================================
  Files            287      287              
  Lines          11313    11315       +2     
  Branches        1449     1449              
=============================================
+ Hits            4355     4357       +2     
  Misses          6530     6530              
  Partials         428      428
Impacted Files Coverage Δ
.../messagecomposer/state/MessageCompositionHolder.kt 28.90% <100.00%> (+0.68%) ⬆️

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 b6f6b1f...407cd33. Read the comment docs.

@github-actions
Copy link
Contributor

Build (beta-debug) available here. Scroll down to Artifacts!

@github-actions
Copy link
Contributor

Build (dev-debug) available here. Scroll down to Artifacts!

@github-actions
Copy link
Contributor

github-actions bot commented Jul 19, 2023 •
edited
Loading

Test Results

506 tests   505 ✔️  24s ⏱️
  74 suites      1 💤
  74 files        0 ❌

Results for commit 407cd33.

♻️ This comment has been updated with latest results.

@AndroidBob
Copy link
Collaborator

Build 520 succeeded.

The build produced the following APK's:

@alexandreferris alexandreferris requested review from a team, typfel, gongracr, mchenani, Garzas and saleniuk and removed request for a team July 19, 2023 12:54
@CLAassistant
Copy link

CLAassistant commented Jul 19, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

saleniuk
saleniuk reviewed Jul 19, 2023
View reviewed changes
...src/main/kotlin/com/wire/android/ui/home/messagecomposer/recordaudio/RecordAudioViewModel.kt Outdated
@@ -241,6 +241,7 @@ class RecordAudioViewModel @Inject constructor(

override fun onCleared() {
super.onCleared()
stopRecording()
Copy link
Contributor

@saleniuk saleniuk Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I believe it won't stop recording when you navigate to another screen because in such situation current one is kept in the back stack and its ViewModel is not destroyed so onCleared isn't called. It will only happen when we close the screen and remove it from the back stack, so for instance when navigating back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I have moved it to onDispose on lifecycle observer in the View Component as we talked.

@github-actions
Copy link
Contributor

Build (dev-debug) available here. Scroll down to Artifacts!

@github-actions
Copy link
Contributor

Build (beta-debug) available here. Scroll down to Artifacts!

@alexandreferris alexandreferris requested a review from saleniuk July 19, 2023 13:45
@github-actions
Copy link
Contributor

Build (beta-debug) available here. Scroll down to Artifacts!

@github-actions
Copy link
Contributor

Build (dev-debug) available here. Scroll down to Artifacts!

@alexandreferris alexandreferris added this pull request to the merge queue Jul 19, 2023
@AndroidBob
Copy link
Collaborator

Build 528 succeeded.

The build produced the following APK's:

Merged via the queue into develop with commit 865f4fb Jul 19, 2023
@alexandreferris alexandreferris deleted the fix/release_mic_from_audio_component branch July 19, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trOnk12 trOnk12 trOnk12 approved these changes

@Garzas Garzas Garzas approved these changes

@typfel typfel Awaiting requested review from typfel typfel was automatically assigned from wireapp/android

@gongracr gongracr Awaiting requested review from gongracr gongracr was automatically assigned from wireapp/android

@mchenani mchenani Awaiting requested review from mchenani mchenani was automatically assigned from wireapp/android

@saleniuk saleniuk Awaiting requested review from saleniuk

Assignees

@alexandreferris alexandreferris

Labels
size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alexandreferris @AndroidBob @CLAassistant @Garzas @trOnk12 @saleniuk