Voice Draft support #4527

ouchadam · 2021-11-19T17:14:21Z

Continuation of @onurays #4237 rebased on all of the refactors around the VoiceMessageRecorderView and MessageComposerViewModel refactors

This doesn't include moving any logic to the SDK yet but all the Text + Voice draft logic sits in one place under the MessageComposerViewModel
Also fixes [Bug|Crash] Voice Messages sending need more robustfully #3833 which was caused by only using a single file for all voice recordings, the fix is to simply use a unique file per voice recording

BEFORE	AFTER

ouchadam · 2021-11-19T17:15:54Z

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

+    private fun handleEntersBackground(composerText: String) {
+        withState {
+            if (it.isVoiceRecording) {
+                handleEndAllVoiceActions(deleteRecord = false)?.toContentAttachmentData()?.let { voiceDraft ->


in order to move all of the voice draft logic to the SDK, we'd also need to move the voice helpers, is that what you had in mind @bmarty ?

Not really, I would like the SDK be able to manage the draft file storage itself, but not the logic. We can discuss this on Monday.

I could imagine something like...

override fun initializeRecord(roomId: String) { outputFile = draftService.readFile(key = roomId) } override fun startRecord(roomId: String) { outputFile = draftService.createFile(key = roomId) }

github-actions · 2021-11-19T18:08:18Z

Unit Test Results

  66 files ±0   66 suites ±0 58s ⏱️ -10s
135 tests ±0 135 ✔️ ±0 0 💤 ±0 0 ❌ ±0
418 runs ±0 418 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 9939298. ± Comparison against base commit 61faf71.

♻️ This comment has been updated with latest results.

DoM1niC · 2021-11-19T18:26:28Z

This will fix @ouchadam ? #3833

in my case this could be reproduce in flight mode and more than one sendet Voice Messages after back online only one will be success send and the other show resend error state but couldn't be resend .... and have to dismiss and new recorded...

Bug in voice-composer Commit:
voice-composer changes brick the Message edit Stuff here with the Info you cant edit Messages while Recording a Voice Message :D ^^

…d being able to edit messages

ouchadam · 2021-11-22T12:57:44Z

This will fix @ouchadam ? #3833

in my case this could be reproduce in flight mode and more than one sendet Voice Messages after back online only one will be success send and the other show resend error state but couldn't be resend .... and have to dismiss and new recorded...

Not yet, but I have been able to reproduce the issue locally thanks to your steps, I'm looking into the cause 🤞

Bug in voice-composer Commit: voice-composer changes brick the Message edit Stuff here with the Info you cant edit Messages while Recording a Voice Message :D ^^

great catch! 5687c36

BEFORE	AFTER

EDIT: turns out the offline messages fix was quite simple! 6e8c545

BEFORE	AFTER

…starting a recording, which means any previous unsent recordings get deleted

ouchadam · 2021-11-22T14:20:22Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/send/UserDraft.kt

+    val content: String
+    val messageType: String
+
+    data class REGULAR(override val content: String, override val messageType: String) : UserDraft


I'm wondering if Voice should be a dedicated draft type rather than a messageType sub type 🤔

I think the code would be clearer with a VOICE data class (they are allcaps...). But I do not know if we will end up with many other cases. @onurays WDYT?

it would also mean we can avoid any database entity changes

Good catch. That make sense and yes we are preserving the draftMode in DraftEntity. So no migration will be needed.

ouchadam · 2021-11-22T17:08:14Z

vector/src/main/java/im/vector/app/features/voice/AbstractVoiceRecorder.kt

+    override fun startRecord(roomId: String) {
        init()
-        outputFile = File(outputDirectory, "Voice message.$filenameExt")
+        val fileName = "Voice message-${UUID.randomUUID()}.$filenameExt"


using unique files for the recording fixes #3833

The filename can in some circumstance be visible to the user, is it possible to use a subfolder instead, and keep a human readable filename?

I'll have a look, out of interest do we need to display the real file name here, could the label be hardcoded?

Yes, this is another way to fix the display issue. But also the file can be downloaded, etc. so a human readable filename should still be nice.

c2ec157

We can use the file provider to provide a human readable name, downloads automatically append a timestamp

NAME DOWNLOAD

bmarty

Nice work! Still some open question about who is responsible to save the draft voice file.

bmarty · 2021-11-22T17:16:42Z

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

            when (mode) {
-                is SendMode.REGULAR -> renderRegularMode(mode.text)
+                is SendMode.REGULAR -> {
+                    if (mode.messageType == MessageType.MSGTYPE_AUDIO) {


This is where the code is not typed enough IMO, MSGTYPE_AUDIO is not necessarily a voice message. But for draft this is the only possible case I think...

I think this is a sign to move away from the MSGTYPE and have a separate Voice send mode

bmarty · 2021-11-22T17:20:28Z

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

+                handleEndAllVoiceActions(deleteRecord = false)?.toContentAttachmentData()?.let { voiceDraft ->
+                    handleSaveDraft(draft = voiceDraft.toJsonString(), messageType = MessageType.MSGTYPE_AUDIO)
+                }
+                setState { it.copy(voiceRecordingUiState = VoiceMessageRecorderView.RecordingUiState.None) }


Is it worth (re-)setting the state when entering Background? What about screen rotation?

this state update isn't needed, will remove and for rotation the recording stops and the playback composer is displayed until sent or deleted

4b67a6d

Nice to have: the recording should not stop on screen rotation... (maybe handle that in another PR)

Not stopping the recording will probably mean moving the recording/playback to a service (it would also enable recording/playing whilst the screen is off or in other screens), would need its own ticket/issue

The rotation is already a bit funny in develop~

DEVELOP ROTATION PR - RECORDING ROTATION PR - DRAFT ROTATION

EDIT: we also lose some of the state during rotation because the VoiceMessagePlaybackTracker and the ViewModel creation isn't tied to the ViewModelFactory but instead the fragment/activity

bmarty · 2021-11-22T17:21:00Z

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

+        withState {
+            if (it.isVoiceRecording) {
+                handleEndAllVoiceActions(deleteRecord = false)?.toContentAttachmentData()?.let { voiceDraft ->
+                    handleSaveDraft(draft = voiceDraft.toJsonString(), messageType = MessageType.MSGTYPE_AUDIO)


Here maybe give the voice file to the SDK (?)

I'll give it a go 🤞

if we pass the file it means the SDK will have parse the vector Uri https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/features/home/room/detail/composer/VoiceMessageHelper.kt#L78 which includes interacting with the multipicker

the current recording and draft file are the same instance, the draft itself is the audio file metadata and the uri reference back to the file, other than creating the initial file and reopening I'm not sure we can move much else to the SDK (without moving more of the voice logic)

bmarty · 2021-11-22T17:23:18Z

vector/src/main/java/im/vector/app/features/voice/AbstractVoiceRecorder.kt

+        val fileName = "Voice message-${UUID.randomUUID()}.$filenameExt"
+        val outputDirectoryForRoom = File(outputDirectory, roomId.md5()).apply {
+            if (!exists()) {
+                mkdirs()


mkdirs() do the existing check, do you remember :)

…ecording stops

ouchadam · 2021-11-23T09:27:32Z

closing whilst addressing comments

…update the waveform once the view is ready

… send type

… experience

…agment

…ent provider to provide a human readable display name

ouchadam · 2021-11-23T16:05:40Z

noticed some more oddities with rotation, closing again :(

…rkaround to inject into the viewmodel, fixes multiple instances being created out of sync when rotating

…le exists at a previously created folder directory

ouchadam · 2021-11-23T18:25:35Z

closing again in favour of breaking this down into smaller PRs #4548 #4547

Onuray Sahin and others added 11 commits November 19, 2021 16:58

Extend DraftEntity to be able to handle multiple message types.

1c8fd30

Fix draft entity migration.

3cc57f2

Save draft with message type.

eaf063c

Render draft message.

e97c065

Do not create redundant temp files.

a459692

Changelog added.

dfd6884

Code review fixes.

e5865bf

removing unused import

bfdacfe

using sealed interface to avoid duplicated constructor calls

2d81260

handling voice drafts via the message composer view model

a345f26

adding dedicated migration for the draft message type

95e5c12

ouchadam commented Nov 19, 2021

View reviewed changes

classifying the draft mode as the voice recorder being active to avoi…

5687c36

…d being able to edit messages

ouchadam added 2 commits November 22, 2021 13:06

fixing #3833, it's caused by us always using the same file name when …

6e8c545

…starting a recording, which means any previous unsent recordings get deleted

fixing line length

e790091

Base automatically changed from feature/adm/voice-composer to develop November 22, 2021 14:01

ouchadam marked this pull request as ready for review November 22, 2021 14:03

ouchadam added 2 commits November 22, 2021 14:05

adding changelog entry

b9b51af

removing duplicated migration due to rebase

00b954f

ouchadam commented Nov 22, 2021

View reviewed changes

This was referenced Nov 22, 2021

[Bug|Crash] Voice Messages sending need more robustfully #3833

Closed

VoiceMessage buglist / improvements from the PR #3770

Open

ouchadam commented Nov 22, 2021

View reviewed changes

bmarty reviewed Nov 22, 2021

View reviewed changes

ouchadam added 3 commits November 22, 2021 17:30

removing redundant exists() checks, mkdirs already does this

cd4e991

removing redundant state update, the state already updates when the r…

4b67a6d

…ecording stops

removing unused base property and fixing sealed class casing

7655c25

ouchadam added 2 commits November 22, 2021 18:16

removing unused message types

94161b2

using dedicated Voice types instead of message sub type

517a0d6

ouchadam closed this Nov 23, 2021

ouchadam added 4 commits November 23, 2021 12:22

replacing flaky post check with a doOnLayout, this allows us to only …

c11bc2d

…update the waveform once the view is ready

replacing message type entity change with a dedicated voice draft and…

60ed9ec

… send type

merging the playback and draft states together, they're the same user…

bf968cc

… experience

replacing cancelled state with None, lifts the vibration up to the fr…

f360021

…agment

ouchadam reopened this Nov 23, 2021

renaming recording ui states to better align to the user facing states

bb2239c

ouchadam force-pushed the feature/adm/voice-draft branch from 3c5832d to bb2239c Compare November 23, 2021 12:48

ouchadam added 2 commits November 23, 2021 13:04

using random file name for the voice message files and using the cont…

c2ec157

…ent provider to provide a human readable display name

inlining single use playback views

be103fc

ouchadam closed this Nov 23, 2021

ouchadam added 2 commits November 23, 2021 16:17

making the playback tracker a singleton to avoid using the factory wo…

ff2dc71

…rkaround to inject into the viewmodel, fixes multiple instances being created out of sync when rotating

parsing the file path from the content uri instead of assuming the fi…

9939298

…le exists at a previously created folder directory

ouchadam reopened this Nov 23, 2021

This was referenced Nov 23, 2021

Timeline editing and offline voice message fixes #4547

Merged

Voice related sealed class cleanup #4548

Merged

ouchadam closed this Nov 23, 2021

ouchadam mentioned this pull request Nov 24, 2021

Adding support for voice drafts #4558

Merged

Voice Draft support #4527

Voice Draft support #4527

Uh oh!

Conversation

ouchadam commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

DoM1niC commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ouchadam commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam commented Nov 23, 2021

Uh oh!

ouchadam commented Nov 19, 2021 •

edited

Loading

github-actions bot commented Nov 19, 2021 •

edited

Loading

DoM1niC commented Nov 19, 2021 •

edited

Loading

ouchadam commented Nov 22, 2021 •

edited

Loading

ouchadam Nov 22, 2021 •

edited

Loading

ouchadam Nov 23, 2021 •

edited

Loading

ouchadam Nov 22, 2021 •

edited

Loading