Skip to content

Pause voice broadcast listening on new VB recording #7973

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
Merged
merged 1 commit into from
Jan 20, 2023
Merged

Pause voice broadcast listening on new VB recording #7973

merged 1 commit into from
Jan 20, 2023

Conversation

yostyle
Copy link
Contributor

@yostyle yostyle commented Jan 19, 2023

@yostyle yostyle added the A-Voice Broadcast Broadcast-style voice messages label Jan 19, 2023
@yostyle yostyle requested a review from Florian14 January 19, 2023 00:16
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@yostyle yostyle requested a review from bmarty January 19, 2023 10:15
Florian14
Florian14 reviewed Jan 20, 2023
View reviewed changes
vector/src/main/java/im/vector/app/features/home/room/detail/TimelineViewModel.kt
@@ -626,13 +626,17 @@ class TimelineViewModel @AssistedInject constructor(
viewModelScope.launch {
when (action) {
VoiceBroadcastAction.Recording.Start -> {
voiceBroadcastHelper.pausePlayback()
Copy link
Contributor

@Florian14 Florian14 Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if it would have been better to do this in StartVoiceBroadcastUseCase.startVoiceBroadcast (after checking if the user is allowed to start a VB). wdyt? It will introduce a circular DI issue, seems good like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to do that I need to inject the VoiceBroadcastHelper in StartVoiceBroadcastUseCase but StartVoiceBroadcastUseCase is already inject in VoiceBroadcastHelper

Florian14
Florian14 reviewed Jan 20, 2023
View reviewed changes
vector/src/main/java/im/vector/app/features/home/room/detail/TimelineViewModel.kt
voiceBroadcastHelper.startVoiceBroadcast(room.roomId).fold(
{ _viewEvents.post(RoomDetailViewEvents.ActionSuccess(action)) },
{ _viewEvents.post(RoomDetailViewEvents.ActionFailure(action, it)) },
)
}
VoiceBroadcastAction.Recording.Pause -> voiceBroadcastHelper.pauseVoiceBroadcast(room.roomId)
VoiceBroadcastAction.Recording.Resume -> voiceBroadcastHelper.resumeVoiceBroadcast(room.roomId)
VoiceBroadcastAction.Recording.Resume -> {
voiceBroadcastHelper.pausePlayback()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@yostyle yostyle added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Jan 20, 2023
@yostyle yostyle merged commit 5b22a12 into develop Jan 20, 2023
@yostyle yostyle deleted the yostyle/voicebroadcast_pause_playback branch January 20, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Florian14 Florian14 Florian14 approved these changes

@bmarty bmarty Awaiting requested review from bmarty

Assignees
No one assigned
Labels
A-Voice Broadcast Broadcast-style voice messages Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yostyle @Florian14