Voice Broadcast - Improve recording and listening tiles in the timeline #7421

Florian14 · 2022-10-20T17:10:23Z

Type of change

WIP Feature
Bugfix
Technical
Other :

Content

Display dedicated tiles in the timeline to distinguish a recording VB from a listening one inspired according to the provided design

Motivation and context

Continue #7127

Screenshots / GIFs

Tests

Play with the voice broadcast and verify that the UI is updated as expected from the recorder and the listener perspective (live, paused, etc...)

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

bmarty

Just a few remarks, else LGTM.

bmarty · 2022-10-20T19:14:29Z

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

+                .drawableProvider(drawableProvider)
+                .voiceBroadcastRecorder(voiceBroadcastRecorder)
+                .leftGuideline(avatarSizeProvider.leftGuideline)
+                .callback(callback)


You need to call .id(...). There are lots of ...Item_() without id in the project though, this is strange.

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

bmarty · 2022-10-20T19:21:23Z

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

+                    recordButton.setImageDrawable(drawable)
+                    recordButton.contentDescription = holder.view.resources.getString(R.string.a11y_pause_voice_broadcast_record)
+                    recordButton.setOnClickListener { attributes.callback?.onTimelineItemAction(VoiceBroadcastAction.Recording.Pause) }
+                    stopRecordButton.setOnClickListener { attributes.callback?.onTimelineItemAction(VoiceBroadcastAction.Recording.Stop) }


You can use onClick { which include a debouncer. Applicable to all setOnClickListener in the Item

bmarty · 2022-10-20T19:24:32Z

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

        }
    private var currentRoomId: String? = null
+    private var listeners = mutableListOf<Listener>()


Better to use a CopyOnWriteArrayList

bmarty · 2022-10-20T19:25:06Z

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


-    override var listener: VoiceBroadcastRecorder.Listener? = null
+    private val listeners = mutableListOf<VoiceBroadcastRecorder.Listener>()


Better to use a CopyOnWriteArrayList

bmarty · 2022-10-20T19:28:52Z

...or/src/main/java/im/vector/app/features/voicebroadcast/usecase/StartVoiceBroadcastUseCase.kt

+            override fun onVoiceMessageCreated(file: File, sequence: Int) {
+                sendVoiceFile(room, file, eventId, sequence)
+            }
+        })


You need to remove the listener somewhere.

I did not remove it from here because it will be removed when the record will be stopped from StopVoiceBroadcastUseCase (stopping a record cause the cleaning of the listeners).

yostyle · 2022-10-20T21:28:09Z

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

@@ -27,5 +27,5 @@ object VoiceBroadcastConstants {
    const val VOICE_BROADCAST_CHUNK_KEY = "io.element.voice_broadcast_chunk"

    /** Default voice broadcast chunk duration, in seconds. */
-    const val DEFAULT_CHUNK_LENGTH_IN_SECONDS = 120
+    const val DEFAULT_CHUNK_LENGTH_IN_SECONDS = 10


Please undo this change

giomfo

SGTM

ElementBot · 2022-10-20T22:01:10Z

	Warnings
⚠️	vector/src/main/res/drawable/ic_voice_broadcast_16.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_16.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_16.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_16.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_16.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_16.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_16.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_16.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/layout/item_timeline_event_view_stubs_container.xml#L2 - This `<FrameLayout>` can be replaced with a `<merge>` tag
⚠️	vector/src/main/res/layout/item_timeline_event_view_stubs_container.xml#L2 - This `<FrameLayout>` can be replaced with a `<merge>` tag
⚠️	vector/src/main/res/layout/item_timeline_event_voice_broadcast_listening_stub.xml#L59 - This tag and its children can be replaced by one `<TextView/>` and a compound drawable
⚠️	vector/src/main/res/layout/item_timeline_event_voice_broadcast_listening_stub.xml#L59 - This tag and its children can be replaced by one `<TextView/>` and a compound drawable
⚠️	vector/src/main/res/layout/item_timeline_event_voice_broadcast_listening_stub.xml#L86 - This tag and its children can be replaced by one `<TextView/>` and a compound drawable
⚠️	vector/src/main/res/layout/item_timeline_event_voice_broadcast_listening_stub.xml#L86 - This tag and its children can be replaced by one `<TextView/>` and a compound drawable

Generated by 🚫 dangerJS against 926f4d9

sonarqubecloud · 2022-10-20T22:51:45Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

1.5% Coverage
0.0% Duplication

bmarty reviewed Oct 20, 2022

View reviewed changes

Florian14 force-pushed the feature/fre/voice_broadcast_timeline_tiles branch from ec2041b to 5cf52ad Compare October 20, 2022 20:39

Florian14 force-pushed the feature/fre/voice_broadcast_live_listening branch from 1004496 to 0a9f2bf Compare October 20, 2022 21:03

Florian Renaud added 12 commits October 20, 2022 23:28

VoiceBroadcast - Add recording view

4c71209

VoiceBroadcast - Introduce listening view

f1b4ebb

VoiceBroadcast - Listening view

f711a0e

VoiceBroadcast - Update live indicator icon

cebc096

Fix null voiceBroadcastId when the playlist is empty

9c5b6d8

Update buffering view

6ff7a7f

Fix voice broadcast state update on wrong thread

72a1ace

Add additional information in listening tile

930c856

Add changelog

8869d82

Set id to VoiceBroadcast items

9a96de4

Call onClick instead of setOnClickListener

f2cc082

Use CopyOnWriteArrayList

4a76998

yostyle reviewed Oct 20, 2022

View reviewed changes

Add missing content description

34cafa3

Florian14 force-pushed the feature/fre/voice_broadcast_timeline_tiles branch from f21f753 to 34cafa3 Compare October 20, 2022 21:43

giomfo approved these changes Oct 20, 2022

View reviewed changes

Base automatically changed from feature/fre/voice_broadcast_live_listening to develop October 20, 2022 21:52

Fix play/pause button disabled

926f4d9

Florian14 merged commit 2a977f3 into develop Oct 20, 2022

Florian14 deleted the feature/fre/voice_broadcast_timeline_tiles branch October 20, 2022 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Voice Broadcast - Improve recording and listening tiles in the timeline #7421

Voice Broadcast - Improve recording and listening tiles in the timeline #7421

Florian14 commented Oct 20, 2022 •

edited

Loading

bmarty left a comment

bmarty Oct 20, 2022

bmarty Oct 20, 2022

bmarty Oct 20, 2022

bmarty Oct 20, 2022

bmarty Oct 20, 2022

Florian14 Oct 20, 2022

yostyle Oct 20, 2022

giomfo left a comment

ElementBot commented Oct 20, 2022 •

edited

Loading

sonarqubecloud bot commented Oct 20, 2022


		override var listener: VoiceBroadcastRecorder.Listener? = null
		private val listeners = mutableListOf<VoiceBroadcastRecorder.Listener>()

Voice Broadcast - Improve recording and listening tiles in the timeline #7421

Voice Broadcast - Improve recording and listening tiles in the timeline #7421

Conversation

Florian14 commented Oct 20, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

bmarty left a comment

Choose a reason for hiding this comment

bmarty Oct 20, 2022

Choose a reason for hiding this comment

bmarty Oct 20, 2022

Choose a reason for hiding this comment

bmarty Oct 20, 2022

Choose a reason for hiding this comment

bmarty Oct 20, 2022

Choose a reason for hiding this comment

bmarty Oct 20, 2022

Choose a reason for hiding this comment

Florian14 Oct 20, 2022

Choose a reason for hiding this comment

yostyle Oct 20, 2022

Choose a reason for hiding this comment

giomfo left a comment

Choose a reason for hiding this comment

ElementBot commented Oct 20, 2022 • edited Loading

sonarqubecloud bot commented Oct 20, 2022

Florian14 commented Oct 20, 2022 •

edited

Loading

ElementBot commented Oct 20, 2022 •

edited

Loading