Receive and play a voice message #1503

julioromano · 2023-10-06T08:33:54Z

Type of change

Feature
Bugfix
Technical
Other :

Content

This PR consists of several macro-blocks separated by path/package:

messages.impl.mediaplayer : Global (room-wide) media player, now used only for voice messages but could be used for all media within EX in the future. It is backed by media3's exoplayer. Currently not unit-tested because mocking exoplayer is not trivial.
messages.impl.voicemessages.play : Business logic of a timeline voice message. This is all the logic that manages the voice message bubble.
messages.impl.timeline.model & messages.impl.timeline.factories: Timeline code that takes care of creating the content object for voice messages.
messages.impl.timeline.components : The actual View composable that shows the UI inside a voice message bubble.

All the rest is just small related changes that must be done here and there in existing code.

From a high level perspective this is how it works:

Voice messages are unlike other message bubbles because they carry state (i.e. playing, downloading...) so they have a Presenter managing this state.
Media content (i.e. the ogg file) of a voice message is downloaded from the rust SDK on first play then stored in a voice messages cache (see the VoiceMessageCache class, it is just a subdirectory in the app's cacheDir which is indexed by the matrix content uri). All further play attempts are done from the cache without hitting the rust SDK anymore.
Playback of the ogg file is handled with the VoiceMessagePlayer class which is basically a "view" of the global MediaPlayer that allow the voice message to only see the media player state belonging to its media content.
Drawing of the waveform is done with an OSS library wrapped in the WaveformProgressIndicator composable.

Known issues:

The waveform has no position slider.
The waveform (and together with it the whole message bubble) is taller than the actual Figma design.
Swipe to reply for voice messages is disabled to avoid conflict with the audio scrubbing gesture (to reply to a voice message you have to use the long press menu).
The loading indicator is always shown (there is no delay).
Voice messages don't stop playing when redacted.

Motivation and context

element-hq/element-meta#2083

Screenshots / GIFs

Provided by Screenshot tests in the PR itself.

github-actions · 2023-10-06T08:44:16Z

📱 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/bEoNrU

DoM1niC · 2023-10-11T13:16:34Z

earpiece listening would be nice to have, but works fine here on my fork!

julioromano · 2023-10-11T20:02:59Z

earpiece listening would be nice to have, but works fine here on my fork!

Thanks for the feedback!!!

ganfra · 2023-10-12T16:03:46Z

.../kotlin/io/element/android/features/messages/impl/timeline/TimelineItemPresenterFactories.kt

+@Composable
+inline fun <reified C : TimelineItemEventContent, reified S : Any> TimelineItemPresenterFactories.rememberPresenter(
+    content: C
+): Presenter<S> = remember {


Should remember use key param?

I suppose you mean keying by content?
Right now every voice message gets its own presenter so I'd think this is working as intended.
Is there anything else I should check to ensure we're fine without a key?

It's more about handling content updates

codecov · 2023-10-21T15:21:10Z

Codecov Report

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

Comparison is base (9b027fc) 59.22% compared to head (b23f1d4) 59.26%.
Report is 9 commits behind head on develop.

❗ Current head b23f1d4 differs from pull request most recent head 41d4d1f. Consider uploading reports for the commit 41d4d1f to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1503      +/-   ##
===========================================
+ Coverage    59.22%   59.26%   +0.03%     
===========================================
  Files         1250     1262      +12     
  Lines        32327    32721     +394     
  Branches      6633     6715      +82     
===========================================
+ Hits         19147    19393     +246     
- Misses       10306    10418     +112     
- Partials      2874     2910      +36

Files	Coverage Δ
...eatures/messages/impl/actionlist/ActionListView.kt	`59.64% <100.00%> (+0.17%)`	⬆️
...line/factories/event/TimelineItemContentFactory.kt	`56.00% <100.00%> (ø)`
...ures/messages/impl/timeline/groups/Groupability.kt	`59.25% <100.00%> (+1.56%)`	⬆️
...l/timeline/model/event/TimelineItemVoiceContent.kt	`100.00% <100.00%> (ø)`
.../impl/voicemessages/timeline/VoiceMessageEvents.kt	`100.00% <100.00%> (ø)`
...s/impl/voicemessages/timeline/VoiceMessageState.kt	`100.00% <100.00%> (ø)`
...ndroid/features/messages/impl/MessagesPresenter.kt	`82.63% <0.00%> (-0.44%)`	⬇️
...l/timeline/model/event/TimelineItemEventContent.kt	`91.66% <50.00%> (-3.79%)`	⬇️
...tils/messagesummary/MessageSummaryFormatterImpl.kt	`50.00% <0.00%> (-3.34%)`	⬇️
...s/impl/voicemessages/timeline/VoiceMessageCache.kt	`93.75% <93.75%> (ø)`
... and 11 more

... and 3 files with indirect coverage changes

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

jonnyandrew

Looks good! I haven't gotten to review the tests yet. Here's my comments in the mean time

...io/element/android/features/messages/impl/timeline/components/event/TimelineItemVoiceView.kt

...n/io/element/android/features/messages/impl/timeline/model/event/TimelineItemVoiceContent.kt

jonnyandrew · 2023-10-24T15:50:15Z

...ment/android/features/messages/impl/timeline/model/event/TimelineItemVoiceContentProvider.kt

+        get() = sequenceOf(
+            aTimelineItemVoiceContent(body = "A sound.mp3"),
+            aTimelineItemVoiceContent(body = "A bigger name sound.mp3"),
+            aTimelineItemVoiceContent(body = "An even bigger bigger bigger bigger bigger bigger bigger sound name which doesn't fit .mp3"),


nit: Should we add previews with waveforms containing 0 items and greater than 50 items?

This provider is actually unused, I initially created it to match TimelineItemAudioContentProvider but then when adding VoiceMessagePresenter I opted to use VoiceMessageStateProvider in TimelineItemVoiceView instead.
Thing is a preview can only use one provider (instead here I'd love to have 2) so I kind of forgot about it over time.
I now created TimelineItemVoiceViewParametersProvider which combines the 2 so I can use that in the preview instead and added some previews based on your suggestions.

But I still think there is still work to do as refinement here, for instance: should we keep the separate content object or should we just embed it in the state and only expose the state on the view side? @ganfra I'd like to also have your take on this.

jonnyandrew · 2023-10-24T15:53:49Z

...kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessageCache.kt

+     *
+     * @return the file path of the voice message in the cache directory.
+     */
+    val cachePath: String


nit:

Suggested change

val cachePath: String

val cacheDir: File

That's a file path (as in full path to file e.g. /a/b/c/d/something.ogg) not a directory.
I get it was not clear enough from the docstring, is there a better way to explain it in English?

I see - I was a bit confused when I first saw the interface what it's for. I thought this is used to provide access to the file. But I think it's used to create the mediaUri here?

It's not a big problem if we need it in this format - happy to leave as is!

Both.
The idea is that we map one mxc:// content uri to one file in the cache via a simple mapping: "${cacheDir.path}/$CACHE_VOICE_SUBDIR/${mxcUri2FilePath(mxcUri)}" .

So once we have a content uri we can initialize an instance of this class and:

it will give us the path to the cache file using cachePath

it will tell us if the file at such path actually exists with isInCache() (i.e. cache hit) or not (i.e. cache miss)

it will give use the option to fill the cache with such content (moveToCache())

Perhaps we can find better method names?

jonnyandrew · 2023-10-24T15:56:52Z

...kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessageCache.kt

+ *
+ * See: https://spec.matrix.org/v1.8/client-server-api/#matrix-content-mxc-uris
+ */
+private val mxcRegex = Regex("""^mxc:\/\/([^\/]+)\/([^\/]+)$""")


Might this fit better in the libraries:matrix module?

Not sure, that module is only to interface with the rust sdk AFAIK. @bmarty any suggestions?

jonnyandrew · 2023-10-24T16:06:43Z

...in/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessagePresenter.kt

+                    if (playerState.isMyMedia) {
+                        if (playerState.isPlaying) {
+                            player.pause()
+                        } else {
+                            player.play()
+                        }
+                    } else {
+                        if (voiceCache.isInCache()) {
+                            player.acquireControlAndPlay()
+                        } else {
+                            scope.launch { downloadCacheAndPlay() }
+                        }
+                    }


The ownership aspect is a lot for the presenter to know about. Could the VoiceMessagePlayer not take care of some of this logic? Leaving us with something simpler like

if (!isDownloaded) { download() } if (isPlaying) { pause() } else { play() }

I agree, but I prefer to take some more time aside to make this better.

Of course, I don't think it should block this PR

...in/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessagePresenter.kt

jonnyandrew · 2023-10-24T16:14:02Z

...o/element/android/features/messages/impl/voicemessages/timeline/WaveformProgressIndicator.kt

+        onProgressChangeFinished = {
+            // This is to send just one onSeek callback after the user has finished seeking.
+            // Otherwise the AudioWaveform library would send multiple callbacks while the user is seeking.
+            val p = seekProgress!!


Instead of crashing with NPE can we log an error and fail silently?

If it's an important error, we could also track it to Sentry?

It is my understanding that onProgressChangeFinished will always be called after at least one invocation of onProgressChange so NPEs should be impossible here.

If it certainly is the case, no problem. Personally I would er on the side of caution!

...o/element/android/features/messages/impl/voicemessages/timeline/WaveformProgressIndicator.kt

...impl.timeline.components.event_null_TimelineItemVoiceView-D-39_39_null_1,NEXUS_5,1.0,en].png

julioromano · 2023-10-24T20:20:00Z

Looks good! I haven't gotten to review the tests yet. Here's my comments in the mean time

Pushed first batch of fixes, there are still a few to go.

ElementBot · 2023-10-24T20:29:09Z

	Warnings
⚠️	features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/actionlist/ActionListView.kt#L37 - Using a material import while also using the material3 library
⚠️	features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/actionlist/ActionListView.kt#L38 - Using a material import while also using the material3 library
⚠️	features/messages/impl/src/main/res/drawable/retry.xml#L7 - Very long vector path (1009 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.
⚠️	gradle/libs.versions.toml#L24 - A newer version of androidx.compose:compose-bom than 2023.10.00 is available: 2023.10.01
⚠️	gradle/libs.versions.toml#L64 - A newer version of com.google.android.material:material than 1.9.0 is available: 1.10.0
⚠️	gradle/libs.versions.toml#L101 - A newer version of androidx.compose.material3:material3 than 1.2.0-alpha09 is available: 1.2.0-alpha10
⚠️	gradle/libs.versions.toml#L136 - A newer version of com.google.testparameterinjector:test-parameter-injector than 1.13 is available: 1.14

Generated by 🚫 dangerJS against 41d4d1f

julioromano · 2023-10-24T20:38:04Z

Addressed all suggestions except: #1503 (comment)
For which I'd like to take some more time to come up with a better solution.

jonnyandrew

Thanks for the changes. I don't think we have any blocking issues here. Looking forward to merging this!

...ges/impl/src/test/kotlin/io/element/android/features/messages/mediaplayer/MediaPlayerFake.kt

...test/kotlin/io/element/android/features/messages/voicemessages/play/VoiceMessageCacheFake.kt

sonarcloud · 2023-10-24T21:27:11Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

julioromano self-assigned this Oct 6, 2023

julioromano mentioned this pull request Oct 6, 2023

[Story] Receive and play a voice message element-hq/element-meta#2083

Closed

7 tasks

julioromano force-pushed the julioromano/2084_receive_and_play_a_voice_message branch 10 times, most recently from 14f01ee to 0700c95 Compare October 10, 2023 19:41

julioromano force-pushed the julioromano/2084_receive_and_play_a_voice_message branch 4 times, most recently from dc91436 to 9f2b1da Compare October 12, 2023 15:31

ganfra reviewed Oct 12, 2023

View reviewed changes

julioromano force-pushed the julioromano/2084_receive_and_play_a_voice_message branch 10 times, most recently from b8f62f5 to f7f7e8a Compare October 17, 2023 14:26

julioromano force-pushed the julioromano/2084_receive_and_play_a_voice_message branch 3 times, most recently from a483409 to 7330dfc Compare October 21, 2023 14:31

julioromano marked this pull request as ready for review October 21, 2023 14:52

julioromano requested a review from a team as a code owner October 21, 2023 14:52

julioromano requested review from ganfra and jonnyandrew and removed request for a team October 21, 2023 14:52

julioromano force-pushed the julioromano/2084_receive_and_play_a_voice_message branch 2 times, most recently from 2695b13 to 2a1862e Compare October 23, 2023 09:57

julioromano mentioned this pull request Oct 23, 2023

Record and send voice messages #1596

Merged

14 tasks

julioromano force-pushed the julioromano/2084_receive_and_play_a_voice_message branch 2 times, most recently from d0cbf0d to c647bc7 Compare October 24, 2023 14:49

Receive and play a voice message

f2df7c5

julioromano force-pushed the julioromano/2084_receive_and_play_a_voice_message branch from c647bc7 to f2df7c5 Compare October 24, 2023 14:56

Update screenshots

b23f1d4

jonnyandrew reviewed Oct 24, 2023

View reviewed changes

Apply review suggestions

6b68445

Alt text

a8066bb

jonnyandrew approved these changes Oct 24, 2023

View reviewed changes

...ges/impl/src/test/kotlin/io/element/android/features/messages/mediaplayer/MediaPlayerFake.kt Outdated Show resolved Hide resolved

...test/kotlin/io/element/android/features/messages/voicemessages/play/VoiceMessageCacheFake.kt Outdated Show resolved Hide resolved

Apply some more suggestions.

a0722fa

julioromano enabled auto-merge (squash) October 24, 2023 20:51

Update screenshots

41d4d1f

julioromano merged commit 6e66c98 into develop Oct 24, 2023
13 checks passed

julioromano deleted the julioromano/2084_receive_and_play_a_voice_message branch October 24, 2023 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Receive and play a voice message #1503

Receive and play a voice message #1503

julioromano commented Oct 6, 2023 •

edited

github-actions bot commented Oct 6, 2023 •

edited

DoM1niC commented Oct 11, 2023

julioromano commented Oct 11, 2023

ganfra Oct 12, 2023

julioromano Oct 12, 2023

ganfra Oct 13, 2023

julioromano Oct 16, 2023

codecov bot commented Oct 21, 2023 •

edited

jonnyandrew left a comment

jonnyandrew Oct 24, 2023

julioromano Oct 24, 2023 •

edited

jonnyandrew Oct 24, 2023

julioromano Oct 24, 2023

jonnyandrew Oct 24, 2023

julioromano Oct 24, 2023

jonnyandrew Oct 24, 2023

julioromano Oct 24, 2023

jonnyandrew Oct 24, 2023

julioromano Oct 24, 2023

jonnyandrew Oct 24, 2023

jonnyandrew Oct 24, 2023

julioromano Oct 24, 2023

jonnyandrew Oct 24, 2023

julioromano commented Oct 24, 2023

ElementBot commented Oct 24, 2023 •

edited

julioromano commented Oct 24, 2023

jonnyandrew left a comment

sonarcloud bot commented Oct 24, 2023

Receive and play a voice message #1503

Receive and play a voice message #1503

Conversation

julioromano commented Oct 6, 2023 • edited

Type of change

Content

Motivation and context

Screenshots / GIFs

github-actions bot commented Oct 6, 2023 • edited

DoM1niC commented Oct 11, 2023

julioromano commented Oct 11, 2023

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

codecov bot commented Oct 21, 2023 • edited

Codecov Report

jonnyandrew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julioromano Oct 24, 2023 • 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

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

julioromano commented Oct 24, 2023

ElementBot commented Oct 24, 2023 • edited

julioromano commented Oct 24, 2023

jonnyandrew left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 24, 2023

julioromano commented Oct 6, 2023 •

edited

github-actions bot commented Oct 6, 2023 •

edited

codecov bot commented Oct 21, 2023 •

edited

julioromano Oct 24, 2023 •

edited

ElementBot commented Oct 24, 2023 •

edited