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.

Sign up for GitHub

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

Jump to bottom

Voice Broadcast - Send state events #7273

Merged
merged 15 commits into from
Oct 7, 2022
Merged

Voice Broadcast - Send state events #7273

merged 15 commits into from
Oct 7, 2022

Conversation

Florian14
Copy link
Contributor

@Florian14 Florian14 commented Oct 3, 2022 •
edited
Loading

Type of change

  • WIP Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Add the io.element.voice_broadcast_info state event related to Voice Broadcast (by message chunking) element-meta#632
  • Add use cases to play/pause/resume/stop a voice broadcast (by just sending the correct state events, without recording for the moment)
  • Add a minimalist view in the timeline related to this state event

Motivation and context

Continue #7127

Screenshots / GIFs

image

Tests

  • Enable the voice broadcast feature flag
  • Start a voice broadcast from the message composer actions
  • Verify that a widget is displayed in the timeline
  • Play with the widget buttons and verify that the voice broadcast state is correctly updated

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@Florian14 Florian14 force-pushed the feature/fre/voice_broadcast_state_event branch 5 times, most recently from a619173 to 1f23162 Compare October 3, 2022 09:13
@Florian14 Florian14 requested review from a team and bmarty and removed request for a team October 3, 2022 09:14
@Florian14 Florian14 marked this pull request as ready for review October 3, 2022 09:15
Base automatically changed from feature/fre/voice_broadcast_feature_flag to develop October 4, 2022 09:25
bmarty
bmarty approved these changes Oct 4, 2022
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, only small remarks!

vector/src/main/java/im/vector/app/features/home/room/detail/TimelineViewModel.kt Show resolved Hide resolved
...src/main/java/im/vector/app/features/home/room/detail/timeline/factory/MessageItemFactory.kt Outdated Show resolved Hide resolved
...main/java/im/vector/app/features/home/room/detail/timeline/item/MessageVoiceBroadcastItem.kt Outdated Show resolved Hide resolved
...rc/main/java/im/vector/app/features/voicebroadcast/model/MessageVoiceBroadcastInfoContent.kt
@Json(name = "m.new_content") override val newContent: Content? = null,

/** The [VoiceBroadcastState] value. **/
@Json(name = "state") val voiceBroadcastStateStr: String = "",
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for using a String here.

...r/src/main/java/im/vector/app/features/voicebroadcast/usecase/ResumeVoiceBroadcastUseCase.kt Outdated

Timber.d("## ResumeVoiceBroadcastUseCase: Resume voice broadcast requested")

val lastVoiceBroadcastEvent = room.stateService().getStateEvent(STATE_ROOM_VOICE_BROADCAST_INFO, QueryStringValue.Equals(session.myUserId))
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if we could have a object which observe the state, to always have the latest state event of the voice broadcast per room. Maybe this will be needed later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about it, I will see if I do it in this PR or improve it in a future PR (I am still looking for the implementation)

Copy link
Member

Choose a reason for hiding this comment

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

I suggest considering this later

...r/src/main/java/im/vector/app/features/voicebroadcast/usecase/ResumeVoiceBroadcastUseCase.kt Outdated Show resolved Hide resolved
...or/src/main/java/im/vector/app/features/voicebroadcast/usecase/StartVoiceBroadcastUseCase.kt

private suspend fun startVoiceBroadcast(room: Room) {
Timber.d("## StartVoiceBroadcastUseCase: Send new voice broadcast info state event")
room.stateService().sendStateEvent(
Copy link
Member

Choose a reason for hiding this comment

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

This method can throw (no network for instance). Maybe protect by a try/catch. (applicable to the other similar calls)

Copy link
Contributor Author

@Florian14 Florian14 Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't find a better way than catching the whole execute function: b286a52

Copy link
Member

Choose a reason for hiding this comment

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

@bmarty I let you review this commit to resolve or not this conversation here

Copy link
Member

Choose a reason for hiding this comment

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

ok, this is fine.

vector/src/main/java/im/vector/app/features/voicebroadcast/usecase/StopVoiceBroadcastUseCase.kt Outdated Show resolved Hide resolved
giomfo
giomfo approved these changes Oct 5, 2022
View reviewed changes
Copy link
Member

@giomfo giomfo left a comment

Choose a reason for hiding this comment

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

SGTM

@Florian14 Florian14 force-pushed the feature/fre/voice_broadcast_state_event branch from b06c2af to 07f0069 Compare October 5, 2022 08:01
@Florian14
Copy link
Contributor Author

Force pushed to rebase on develop and remove a commit from the state events aggregation which is the purpose of another PR

@Florian14 Florian14 force-pushed the feature/fre/voice_broadcast_state_event branch from 987b506 to 688c908 Compare October 6, 2022 12:45
@Florian14 Florian14 force-pushed the feature/fre/voice_broadcast_state_event branch from 688c908 to b286a52 Compare October 6, 2022 14:35
@ElementBot
Copy link

Warnings
⚠️

vector/src/main/res/layout/item_timeline_event_view_stubs_container.xml#L2 - This <FrameLayout> can be replaced with a <merge> tag
⚠️

vector/src/main/res/layout/item_timeline_event_view_stubs_container.xml#L2 - This <FrameLayout> can be replaced with a <merge> tag

Generated by 🚫 dangerJS against b286a52

@sonarcloud
Copy link

sonarcloud bot commented Oct 6, 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 6 Code Smells

59.1% 59.1% Coverage
0.0% 0.0% Duplication

@Florian14 Florian14 merged commit 48d2cc4 into develop Oct 7, 2022
@Florian14 Florian14 deleted the feature/fre/voice_broadcast_state_event branch October 7, 2022 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giomfo giomfo giomfo approved these changes

@bmarty bmarty bmarty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Florian14 @ElementBot @bmarty @giomfo