-
Notifications
You must be signed in to change notification settings - Fork 754
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 Broadcast - Improve live indicator icon rendering #7579
Voice Broadcast - Improve live indicator icon rendering #7579
Conversation
a9b1637
to
288fc35
Compare
Also, it might be a bit subjective, but the debounced clicks on the -30s and +30s buttons might be a bit too 'eager'? I hit the -30s several times trying to go back ~1:30min and it seemed to get 'stuck', I just realised what was happening because I'm familiar with our 'debouncedClicks` function in the codebase, but at first it seemed like some weird bug. |
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.
Since 2 of the issues I mentioned are not strictly related to the new feature and the other one seems to be expected behaviour, I'd say it LGTM. Just a couple of suggestions.
private fun updateLiveListeningMode(seekPosition: Int? = null) { | ||
isLiveListening = when { | ||
// the current voice broadcast is not live (ended) | ||
!currentVoiceBroadcastEvent?.isLive.orFalse() -> false |
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.
Little nit pick here: I'd either use .not()
at the end or .isLive == false
, the current version makes the starting !
easy to miss.
val seekDirection = seekPosition.compareTo(getCurrentPlaybackPosition() ?: 0) | ||
when { | ||
// backward | ||
seekDirection < 0 -> false |
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.
Oh, that's what caused one of the issues I mentioned. I guess it's a expected behaviour then.
when { | ||
// backward | ||
seekDirection < 0 -> false | ||
// forward: check if new sequence is the last one |
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.
Nit: last
-> latest
?
@jmartinesp Thanks for the review, I took your feedback into account:
About the buffering, I thought it was fixed but I also reproduced it, I still have an issue related to the chunks recovery in my player, I will consider it in a dedicated bugfix. |
SonarCloud Quality Gate failed. |
Type of change
Content
Improve the rendering of the "live" indicator icon in the timeline.
Fixed a bug on the playlist recovery which was handling events from a potential previous voice broadcast.
Minor cleanup.
Motivation and context
Continue #7127
The live indicator icon is not updated according to the following rules:
Screenshots / GIFs
voice_broadcast_live_indicator.mp4
Tests
Tested devices
Checklist