Skip to content
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

Merged
merged 8 commits into from Oct 19, 2022

Conversation

Florian14
Copy link
Contributor

Type of change

  • WIP Feature
  • Bugfix
  • Technical
  • Other :

Content

Add io.element.voice_broadcast_chunk in voice message content
Add device_id in io.element.voice_broadcast_info state event
Add 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

  • Physical
  • Emulator
  • OS version(s):

Checklist

@Florian14 Florian14 marked this pull request as ready for review October 18, 2022 14:40
@Florian14 Florian14 requested review from a team and ganfra and removed request for a team October 18, 2022 14:41
@Florian14 Florian14 added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Oct 18, 2022
Copy link
Contributor

@ganfra ganfra left a 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(),
Copy link
Contributor

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?

Copy link
Contributor Author

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,
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@ganfra ganfra left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@Florian14
Copy link
Contributor Author

Just did two small fixes before merging

@ganfra
Copy link
Contributor

ganfra commented Oct 18, 2022

Can you look at the failing tests?

@Florian14 Florian14 force-pushed the feature/fre/voice_broadcast_labs_flag branch from f8261c1 to 90803be Compare October 18, 2022 19:08
@Florian14 Florian14 force-pushed the feature/fre/voice_broadcast_additional_content branch from 894c3e8 to 0781ee8 Compare October 18, 2022 19:09
@sonarcloud
Copy link

sonarcloud bot commented Oct 18, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

4.0% 4.0% Coverage
0.0% 0.0% Duplication

Base automatically changed from feature/fre/voice_broadcast_labs_flag to develop October 19, 2022 05:50
@Florian14 Florian14 merged commit 1397266 into develop Oct 19, 2022
@Florian14 Florian14 deleted the feature/fre/voice_broadcast_additional_content branch October 19, 2022 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants