Voice Broadcast - Improve timeline rendering code #7448
Conversation
Florian14
requested review from
a team and
jonnyandrew
and removed request for
a team
| val voiceBroadcastAttributes = AbsMessageVoiceBroadcastItem.Attributes( | ||
| voiceBroadcastId = voiceBroadcastId, | ||
| voiceBroadcastState = voiceBroadcastContent.voiceBroadcastState, | ||
| recorderName = params.event.root.stateKey?.let { session.getUserOrDefault(it) }?.toMatrixItem()?.getBestName().orEmpty(), |
There was a problem hiding this comment.
Should we inject ActiveSessionHolder instead of Session here?
There was a problem hiding this comment.
I don't think so, the factory is not a singleton and we should not be able to display a timeline if there is no session, it will fail in a previous step in that case
| @@ -128,49 +60,34 @@ abstract class MessageVoiceBroadcastListeningItem : AbsMessageItem<MessageVoiceB | |||
| VoiceBroadcastPlayer.State.PLAYING -> { | |||
| playPauseButton.setImageResource(R.drawable.ic_play_pause_pause) | |||
| playPauseButton.contentDescription = view.resources.getString(R.string.a11y_play_voice_broadcast) | |||
There was a problem hiding this comment.
I think content description should be R.string.a11y_pause_voice_broadcast here no?
| @@ -128,49 +60,34 @@ abstract class MessageVoiceBroadcastListeningItem : AbsMessageItem<MessageVoiceB | |||
| VoiceBroadcastPlayer.State.PLAYING -> { | |||
| playPauseButton.setImageResource(R.drawable.ic_play_pause_pause) | |||
| playPauseButton.contentDescription = view.resources.getString(R.string.a11y_play_voice_broadcast) | |||
| playPauseButton.onClick { attributes.callback?.onTimelineItemAction(RoomDetailAction.VoiceBroadcastAction.Listening.Pause) } | |||
| playPauseButton.onClick { callback?.onTimelineItemAction(RoomDetailAction.VoiceBroadcastAction.Listening.Pause) } | |||
| } | |||
| VoiceBroadcastPlayer.State.IDLE, | |||
| VoiceBroadcastPlayer.State.PAUSED -> { | |||
| playPauseButton.setImageResource(R.drawable.ic_play_pause_play) | |||
| playPauseButton.contentDescription = view.resources.getString(R.string.a11y_pause_voice_broadcast) | |||
There was a problem hiding this comment.
Here I think content description should be R.string.a11y_play_voice_broadcast.
| } | ||
| } | ||
|
|
||
| private fun renderPlayingState(holder: Holder) = with(holder) { |
There was a problem hiding this comment.
Maybe to be consistent, we could rename the method to renderRecordingState?
| /** | ||
| * Map voiceBroadcastId to listeners. | ||
| */ | ||
| private var listeners: MutableMap<String, CopyOnWriteArrayList<Listener>> = mutableMapOf() |
There was a problem hiding this comment.
It could be a val instead.
|
|
SonarCloud Quality Gate failed. |
Florian14
deleted the
feature/fre/voice_broadcast_timeline_improvements
branch
Type of change
Content
Motivation and context
Improve the code and manage unhandled state
Screenshots / GIFs
(Non-exhaustive screenshots, rendering is almost the same as before, only some padding changes)
Tests
Play with voice broadcast and observe timeline update according to the recording & listening state changes
Tested devices
Checklist
developresilience-rc branch