-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
MOBILE-212: Highlight current playing song & dynamic PlayButton #518
base: main
Are you sure you want to change the base?
Conversation
|
||
init { | ||
updatePlayerPosition() | ||
viewModelScope.launch(Dispatchers.IO) { | ||
brainzPlayerServiceConnection.currentPlayingSong.collectLatest { song -> |
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.
no need to collect the flow and emit to another flow.
val currentlyPlayingTitle = brainzPlayerServiceConnection.currentPlayingSong.map { it.title }
Also, rather than just flowing the title here, can we do brainzPlayerServiceConnection.currentPlayingSong
only and use title wherever needed.
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.
Yeah, that’s better.
Removing the redundant flow.
@@ -172,6 +179,7 @@ class BrainzPlayerViewModel @Inject constructor( | |||
viewModelScope.launch { songRepository.updateSong(mediaItem) } | |||
brainzPlayerServiceConnection.transportControls.playFromMediaId(mediaItem.mediaID.toString(), null) | |||
} | |||
_currentlyPlayingTitle.value = mediaItem.title |
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.
no need to emit here, service connection should automatically update the above flow. If not, it would be a bug for BP Service connection that would need to be resolved.
) { | ||
val isPlaying by viewModel.isPlaying.collectAsState() | ||
val currentlyPlayingTitle by viewModel.currentlyPlayingTitle.collectAsState() | ||
val titleColor = if (currentlyPlayingTitle == title) { |
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.
Have you tried using some ID of sorts like mediaID
of a song? That would be a better candidate for this kind of use case.
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 tried to match the currentlyPlayingSong.id
with the mediaId
passed to the BrainzPlayerListenCard
from SongsOverviewScreen
, but that didn't work out.
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.
wait I figured out a way to use mediaId
instead of the Title
val isPlaying by viewModel.isPlaying.collectAsState() | ||
val currentlyPlayingTitle by viewModel.currentlyPlayingTitle.collectAsState() | ||
val titleColor = if (currentlyPlayingTitle == title) { | ||
Color(0xFFB94FE5) |
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.
Use ListenBrainzTheme.colorScheme.signatureInverse
maybe or is it not legible enough?
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.
If I’m not wrong, lbSignatureInverse
uses lb_orange
, but Aerozol asked for a combination of blue and violet. Please suggest any combination or shade you’d like.
This PR focuses on highlighting the title of the currently playing song and changing the icon in
BrainzPlayerListenCard
.Changes:
currentlyPlayingTitle
with the title passed to theBrainzPlayerListenCard
composable. If they match, the title color will be changed; otherwise, it uses the default text color from the theme.BrainzPlayerListenCard
Screenshots:
Issue Link: MOBILE-212
This is the current implementation of feature. I’d love to hear your feedback or suggestions for improvement.