Enable seeking a recorded voice message #1758

jonnyandrew · 2023-11-07T16:27:57Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Enable seeking the voice message player after recording a voice message.

Motivation and context

Part of [Story] Scrub the playback position of a voice message element-meta#2113

Screenshots / GIFs

Screen_recording_20231107_162647.webm

Tests

Record a voice message
Seek the preview player by tapping the waveform
Play a timeline voice message
Try seeking the preview player again in the same way

Tested devices

Physical
Emulator
OS version(s): 13

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-x-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-x-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

jonnyandrew · 2023-11-07T16:29:35Z

...lement/android/features/messages/voicemessages/composer/VoiceMessageComposerPresenterTest.kt

@@ -323,6 +340,7 @@ class VoiceMessageComposerPresenterTest {
            awaitItem().eventSink(VoiceMessageComposerEvents.PlayerEvent(VoiceMessagePlayerEvent.Play))
            awaitItem().eventSink(VoiceMessageComposerEvents.SendVoiceMessage)
            assertThat(awaitItem().voiceMessageState).isEqualTo(aPlayingState().toSendingState())
+            skipItems(1) // Duplicate sending state


I needed to add this after this change but couldn't get to the bottom of why. The state flow still emits the exact same values in this test.

github-actions · 2023-11-07T16:33:40Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/gS8gHB

julioromano

Sorry if I'm being pedantic with changes to MediaPlayer.
The intent is to try to keep it as slim and simple as possible because that's the part which interfaces with the ExoPlayer beast which is missing tests.
So the more stuff we add here the more undetected issues we might have in the future.
We should probably think of building some tests so we can more freely enhance MediaPlayer but it is potentially cumbersome (currently in the tech debt list).

...diaplayer/impl/src/main/kotlin/io/element/android/libraries/mediaplayer/impl/SimplePlayer.kt

...player/impl/src/main/kotlin/io/element/android/libraries/mediaplayer/impl/MediaPlayerImpl.kt

codecov · 2023-11-08T11:37:18Z

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (5d2770a) 63.36% compared to head (93cb444) 63.40%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1758      +/-   ##
===========================================
+ Coverage    63.36%   63.40%   +0.03%     
===========================================
  Files         1292     1292              
  Lines        33502    33568      +66     
  Branches      6971     6981      +10     
===========================================
+ Hits         21230    21284      +54     
- Misses        9086     9088       +2     
- Partials      3186     3196      +10

Files	Coverage Δ
.../impl/voicemessages/timeline/VoiceMessagePlayer.kt	`72.91% <100.00%> (ø)`
...ies/textcomposer/components/VoiceMessagePreview.kt	`74.32% <100.00%> (ø)`
.../libraries/voicerecorder/api/VoiceRecorderState.kt	`100.00% <ø> (ø)`
...messages/composer/VoiceMessageComposerPresenter.kt	`86.90% <90.90%> (-7.84%)`	⬇️
...icemessages/composer/VoiceMessageComposerPlayer.kt	`85.10% <83.90%> (+7.32%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sonarqubecloud · 2023-11-09T16:57:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

julioromano · 2023-11-10T08:17:49Z

...der/api/src/main/kotlin/io/element/android/libraries/voicerecorder/api/VoiceRecorderState.kt

 import java.io.File
 import kotlin.time.Duration

+@Immutable


Just for my info: What does this do? I mean, what wasn't working well before that prompted you to add this?

IIRC it was flagged as a problem when I added VoiceRecorderState as a parameter to a composable function here. It's a class from another module that wasn't built with the compose compiler so the stability couldn't be inferred (see here for more info).

julioromano · 2023-11-10T08:33:12Z

.../element/android/features/messages/impl/voicemessages/composer/VoiceMessageComposerPlayer.kt

+
+    private suspend fun MediaPlayer.ensureMediaReady(mediaPath: String): MediaPlayer.State {
+        val state = state.value
+        if (state.mediaId == mediaPath && state.isReady) {


Sorry about not checking for this properly in the MediaPlayer. I've opened this: #1783

jonnyandrew commented Nov 7, 2023

View reviewed changes

jonnyandrew marked this pull request as ready for review November 7, 2023 16:31

jonnyandrew requested a review from a team as a code owner November 7, 2023 16:31

jonnyandrew requested review from julioromano and removed request for a team November 7, 2023 16:31

julioromano reviewed Nov 8, 2023

View reviewed changes

jonnyandrew force-pushed the jonny/vm-seek-preview branch from 1120862 to 5a41a4e Compare November 8, 2023 21:23

jonnyandrew requested a review from julioromano November 9, 2023 10:04

Enable seeking a recorded voice message

93cb444

jonnyandrew force-pushed the jonny/vm-seek-preview branch from 94a9efa to 93cb444 Compare November 9, 2023 16:52

julioromano reviewed Nov 10, 2023

View reviewed changes

julioromano approved these changes Nov 10, 2023

View reviewed changes

jonnyandrew merged commit 14be887 into develop Nov 10, 2023
19 checks passed

jonnyandrew deleted the jonny/vm-seek-preview branch November 10, 2023 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable seeking a recorded voice message #1758

Enable seeking a recorded voice message #1758

jonnyandrew commented Nov 7, 2023

jonnyandrew Nov 7, 2023

github-actions bot commented Nov 7, 2023 •

edited

Loading

julioromano left a comment

codecov bot commented Nov 8, 2023 •

edited

Loading

sonarqubecloud bot commented Nov 9, 2023

julioromano Nov 10, 2023

jonnyandrew Nov 10, 2023 •

edited

Loading

julioromano Nov 10, 2023

Enable seeking a recorded voice message #1758

Enable seeking a recorded voice message #1758

Conversation

jonnyandrew commented Nov 7, 2023

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

jonnyandrew Nov 7, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 7, 2023 • edited Loading

julioromano left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 8, 2023 • edited Loading

Codecov Report

sonarqubecloud bot commented Nov 9, 2023

julioromano Nov 10, 2023

Choose a reason for hiding this comment

jonnyandrew Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

julioromano Nov 10, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 8, 2023 •

edited

Loading

jonnyandrew Nov 10, 2023 •

edited

Loading