Voice Broadcast - Handle event deletion when listening or recording #7629

Florian14 · 2022-11-23T13:41:50Z

Type of change

WIP Feature
Bugfix
Technical
Other :

Content

Handle the deletion of a voice broadcast state event on the recorder and the listener sides.

Improve the voice broadcast state observation by handling the event redaction.
Stop listening to this VB as soon as the started event is redacted
Stop recording this VB as soon as the started event is redacted
The recorder now reacts to the state event changes (previously, we sent the state event and did the action on the recorder at the same time)

Motivation and context

Continue #7127

Screenshots / GIFs

Tests

Show hidden events in the timeline (advanced settings -> developer mode)
Record a VB with several state events (started/paused/resumed/stopped) and verify that the tile in the timeline is correctly updated on listener and recorder sides
Redact some state events and verify that the tile is always displayed according to the most recent not redacted state event
When redacting the started state event (behind the tile), the tile should be removed

Test again during recording and listening to the VB, delete the started event should stop the action.

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

mnaturel · 2022-11-24T13:05:01Z

vector/src/main/java/im/vector/app/features/voicebroadcast/recording/VoiceBroadcastRecorderQ.kt

+        private val getVoiceBroadcastEventUseCase: GetMostRecentVoiceBroadcastStateEventUseCase
 ) : AbstractVoiceRecorderQ(context), VoiceBroadcastRecorder {

+    private val session get() = sessionHolder.getActiveSession()


Have you considered it may throw an exception when getting the session variable?

mnaturel · 2022-11-24T13:06:40Z

.../java/im/vector/app/features/voicebroadcast/recording/usecase/ResumeVoiceBroadcastUseCase.kt

 class ResumeVoiceBroadcastUseCase @Inject constructor(
-        private val session: Session,
-        private val voiceBroadcastRecorder: VoiceBroadcastRecorder?,
+        private val session: Session


Should we keep the trailing comma?

mnaturel · 2022-11-24T13:14:03Z

vector/src/main/java/im/vector/app/features/voicebroadcast/recording/VoiceBroadcastRecorder.kt

    val currentRemainingTime: Long?

-    fun startRecord(roomId: String, chunkLength: Int, maxLength: Int)
+    fun startRecordVoiceBroadcast(voiceBroadcast: VoiceBroadcast, chunkLength: Int, maxLength: Int)


Do you think we need to add VoiceBroadcast in the name of the method? I personnally think this is enough with startRecord since first parameter is a VoiceBroadcast.

My problem is that I want to distinguish this method from the startRecord which is inherited from VoiceRecorder to avoid confusion. Ideally, I would like to only expose this one to the outside and change the visibility of the other method to "protected", but afaik I can do it programmatically but this is a bit ugly imo. Wdyt @mnaturel?

I see. I didn't see the method in the VoiceRecorder Interface. Let's keep it like this then.

mnaturel · 2022-11-24T13:21:56Z

...n/java/im/vector/app/features/voicebroadcast/recording/usecase/StartVoiceBroadcastUseCase.kt


-        startRecording(room, eventId, chunkLength, maxLength)
+        val voiceBroadcast = VoiceBroadcast(roomId = room.roomId, voiceBroadcastId = eventId)
+        room.flow().liveTimelineEvent(eventId).unwrap().first() // wait for the event come back from the sync


I think we need to update the related unit tests to take this change into account.

I tried but I was stuck with an infinite loading on the Flow, I added a todo to not forget to update the related test

mnaturel · 2022-11-24T13:23:02Z

...m/vector/app/features/voicebroadcast/usecase/GetMostRecentVoiceBroadcastStateEventUseCase.kt

+import timber.log.Timber
+import javax.inject.Inject
+
+class GetMostRecentVoiceBroadcastStateEventUseCase @Inject constructor(


I think this use case needs some unit tests.

I suggest doing it in a dedicated PR to unblock this one, I created an issue with the missing tests to track this one

mnaturel · 2022-11-24T13:34:07Z

...m/vector/app/features/voicebroadcast/usecase/GetMostRecentVoiceBroadcastStateEventUseCase.kt

+                        flowOf(Optional.empty())
+                    } else {
+                        // otherwise, observe most recent event changes
+                        getMostRecentRelatedEventFlow(room, voiceBroadcast)


Have you thought about this way of writing this logic?

getMostRecentRelatedEventFlow(room, voiceBroadcast) .transformWhile { mostRecentEvent -> val hasValue = mostRecentEvent.hasValue() if(hasValue) { // keep the most recent event emit(mostRecentEvent) } else { // no most recent event, fallback to started event emit(startedEvent) } hasValue }

I definitely prefer this format, updated aa53105

mnaturel · 2022-11-24T13:57:18Z

...m/vector/app/features/voicebroadcast/usecase/GetMostRecentVoiceBroadcastStateEventUseCase.kt

+                                        event.hasValue() && event.get().root.isRedacted().not()
+                                    }
+                                    .flatMapLatest { event ->
+                                        if (event.getOrNull()?.root?.isRedacted().orFalse()) {


What was your intention when event is null? The comment says it should be in this if branch but using orFalse means it will go through the else branch.

you're right, I fixed and rewrote the condition for better clarity

mnaturel

I have added some small comments.
Just a general thought about redaction of voice broadcast events. For the default user, when pressing the "Remove" button on a voice broadcast, it seems it only redacts the started event. It is intended? I expected it redacts every associated events of the given voice broadcast (message events + state events).

…oadcast state event

…corder

sonarqubecloud · 2022-11-28T16:13:38Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

6.9% Coverage
0.0% Duplication

mnaturel

LGTM. Thanks for the updates!
I have just a general question: I have seen when removing from the tile of a voice broadcast, it actually doesn't remove all the related events like messages or related state events to the original event. As a user, I would expect it redacts every related events to the selected voice broadcast. Can you confirm it will be implemented this way in the future?

Florian14 · 2022-11-29T08:34:57Z

LGTM. Thanks for the updates! I have just a general question: I have seen when removing from the tile of a voice broadcast, it actually doesn't remove all the related events like messages or related state events to the original event. As a user, I would expect it redacts every related events to the selected voice broadcast. Can you confirm it will be implemented this way in the future?

Sorry, I forgot to answer this question, indeed it is not done for the moment but we'll implement the proper redaction of all the related events in a dedicated PR by using this MSC

Florian14 requested review from a team and mnaturel and removed request for a team November 23, 2022 16:56

Florian14 marked this pull request as ready for review November 23, 2022 16:56

mnaturel reviewed Nov 24, 2022

View reviewed changes

Florian14 force-pushed the feature/fre/voice_broadcast_deletion branch 3 times, most recently from f59be73 to a8f3bb1 Compare November 25, 2022 13:45

Florian14 force-pushed the feature/fre/voice_broadcast_handle_event_deletion branch from 0e74402 to 81a2a10 Compare November 25, 2022 14:38

Base automatically changed from feature/fre/voice_broadcast_deletion to develop November 28, 2022 12:57

Florian Renaud added 10 commits November 28, 2022 13:57

Handle redaction when observing voice broadcast state changes

2d60e49

Handle voice broadcast deletion on listener side

f436de1

Update voice broadcast recorder according to the most recent voice br…

763b60e

…oadcast state event

changelog

3ebcd8c

Do not wait for state event feedback for pause/stop actions on the re…

023326a

…corder

Fix bad condition

a2dee21

Fix wrong sequence number in stopped state event content

d092c83

Fix issue on live playback detection

9dba6d7

Restore trailing comma

4427156

improve flow stream

aa53105

Florian14 force-pushed the feature/fre/voice_broadcast_handle_event_deletion branch from 81a2a10 to f85f4cf Compare November 28, 2022 15:03

Rewrite condition for better clarity

620bebc

Florian14 force-pushed the feature/fre/voice_broadcast_handle_event_deletion branch from f85f4cf to 620bebc Compare November 28, 2022 15:08

Add todo for missing unit test

9840731

Florian14 requested a review from mnaturel November 28, 2022 15:54

mnaturel approved these changes Nov 29, 2022

View reviewed changes

Florian14 merged commit d6fd32b into develop Nov 29, 2022

Florian14 deleted the feature/fre/voice_broadcast_handle_event_deletion branch November 29, 2022 08:51

giomfo mentioned this pull request Jan 6, 2023

voice broadcast：broadcasting event still on the screen after removing... #7470

Closed

Voice Broadcast - Handle event deletion when listening or recording #7629

Voice Broadcast - Handle event deletion when listening or recording #7629

Uh oh!

Conversation

Florian14 commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaturel Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaturel left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 28, 2022

Uh oh!

mnaturel left a comment

Choose a reason for hiding this comment

Uh oh!

Florian14 commented Nov 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Florian14 commented Nov 23, 2022 •

edited

Loading

mnaturel Nov 24, 2022 •

edited

Loading