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

 ) : 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

@@ -31,8 +30,7 @@ import timber.log.Timber
 import javax.inject.Inject

 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

@@ -31,7 +32,7 @@ interface VoiceBroadcastRecorder : VoiceRecorder {
    /** Current remaining time of recording, in seconds, if any. */
    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

@@ -79,13 +84,15 @@ class StartVoiceBroadcastUseCase @Inject constructor(
                ).toContent()
        )

-        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

sonarcloud · 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Florian14 commented Nov 23, 2022 •

edited

mnaturel Nov 24, 2022

mnaturel Nov 24, 2022

mnaturel Nov 24, 2022

Florian14 Nov 24, 2022

mnaturel Nov 24, 2022

mnaturel Nov 24, 2022

Florian14 Nov 28, 2022

mnaturel Nov 24, 2022

Florian14 Nov 28, 2022

mnaturel Nov 24, 2022 •

edited

Florian14 Nov 28, 2022

mnaturel Nov 24, 2022

Florian14 Nov 28, 2022

mnaturel left a comment

sonarcloud bot commented Nov 28, 2022

mnaturel left a comment

Florian14 commented Nov 29, 2022

		) : AbstractVoiceRecorderQ(context), VoiceBroadcastRecorder {

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

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

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

Conversation

Florian14 commented Nov 23, 2022 • edited

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel Nov 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 28, 2022

mnaturel left a comment

Choose a reason for hiding this comment

Florian14 commented Nov 29, 2022

Florian14 commented Nov 23, 2022 •

edited

mnaturel Nov 24, 2022 •

edited