-
Notifications
You must be signed in to change notification settings - Fork 784
Add additional data in voice broadcast events #7397
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
Add additional data in voice broadcast events #7397
Conversation
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.
Some small remarks
@@ -98,7 +100,10 @@ class StartVoiceBroadcastUseCase @Inject constructor( | |||
attachment = audioType.toContentAttachmentData(isVoiceMessage = true), | |||
compressBeforeSending = false, | |||
roomIds = emptySet(), |
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.
Is it ok to have roomIds empty 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.
Yes, it is necessary only for sending media to several rooms, plus the current one. So in this case it is fine. see 06ba478#diff-3bf6c59da8f4c1cf912366c300a0b7f3a5e6696795a5667db6cb9bf5823fd7a5R55
text: CharSequence, | ||
msgType: String = MessageType.MSGTYPE_TEXT, | ||
autoMarkdown: Boolean = false, | ||
additionalContent: Content? = null, |
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's a bit weird to have this available in all methods but I guess its acceptable.
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.
Yes, I was not sure but internally we thought that the sdk should allow adding custom fields for any event content following the Matrix specification. So, I added this field for each entry.
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 for me then
@@ -95,7 +95,6 @@ class StartVoiceBroadcastUseCase @Inject constructor( | |||
"Voice Broadcast Part ($sequence).${voiceMessageFile.extension}" | |||
) | |||
val audioType = outputFileUri.toMultiPickerAudioType(context) ?: return | |||
// TODO put sequence in event content |
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.
+1
Just did two small fixes before merging |
Can you look at the failing tests? |
f8261c1
to
90803be
Compare
894c3e8
to
0781ee8
Compare
SonarCloud Quality Gate failed. |
Type of change
Content
Add
io.element.voice_broadcast_chunk
in voice message contentAdd
device_id
inio.element.voice_broadcast_info
state eventAdd entries in matrix-sdk to add additional content fields to the events
Motivation and context
Continue #7127 and follow the "specification" defined in element-hq/element-meta#632
Screenshots / GIFs
Tests
Tested devices
Checklist