Audio files in the timeline now appear with the audio player #5586

ericdecanini · 2022-03-19T12:01:04Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Instead of appearing as a simple downloadable file, audio files uploaded in the timeline will now be displayed with the audio player so they are playable from the timeline

I took this opportunity to improve our UX with the way the timeline handles playing multiple audio files (by that I mean playing one while another one is playing so the other pauses, etc)

The seekbar is a little fiddly though. Clicking on it works nicely to seek and so does swiping right, but swiping left doesn't work (not before already being in a seeking state) because it conflicts with the ItemTouchHelper of the recycler view which already has a swipe left action being handled. Due to this being on the level of the recycler view, I couldn't find a way to handle this from within the context of the audio item

Motivation and context

Closes #2525

Screenshots / GIFs

Light Mode	Dark Mode

Tests

Audio Files

Upload an audio file in the timeline
See that it displays differently from a regular file attachment and plays the audio
See that the seekbar works
Try uploading a second audio file and see that they interact nicely with each other
Try backgrounding and foregrounding the app and see that no strange behaviour is found

Tested devices

Physical
Emulator
OS version(s): Android 10, 12

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

github-actions · 2022-03-19T12:30:03Z

Unit Test Results

110 files +  4 110 suites +4 1m 20s ⏱️ -13s
195 tests +  7 195 ✔️ +  7 0 💤 ±0 0 ❌ ±0
650 runs +28 650 ✔️ +28 0 💤 ±0 0 ❌ ±0

Results for commit d9f2033. ± Comparison against base commit 1097436.

This pull request removes 2 and adds 9 tests. Note that renamed tests count towards both.

im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given registration has started and has dummy step to do, when handling action, then ignores other steps and executes dummy
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ when registering account, then updates state and emits account created event

im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given direct authentication throws, when logging in directly, then returns failure result with original cause
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given wellknown fails with content, when logging in directly, then returns success with direct session result
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given wellknown fails without content, when logging in directly, then returns well known error
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given wellknown throws, when logging in directly, then returns failure result with original cause
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ when logging in directly, then returns success with direct session result
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given has sign in with matrix id sign mode, when handling login or register action fails, then emits error
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given has sign in with matrix id sign mode, when handling login or register action, then logs in directly
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given personalisation enabled and registration has started and has dummy step to do, when handling action, then ignores other steps and executes dummy
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given personalisation enabled, when registering account, then updates state and emits account created event

♻️ This comment has been updated with latest results.

…sages

…-files-player # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt

ericdecanini · 2022-03-22T07:27:12Z

@daniellekirkwood @amshakal can I ask you guys to take a look at this new UI?

amshakal · 2022-03-24T13:06:02Z

Tagging @gaelledel who worked on voice messages UI.

…-files-player # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/composer/VoiceMessageHelper.kt # vector/src/main/java/im/vector/app/features/home/room/detail/composer/voice/VoiceMessageViews.kt # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/MessageVoiceItem.kt

…-files-player # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt

mnaturel

Good work! I have only small remarks. When testing, I have seen something strange. I play an audio file in a room. Then I press back which leads me to previous screen. It pauses the audio but does not reset it to the start. It may appear strange to users since they have no way to restart at the beginning of the audio except restarting the app. It may be a side effect of the bug you fixed when going in background.

I have also a question about UI: Could we indicate somewhere near the filename the total duration of the audio track? When we start playing we loose that information and since there is currently no seekbar we don't really know where we are in the audio track.

mnaturel · 2022-03-28T12:21:55Z

changelog.d/5586.feature

@@ -0,0 +1 @@
+Adds the ability for audio attachments to be played in the timeline


Should we use the Github issue number instead for the Changelog entry? I know there were some discussions about it, but I don't know what was decided. I think all the team should use use the same pattern: either Github issue number or PR number.

When I learned about this process, I heard it's very ambiguous and we go either way, but I agree that one way or the other we should choose one and be consistent with it!

mnaturel · 2022-03-28T12:26:10Z

vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt

+        val nonFormattedBody = when (messageContent) {
+            is MessageAudioContent -> getAudioContentBodyText(messageContent)
+            is MessagePollContent  -> messageContent.getBestPollCreationInfo()?.question?.getBestQuestion()
+            else                   -> messageContent?.body ?: ""


There is the orEmpty extension we can use here to replace Elvis operator. I found it easier to read:
messageContent?.body.orEmpty(). What do you think?

mnaturel · 2022-03-28T12:47:11Z

...src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt

+            informationData: MessageInformationData,
+            highlight: Boolean,
+            attributes: AbsMessageItem.Attributes,
+    ) = if (messageContent.voiceMessageIndicator != null) {


Maybe we could write the if/else in reverse to avoid negative condition which tends to be more difficult to read? Like this:

if (messageContent.voiceMessageIndicator == null) { buildAudioMessageItem(params, messageContent, informationData, highlight, attributes) } else { buildVoiceMessageItem(params, messageContent, informationData, highlight, attributes) }

I can see the Uncle Bob here ✨ Clean Code never mentions this in the case of null checks (and I can't find anything online about it), but imo I think != null is actually positive, because it's akin to "if this data exists"

mnaturel · 2022-03-28T12:54:11Z

...src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt

-                messageContent.encryptedFileInfo?.toElementToDecrypt())
-            )
+            .audioMessagePlaybackTracker(audioMessagePlaybackTracker)
+            .isLocalFile(localFilesHelper.isLocalFile(fileUrl))
            .mxcUrl(fileUrl)


I see we no more set the isDownloaded attribute. Was it intended? I am wondering since it still exists in the MessageVoiceItem class. And if it was not intended, should we have the same attribute in MessageAudioItem?

Good spot! isDownloaded in MessageVoiceItem isn't actually being used. I'll push changes to remove it from that class.

It's used only in MessageFileItem to determine the icon that's shown, but in the case of voice and audio items this is replaced by the play button

mnaturel · 2022-03-28T13:02:36Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/MessageAudioItem.kt

+    }
+
+    private fun bindUploadState(holder: Holder) {
+        if (!attributes.informationData.sendState.hasFailed()) {


Same remark about the condition, it is better to use positive condition when possible, in that case, we would have:

if (attributes.informationData.sendState.hasFailed()) { holder.audioPlaybackControlButton.setImageResource(R.drawable.ic_cross) holder.audioPlaybackControlButton.contentDescription = holder.view.context.getString(R.string.error_audio_message_unable_to_play, filename) holder.progressLayout.isVisible = false } else { contentUploadStateTrackerBinder.bind(attributes.informationData.eventId, isLocalFile, holder.progressLayout) }

mnaturel · 2022-03-28T13:15:46Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/MessageVoiceItem.kt

@@ -55,10 +55,12 @@ abstract class MessageVoiceItem : AbsMessageItem<MessageVoiceItem.Holder>() {
    var waveform: List<Int> = emptyList()

    @EpoxyAttribute
-    var izLocalFile = false
+    @JvmField
+    var isLocalFile = false


Should we really keep these fields which you annotated with @JvmFields. We could use private const instead or directly the boolean value where it is called. Same remark for the field isLocalFile in MessageAudioItem. What do you think?

These can't be const as they're set via epoxy, but the reason for adding @JvmField here is Epoxy returns a compile error without it.

What I find weird is that this only happened after renaming it to fix the spelling. The reasoning for that is something that completely baffles me 🤷

I see. Here is the explanation: airbnb/epoxy#459. The question is now, should we rename the fields with the @JvmField annotation or keep using the iz prefix. It seems iz prefix is used for booleans in Epoxy model in the project. @bmarty Can you confirm?

Ahh this explains it! Imo it is pretty ugly to mispell our own variables just to avoid this. @JvmField isn't great but I think it's a better compromise

I agree. I prefer good naming as well.

mnaturel · 2022-03-28T13:20:56Z

vector/src/main/res/layout/item_timeline_event_audio_stub.xml

@@ -0,0 +1,69 @@
+<?xml version="1.0" encoding="utf-8"?>
+<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"


Couldn't we reuse the existing item_timeline_event_voice_stub or are there any differences?

Pretty major differences. This stub is the screenshot you see above in the pr description

ericdecanini · 2022-03-29T14:24:07Z

Good work! I have only small remarks. When testing, I have seen something strange. I play an audio file in a room. Then I press back which leads me to previous screen. It pauses the audio but does not reset it to the start. It may appear strange to users since they have no way to restart at the beginning of the audio except restarting the app. It may be a side effect of the bug you fixed when going in background.

I have also a question about UI: Could we indicate somewhere near the filename the total duration of the audio track? When we start playing we loose that information and since there is currently no seekbar we don't really know where we are in the audio track.

Good catch with that bug! It should be an easy fix so I'll push that in a bit.

With that UI suggestion, that is a good point. I would like to know what design thinks about it

…tween activity states

mnaturel

Good for me. I will let you see with design team if you want to add total duration of the audio track somewhere near the title.

gaelledel

Cool! Just need to follow the specs for audio files available in compound here https://www.figma.com/file/X4XTH9iS2KGJ2wFKDqkyed/Compound?node-id=2039%3A26421

gaelledel

Yes except we need to remove the underline from the file title please :)

mnaturel

Nice work! We now have a real audio player, hope it will be appreciated by users! I have just a question about the place of some ViewActions.

mnaturel · 2022-04-06T07:28:34Z

vector/src/main/java/im/vector/app/features/home/room/detail/composer/MessageComposerAction.kt

@@ -42,4 +42,5 @@ sealed class MessageComposerAction : VectorViewModelAction {
    data class EndAllVoiceActions(val deleteRecord: Boolean = true) : MessageComposerAction()
    data class VoiceWaveformTouchedUp(val eventId: String, val duration: Int, val percentage: Float) : MessageComposerAction()
    data class VoiceWaveformMovedTo(val eventId: String, val duration: Int, val percentage: Float) : MessageComposerAction()
+    data class AudioSeekBarMovedTo(val eventId: String, val duration: Int, val percentage: Float) : MessageComposerAction()


I am wondering if this is really an action of the MessageComposerView? Does it handle the move of the seek bar inside the timeline or when composing a new message? The question is the same for VoiceWaveformTouchedUp and VoiceWaveformMovedTo. And maybe others but I don't have all the understanding of the feature.

You make a strong point! It doesn't sound like this and some of the other voice actinos like the ones you mentioned and ones concerning play/pause functionality should belong here. Perhaps I could work it in another PR to separate these out a little bit

bmarty

My 2 cents

bmarty · 2022-04-07T09:39:51Z

vector/src/main/java/im/vector/app/features/home/room/detail/composer/AudioMessageHelper.kt

+            mediaPlayer?.pause()
+            playbackTracker.updatePausedAtPlaybackTime(id, toMillisecond, percentage)
+            stopPlaybackTicker()
+        }


👀 thanks!

bmarty · 2022-04-07T09:42:38Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/MessageVoiceItem.kt

@@ -55,10 +55,12 @@ abstract class MessageVoiceItem : AbsMessageItem<MessageVoiceItem.Holder>() {
    var waveform: List<Int> = emptyList()

    @EpoxyAttribute
-    var izLocalFile = false
+    @JvmField
+    var isLocalFile = false


bmarty · 2022-04-07T09:43:50Z

vector/src/main/res/values/strings.xml

@@ -2860,6 +2860,14 @@
    <string name="error_voice_message_cannot_reply_or_edit">Cannot reply or edit while voice message is active</string>
    <string name="voice_message_reply_content">Voice Message (%1$s)</string>

+    <string name="a11y_audio_message_item">%1$s, %2$s, %3$s</string> <!-- filename, duration, file size -->


Comments have to be above the string for Weblate to handle it properly.

Added this to #5714

ericdecanini added 4 commits March 17, 2022 16:40

Changes unTrack to untrack

00bced9

Fixes wrong spellings in Message file and voice items

cde10ca

Fixes epoxy errors with renamed epoxy attributes

2617632

Uses messageVoiceItem for audio files

d54b465

ericdecanini added 8 commits March 19, 2022 18:16

Refactors MessageAudioItem to work for both audio files and voice mes…

fab78c9

…sages

Adapts special body text for audio messages

2e716cb

Renames functions and variables

7a7d36d

Adds new audio timeline stub

ff26829

Merge remote-tracking branch 'origin/develop' into feature/eric/audio…

5a819bb

…-files-player # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt

Code cleanup

c9946b6

Adds changelog file

06468a4

Fixes import lint error

b035911

ericdecanini marked this pull request as ready for review March 21, 2022 19:05

ericdecanini requested review from a team and mnaturel and removed request for a team March 21, 2022 19:06

ericdecanini added the X-Needs-Design May require input from the design team label Mar 22, 2022

Removes ic_headphones

249db18

amshakal requested a review from gaelledel March 24, 2022 13:06

ericdecanini added 7 commits March 24, 2022 20:32

Fixes post merge errors

806af47

Merge remote-tracking branch 'origin/develop' into feature/eric/audio…

c8a56d6

…-files-player # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt

Fixes textview layout bounds in item_timeline_event_audio_stub

313595e

Fixes lint error

8aaaf80

Improves accessibility talkback on MessageAudioItem

931c0e9

Fixes playback button state not tracking after onPause

fa56a5e

mnaturel reviewed Mar 28, 2022

View reviewed changes

ericdecanini added 3 commits March 29, 2022 16:25

Improves code formatting

cc5e8f3

Fixes bugs related to audio message duration being set incorrectly be…

6adf487

…tween activity states

Fixes bug where audio can be played before waveform is ready

8ad4f20

ericdecanini requested a review from mnaturel March 29, 2022 17:56

mnaturel approved these changes Mar 30, 2022

View reviewed changes

gaelledel suggested changes Mar 31, 2022

View reviewed changes

ericdecanini removed the X-Needs-Design May require input from the design team label Apr 1, 2022

ericdecanini added 4 commits April 1, 2022 15:15

Adds layout with seek bar to audio stub

34dcd70

Adds proper handling of audio seek bar

d0155c9

Readds onPermissionDeniedSnackbar import in TimelineFragment

76b2cfd

Improves accessibility on MessageAudioItem

dc36301

ericdecanini requested review from gaelledel and mnaturel April 5, 2022 15:01

Updates some text sizes and colours in audio timeline item

355db98

gaelledel approved these changes Apr 5, 2022

View reviewed changes

ericdecanini added 2 commits April 6, 2022 07:25

Removes underline in audio filename

4159850

Removes unused import

d9f2033

bmarty mentioned this pull request Apr 6, 2022

[Release] Element Android v1.4.10 - v1.4.11 #5699

Closed

59 tasks

mnaturel reviewed Apr 6, 2022

View reviewed changes

ericdecanini mentioned this pull request Apr 7, 2022

Quickly Resolvable Tech Debt #5714

Open

ericdecanini merged commit 45104f8 into develop Apr 7, 2022

ericdecanini deleted the feature/eric/audio-files-player branch April 7, 2022 09:25

bmarty reviewed Apr 7, 2022

View reviewed changes

ericdecanini mentioned this pull request Jun 7, 2022

Trying to send mp3 audio file crashes app #5265

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audio files in the timeline now appear with the audio player #5586

Audio files in the timeline now appear with the audio player #5586

ericdecanini commented Mar 19, 2022 •

edited

Loading

github-actions bot commented Mar 19, 2022 •

edited

Loading

ericdecanini commented Mar 22, 2022

amshakal commented Mar 24, 2022

mnaturel left a comment

mnaturel Mar 28, 2022

ericdecanini Mar 29, 2022

mnaturel Mar 28, 2022

mnaturel Mar 28, 2022

ericdecanini Mar 29, 2022

mnaturel Mar 28, 2022

ericdecanini Mar 29, 2022

mnaturel Mar 28, 2022

mnaturel Mar 28, 2022

ericdecanini Mar 29, 2022

mnaturel Mar 29, 2022 •

edited

Loading

ericdecanini Mar 29, 2022

mnaturel Mar 30, 2022

bmarty Apr 7, 2022

mnaturel Mar 28, 2022 •

edited

Loading

ericdecanini Mar 29, 2022

ericdecanini commented Mar 29, 2022

mnaturel left a comment

gaelledel left a comment

gaelledel left a comment

mnaturel left a comment

mnaturel Apr 6, 2022

ericdecanini Apr 7, 2022

bmarty left a comment

bmarty Apr 7, 2022

bmarty Apr 7, 2022

bmarty Apr 7, 2022

ericdecanini Apr 7, 2022

		@@ -0,0 +1 @@
		Adds the ability for audio attachments to be played in the timeline

		@@ -0,0 +1,69 @@
		<?xml version="1.0" encoding="utf-8"?>
		<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"

Audio files in the timeline now appear with the audio player #5586

Audio files in the timeline now appear with the audio player #5586

Conversation

ericdecanini commented Mar 19, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Mar 19, 2022 • edited Loading

Unit Test Results

ericdecanini commented Mar 22, 2022

amshakal commented Mar 24, 2022

mnaturel left a 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

mnaturel Mar 29, 2022 • edited Loading

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 Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericdecanini commented Mar 29, 2022

mnaturel left a comment

Choose a reason for hiding this comment

gaelledel left a comment

Choose a reason for hiding this comment

gaelledel left a comment

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty left a 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

ericdecanini commented Mar 19, 2022 •

edited

Loading

github-actions bot commented Mar 19, 2022 •

edited

Loading

mnaturel Mar 29, 2022 •

edited

Loading

mnaturel Mar 28, 2022 •

edited

Loading