Voice Broadcast - Improve timeline rendering code #7448

Florian14 · 2022-10-25T14:47:57Z

Type of change

WIP Feature
Bugfix
Technical
Other :

Content

Improve the codebase behind the rendering of a voice broadcast in the timeline.
Reduce duplicated code by adding some abstraction
Rework layouts
Better handling of the recording state after restarting the app (the "recorder" is not active anymore but the voice broadcast is still in a recording state)

Motivation and context

Improve the code and manage unhandled state

Screenshots / GIFs

(Non-exhaustive screenshots, rendering is almost the same as before, only some padding changes)

Recording	Playing

Tests

Play with voice broadcast and observe timeline update according to the recording & listening state changes

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~~ resilience-rc 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-10-26T08:02:55Z

...n/java/im/vector/app/features/home/room/detail/timeline/factory/VoiceBroadcastItemFactory.kt

+        val voiceBroadcastAttributes = AbsMessageVoiceBroadcastItem.Attributes(
+                voiceBroadcastId = voiceBroadcastId,
+                voiceBroadcastState = voiceBroadcastContent.voiceBroadcastState,
+                recorderName = params.event.root.stateKey?.let { session.getUserOrDefault(it) }?.toMatrixItem()?.getBestName().orEmpty(),


Should we inject ActiveSessionHolder instead of Session here?

I don't think so, the factory is not a singleton and we should not be able to display a timeline if there is no session, it will fail in a previous step in that case

mnaturel · 2022-10-26T08:15:43Z

.../im/vector/app/features/home/room/detail/timeline/item/MessageVoiceBroadcastListeningItem.kt

@@ -128,49 +60,34 @@ abstract class MessageVoiceBroadcastListeningItem : AbsMessageItem<MessageVoiceB
                VoiceBroadcastPlayer.State.PLAYING -> {
                    playPauseButton.setImageResource(R.drawable.ic_play_pause_pause)
                    playPauseButton.contentDescription = view.resources.getString(R.string.a11y_play_voice_broadcast)


I think content description should be R.string.a11y_pause_voice_broadcast here no?

Yes, good catch

mnaturel · 2022-10-26T08:17:13Z

.../im/vector/app/features/home/room/detail/timeline/item/MessageVoiceBroadcastListeningItem.kt

@@ -128,49 +60,34 @@ abstract class MessageVoiceBroadcastListeningItem : AbsMessageItem<MessageVoiceB
                VoiceBroadcastPlayer.State.PLAYING -> {
                    playPauseButton.setImageResource(R.drawable.ic_play_pause_pause)
                    playPauseButton.contentDescription = view.resources.getString(R.string.a11y_play_voice_broadcast)
-                    playPauseButton.onClick { attributes.callback?.onTimelineItemAction(RoomDetailAction.VoiceBroadcastAction.Listening.Pause) }
+                    playPauseButton.onClick { callback?.onTimelineItemAction(RoomDetailAction.VoiceBroadcastAction.Listening.Pause) }
                }
                VoiceBroadcastPlayer.State.IDLE,
                VoiceBroadcastPlayer.State.PAUSED -> {
                    playPauseButton.setImageResource(R.drawable.ic_play_pause_play)
                    playPauseButton.contentDescription = view.resources.getString(R.string.a11y_pause_voice_broadcast)


Here I think content description should be R.string.a11y_play_voice_broadcast.

mnaturel · 2022-10-26T08:23:58Z

.../im/vector/app/features/home/room/detail/timeline/item/MessageVoiceBroadcastRecordingItem.kt

        }
    }

+    private fun renderPlayingState(holder: Holder) = with(holder) {


Maybe to be consistent, we could rename the method to renderRecordingState?

mnaturel · 2022-10-26T08:26:56Z

vector/src/main/java/im/vector/app/features/voicebroadcast/VoiceBroadcastPlayer.kt

+    /**
+     * Map voiceBroadcastId to listeners.
+     */
+    private var listeners: MutableMap<String, CopyOnWriteArrayList<Listener>> = mutableMapOf()


It could be a val instead.

ElementBot · 2022-10-26T09:16:19Z

	Warnings
⚠️	vector/src/main/res/drawable/ic_voice_broadcast.xml#L7 - Very long vector path (1082 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.
⚠️	vector/src/main/res/drawable/ic_voice_broadcast.xml#L7 - Very long vector path (1082 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.
⚠️	vector/src/main/res/drawable/ic_voice_broadcast.xml#L10 - Very long vector path (1022 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.
⚠️	vector/src/main/res/drawable/ic_voice_broadcast.xml#L10 - Very long vector path (1022 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.
⚠️	vector/src/main/res/drawable/ic_voice_broadcast.xml#L13 - Very long vector path (997 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.
⚠️	vector/src/main/res/drawable/ic_voice_broadcast.xml#L13 - Very long vector path (997 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.
⚠️	vector/src/main/res/drawable/ic_voice_broadcast.xml#L16 - Very long vector path (928 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.
⚠️	vector/src/main/res/drawable/ic_voice_broadcast.xml#L16 - Very long vector path (928 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.

Generated by 🚫 dangerJS against 1554d79

sonarqubecloud · 2022-10-26T09:34:14Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

Florian Renaud added 7 commits October 25, 2022 16:31

Group voice broadcast controller buttons in a Flow

6a88c61

Timeline - Add abstraction on voice broadcast items

1566adb

Voice Broadcast - Add style for the "live" indicator

4defc3d

Improve VoiceBroadcastItemFactory

a7c24b2

Improve recording state rendering if app has been relaunched

2c14461

Rename renderLiveIcon method

f31429c

Move voice broadcast item attributes to dedicated class

649747b

Florian14 requested review from a team and jonnyandrew and removed request for a team October 25, 2022 14:58

Florian Renaud added 2 commits October 25, 2022 17:38

Fix kdoc issue

5130975

Add changelog

0f21f40

giomfo requested a review from mnaturel October 25, 2022 16:34

mnaturel reviewed Oct 26, 2022

View reviewed changes

Florian Renaud added 3 commits October 26, 2022 10:45

Fix wrong content description

6091ec4

Rename method renderPlayingState to renderRecordingState

8fe3b5e

Change listeners Map variable to immutable

1554d79

mnaturel approved these changes Oct 26, 2022

View reviewed changes

Florian14 merged commit 6f1e0b5 into resilience-rc Oct 26, 2022

Florian14 deleted the feature/fre/voice_broadcast_timeline_improvements branch October 26, 2022 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Voice Broadcast - Improve timeline rendering code #7448

Voice Broadcast - Improve timeline rendering code #7448

Florian14 commented Oct 25, 2022

mnaturel Oct 26, 2022

Florian14 Oct 26, 2022

mnaturel Oct 26, 2022 •

edited

Loading

Florian14 Oct 26, 2022

mnaturel Oct 26, 2022

mnaturel Oct 26, 2022

mnaturel Oct 26, 2022

ElementBot commented Oct 26, 2022

sonarqubecloud bot commented Oct 26, 2022

Voice Broadcast - Improve timeline rendering code #7448

Voice Broadcast - Improve timeline rendering code #7448

Conversation

Florian14 commented Oct 25, 2022

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

mnaturel Oct 26, 2022

Choose a reason for hiding this comment

Florian14 Oct 26, 2022

Choose a reason for hiding this comment

mnaturel Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Florian14 Oct 26, 2022

Choose a reason for hiding this comment

mnaturel Oct 26, 2022

Choose a reason for hiding this comment

mnaturel Oct 26, 2022

Choose a reason for hiding this comment

mnaturel Oct 26, 2022

Choose a reason for hiding this comment

ElementBot commented Oct 26, 2022

sonarqubecloud bot commented Oct 26, 2022

mnaturel Oct 26, 2022 •

edited

Loading