Fix the now playing info center while a voice broadcast is played #7257

nimau · 2023-01-10T17:08:50Z

This PR fixes the display in the Now Playing Info Center (NPIC) when a user is listening to a voice broadcast.
The NPIC was showing a voice message instead of a voice broadcast.

To test:

Listen to a voice broadcast then lock your phone.
The Now Playing Info Center should show "voice broadcast" instead of "voice message" and the duration is now equal to the total duration of the voice broadcast.

RiotSwiftUI/Modules/Room/VoiceBroadcastPlayback/MatrixSDK/VoiceBroadcastPlaybackViewModel.swift

stefanceriu · 2023-01-11T08:12:07Z

Riot/Modules/Room/VoiceMessages/VoiceMessageMediaServiceProvider.swift

@@ -93,6 +94,7 @@ import MediaPlayer
 private override init() {
 audioPlayers = NSMapTable<NSString, VoiceMessageAudioPlayer>(valueOptions: .weakMemory)
 audioRecorders = NSHashTable<VoiceMessageAudioRecorder>(options: .weakMemory)
+ nowPlayingInfoProviders = NSMapTable<VoiceMessageAudioPlayer, VoiceMessageNowPlayingInfoProvider>(valueOptions: .weakMemory)


Wouldn't this make all voice broadcasting audio players be retained forever?

Riot/Modules/Room/VoiceMessages/VoiceMessageMediaServiceProvider.swift

RiotSwiftUI/Modules/Room/VoiceBroadcastPlayback/MatrixSDK/VoiceBroadcastPlaybackViewModel.swift

changelog.d/pr-7257.bugfix

RiotSwiftUI/Modules/Room/VoiceBroadcastPlayback/MatrixSDK/VoiceBroadcastPlaybackViewModel.swift

giomfo · 2023-01-11T14:25:38Z

I tested, this change is working well on ended VB 👍
We just have some trouble in case of buffering, the "now playing info center' considers the playback is stopped
It looks like there is only 2 supported states: playing/stopped

We should check if there is a way to handle this intermediate state (buffering), mainly for live VB. If no solution is available for the moment, we may postpone the management of this potential buffering in the info center view. We should not block this PR for that. The current changes already enhance for some parts the user experience

nimau · 2023-01-12T08:44:21Z

@stefanceriu I made some changes. I tried to be as less invasive as possible to not break anything in the audio players management. Also, the currentlyPlayingAudioPlayer is a bit tricky because we have to consider the player being used by the voice broadcast to be active even if it's stopped (because we're in a buffering state, waiting for the next chunk) to be able to continue to update the NowPlayingInfoCenter.
Can you tell me if it looks ok for you ?

codecov · 2023-01-12T09:13:48Z

Codecov Report

Base: 11.83% // Head: 11.91% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (648b409) compared to base (3159ba1).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #7257      +/-   ##
===========================================
+ Coverage    11.83%   11.91%   +0.08%     
===========================================
  Files         1630     1637       +7     
  Lines       160349   160647     +298     
  Branches     65502    65643     +141     
===========================================
+ Hits         18970    19142     +172     
- Misses      140740   140867     +127     
+ Partials       639      638       -1

Flag	Coverage Δ
uitests	`55.23% <ø> (+0.38%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
Riot/Modules/Application/LegacyAppDelegate.m	`14.55% <0.00%> (ø)`
Riot/Modules/Room/RoomViewController.m	`0.00% <0.00%> (ø)`
...iceMessages/VoiceMessageMediaServiceProvider.swift	`0.00% <0.00%> (ø)`
...oordinator/VoiceBroadcastPlaybackCoordinator.swift	`0.00% <ø> (ø)`
...k/Coordinator/VoiceBroadcastPlaybackProvider.swift	`0.00% <0.00%> (ø)`
...ck/MatrixSDK/VoiceBroadcastPlaybackViewModel.swift	`0.00% <0.00%> (ø)`
...oordinator/VoiceBroadcastRecorderCoordinator.swift	`0.00% <0.00%> (ø)`
...r/Coordinator/VoiceBroadcastRecorderProvider.swift	`0.00% <0.00%> (ø)`
...vice/MatrixSDK/VoiceBroadcastRecorderService.swift	`0.00% <0.00%> (ø)`
...odules/Common/DependencyInjection/Injectable.swift	`0.00% <0.00%> (-100.00%)`	⬇️
... and 25 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

stefanceriu · 2023-01-12T09:42:32Z

Riot/Modules/Room/VoiceMessages/VoiceMessageMediaServiceProvider.swift

@@ -93,6 +94,7 @@ import MediaPlayer
 private override init() {
 audioPlayers = NSMapTable<NSString, VoiceMessageAudioPlayer>(valueOptions: .weakMemory)
 audioRecorders = NSHashTable<VoiceMessageAudioRecorder>(options: .weakMemory)
+ nowPlayingInfoDelegates = NSMapTable<VoiceMessageAudioPlayer, VoiceMessageNowPlayingInfoDelegate>(valueOptions: .weakMemory)


I'm still worried about this strongly retaining the key/audio player

stefanceriu · 2023-01-12T09:43:17Z

As a general rule please don't force push after receiving feedback as it completely breaks where the comments point to

stefanceriu

Just a small comment inline about how we retain audio players and there's still some opened conversations. Otherwise looks good to me 👍

nimau · 2023-01-12T14:58:27Z

Just a small comment inline about how we retain audio players and there's still some opened conversations. Otherwise looks good to me 👍

Following your comment, I also set the key as weak.

giomfo · 2023-01-13T07:40:06Z

Riot/Modules/Room/VoiceMessages/VoiceMessageMediaServiceProvider.swift

@@ -242,7 +264,7 @@ import MediaPlayer
 }
 }

- private func tearDownRemoteCommandCenter() {
+ private func tearDownRemoteCommandCenter(for audioPlayer: VoiceMessageAudioPlayer) {


The audioPlayer parameter is not used
Can you please remove it?

sonarcloud · 2023-01-16T14:26:40Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

nimau · 2023-01-16T14:29:41Z

@giomfo as discussed, a live broadcast should not appear in Info Center while playing. Also if the app goes into background while listening a live broadcast, the playback is paused because we cannot get the next chunks.

giomfo

SGTM, I share my comment while I run some additional tests

RiotSwiftUI/Modules/Room/VoiceBroadcastPlayback/MatrixSDK/VoiceBroadcastPlaybackViewModel.swift

giomfo

Tests ok
LGTM

nimau added the A-Voice-Broadcast label Jan 10, 2023

nimau requested review from giomfo, yostyle, stefanceriu and phloux January 10, 2023 17:14

nimau marked this pull request as ready for review January 10, 2023 17:14

giomfo reviewed Jan 11, 2023

View reviewed changes

RiotSwiftUI/Modules/Room/VoiceBroadcastPlayback/MatrixSDK/VoiceBroadcastPlaybackViewModel.swift Outdated Show resolved Hide resolved

stefanceriu requested changes Jan 11, 2023

View reviewed changes

yostyle suggested changes Jan 11, 2023

View reviewed changes

changelog.d/pr-7257.bugfix Outdated Show resolved Hide resolved

RiotSwiftUI/Modules/Room/VoiceBroadcastPlayback/MatrixSDK/VoiceBroadcastPlaybackViewModel.swift Outdated Show resolved Hide resolved

nimau added 3 commits January 11, 2023 18:14

Fixe the now playing info center while a voice broadcast is played

4dec1ce

Add Towncrier file

7a53a82

Fix NowPlayingInfoCenter for a live voice broadcast

b2ae0e7

nimau force-pushed the nimau/PSF-1734_vb_control_center branch from d25fa41 to b2ae0e7 Compare January 12, 2023 08:35

nimau requested review from stefanceriu, yostyle and giomfo January 12, 2023 08:44

stefanceriu reviewed Jan 12, 2023

View reviewed changes

stefanceriu self-requested a review January 12, 2023 09:46

stefanceriu approved these changes Jan 12, 2023

View reviewed changes

Remove strong references on audio players used for voicebroadcast

8ba0fea

Fix Towncrier file

e62ef38

nimau requested a review from stefanceriu January 12, 2023 15:03

stefanceriu approved these changes Jan 12, 2023

View reviewed changes

giomfo reviewed Jan 13, 2023

View reviewed changes

Live voice broadcast should not appear in Info Center while playing

648b409

giomfo reviewed Jan 16, 2023

View reviewed changes

RiotSwiftUI/Modules/Room/VoiceBroadcastPlayback/MatrixSDK/VoiceBroadcastPlaybackViewModel.swift Show resolved Hide resolved

giomfo approved these changes Jan 16, 2023

View reviewed changes

yostyle approved these changes Jan 17, 2023

View reviewed changes

nimau merged commit a9ff128 into develop Jan 17, 2023

nimau deleted the nimau/PSF-1734_vb_control_center branch January 17, 2023 08:38

giomfo mentioned this pull request Jan 17, 2023

Voice Broadcast post spike #6951

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the now playing info center while a voice broadcast is played #7257

Fix the now playing info center while a voice broadcast is played #7257

nimau commented Jan 10, 2023 •

edited

Loading

stefanceriu Jan 11, 2023

giomfo commented Jan 11, 2023

nimau commented Jan 12, 2023

codecov bot commented Jan 12, 2023 •

edited

Loading

stefanceriu Jan 12, 2023

stefanceriu commented Jan 12, 2023

stefanceriu left a comment

nimau commented Jan 12, 2023

giomfo Jan 13, 2023

sonarcloud bot commented Jan 16, 2023

nimau commented Jan 16, 2023

giomfo left a comment

giomfo left a comment

Fix the now playing info center while a voice broadcast is played #7257

Fix the now playing info center while a voice broadcast is played #7257

Conversation

nimau commented Jan 10, 2023 • edited Loading

stefanceriu Jan 11, 2023

Choose a reason for hiding this comment

giomfo commented Jan 11, 2023

nimau commented Jan 12, 2023

codecov bot commented Jan 12, 2023 • edited Loading

Codecov Report

stefanceriu Jan 12, 2023

Choose a reason for hiding this comment

stefanceriu commented Jan 12, 2023

stefanceriu left a comment

Choose a reason for hiding this comment

nimau commented Jan 12, 2023

giomfo Jan 13, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Jan 16, 2023

nimau commented Jan 16, 2023

giomfo left a comment

Choose a reason for hiding this comment

giomfo left a comment

Choose a reason for hiding this comment

nimau commented Jan 10, 2023 •

edited

Loading

codecov bot commented Jan 12, 2023 •

edited

Loading