-
Notifications
You must be signed in to change notification settings - Fork 743
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Voice Message - Draft support #4237
Conversation
LGTM so far |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work.
Some first small remarks, will test it later.
views.composerLayout.collapse() | ||
views.composerLayout.setTextIfDifferent(text) | ||
views.composerLayout.views.sendButton.contentDescription = getString(R.string.send) | ||
private fun renderRegularMode(content: String, messageType: String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The body content does not match the name of this fun anymore.
Is it possible to not change this fun and create a new one like renderVoiceMessageMode()?
And test the value of messageType at the call site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// We should improve the UX to support going into playback mode when paused and delete the media when the view is destroyed. | ||
roomDetailViewModel.handle(RoomDetailAction.EndAllVoiceActions(deleteRecord = false)) | ||
views.voiceMessageRecorderView.initVoiceRecordingViews() | ||
roomDetailViewModel.handle( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not talk to the textComposerViewModel anymore here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
roomDetailViewModel will decide it and communicate with textComposerViewModel later with SaveDraft event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
views.voiceMessageRecorderView.initVoiceRecordingViews() | ||
roomDetailViewModel.handle( | ||
RoomDetailAction.OnRoomDetailEntersBackground( | ||
isVoiceMessageActive = views.voiceMessageRecorderView.isActive() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is strange to ask the view about the state. The ViewModel should be aware of the current state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it complex to determine the state here, especially if there is already a draft in the room.
override fun startRecord() { | ||
init() | ||
outputFile = File(outputDirectory, "Voice message.$filenameExt") | ||
val fileName = """${UUID.randomUUID()}.$filenameExt""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filename can be visible to the user, it will be a bit weird.
Is it possible to create a folder per room?
Also saving voice message draft could be handled by the Matrix SDK, using the DraftService
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I proposed a way to keep drafts in a roomId based directory structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
CI is not happy, can you handle that please? Thanks |
?.transform { | ||
it.setString(DraftEntityFields.MESSAGE_TYPE, MessageType.MSGTYPE_TEXT) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a problem for those who are already on version 19. In this case you should create a version 20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry I think this has to be modified.
I would like the voice messages to be saved as draft by the SDK, using DraftService
.
It will simplify the code I think and make it more robust, by following our current architecture.
What do you think about it?
@bmarty from my understanding we are storing the draft via the I'm unfamiliar with the area, do you have more insight into the type of change you're expecting? |
val audioJsonString = audioType?.toContentAttachmentData()?.toJsonString() | ||
_viewEvents.post(RoomDetailViewEvents.SaveDraft(audioJsonString, MessageType.MSGTYPE_AUDIO)) | ||
} else { | ||
_viewEvents.post(RoomDetailViewEvents.SaveDraft(null, MessageType.MSGTYPE_TEXT)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird arch
going to close this PR whilst working on it |
If a user leaves the room detail screen without sending or deleting the ongoing voice message record then we need to save this record as a draft message and render it again in playback mode when the user enters the room detail screen again.
EDIT
Fixes #3922