-
Notifications
You must be signed in to change notification settings - Fork 464
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
Update dbus_mpris to rspotify 0.12.0 #1220
Conversation
rspotify 0.11.7 switched completely from std Duration to chrono::Duration, so update our usage accordingly. 0.11.7 also add Collectionyourepisodes to the Type enum, so add it to uri_to_id.
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.
Looks good to me. If you want, have a look at the one comment, I left.
I currently don't have the means to test the changes, but as long as CI passes that should be fine.
src/dbus_mpris.rs
Outdated
let _ = | ||
sp_client.seek_track(new_pos as u32, playback.device.id.as_deref()); | ||
let _ = sp_client.seek_track( | ||
Duration::milliseconds(new_pos), |
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.
Haven't thought about it properly, but now that they're using Durations for seek_track
(again?), would it maybe be nicer to convert our input (new_pos
) to a Duration
and do the addition with the Duration
s?
and also resolve lint errors related to try_into() Signed-off-by: Bailey Kasin <baileykasin@gmail.com>
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.
That looks good to me, just one last comment. Then we should be good to go.
Looking at CI, it seems like we might need a MSRV bump? Would you be able to update that in the CI as well as in the Cargo.toml? |
Signed-off-by: Bailey Kasin <baileykasin@gmail.com>
It looks like the version of |
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.
Thank you!
To be honest my main motivation for this is that my
cargo
isn't obeying the .toml and keep pulling rspotify 0.11.7 which breaks mpris, and now that 0.12.0 is released it feels like it makes more sense to update to that than to 0.11.7.0.11.7 switched completely from std::time::Duration to chrono::Duration, so update our usage accordingly.
0.11.7 also added Collectionyourepisodes to the Type enum, so add it to uri_to_id.
0.12.0 added an optional Market for country code, which for now is just to None in the one spot it was required to be set.