-
-
Notifications
You must be signed in to change notification settings - Fork 521
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
pick random song from tracklist #339
pick random song from tracklist #339
Conversation
780ec70
to
50400ab
Compare
50400ab
to
b368296
Compare
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.
This looks like a useful feature, thank you @BKitor!
I've left a few comments that should simplify the implementation a little. Take a look and let me know if I've preserved the functionality 👍
At the end of the review, I've rewritten the play_random_song
function with all my requested changes.
src/handlers/track_table.rs
Outdated
@@ -149,6 +152,108 @@ pub fn handler(key: Key, app: &mut App) { | |||
} | |||
} | |||
|
|||
fn play_random_song(app: &mut App) { | |||
let TrackTable { context, .. } = &app.track_table; |
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.
We can reduce the nesting here by avoiding destructuring e.g.
if let Some(context) = &app.track_table.context {
match context {
@Rigellute Thanks for the advice! I appreciate it! |
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.
Thanks for pointing out that we do need to deserialise the total number of tracks in a playlist.
I've just left a few more comments - main one is about removing unwrap
, what do you think?
src/handlers/track_table.rs
Outdated
@@ -149,6 +152,107 @@ pub fn handler(key: Key, app: &mut App) { | |||
} | |||
} | |||
|
|||
fn play_random_song(app: &mut App) { | |||
// let TrackTable { context, .. } = &app.track_table; |
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.
Are you able to remove this commented line?
src/handlers/track_table.rs
Outdated
.library | ||
.made_for_you_playlists | ||
.get_results(Some(0)) | ||
.unwrap() |
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.
Are you able to remove these unwrap
s? Something like this?
TrackTableContext::MadeForYou => {
if let Some(playlist) = &app
.library
.made_for_you_playlists
.get_results(Some(0))
.and_then(|playlist| playlist.items.get(app.made_for_you_index))
{
if let Some(num_tracks) = &playlist
.tracks
.get("total")
.and_then(|total| -> Option<usize> { from_value(total.clone()).ok() })
{
let uri = Some(playlist.uri.clone());
app.dispatch(IoEvent::StartPlayback(
uri,
None,
Some(thread_rng().gen_range(0, num_tracks)),
));
}
};
}
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.
This works great! I didn't know and_then() existed. Thanks for the tip.
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.
Great work @BKitor, thank you
@all-contributors please add @BKitor for code |
I've put up a pull request to add @BKitor! 🎉 |
When using Spotify I often use their Shuffle Playlist button. Spotify-tui doesn't seem to have that kind of feature. What if Shift-s on Track Table chooses a random song from whatever context is active, and plays it.