Conversation
f4a3b30 to
5e8fc7d
Compare
7fc4159 to
f6481fd
Compare
|
@roderickvd this PR can probably be considered ready. The only missing feature is to handle the track list. It can probably be done in another PR. Right now, CI is failing but doesn't seems to be related to this specific PR. Do you have an opinion concerning 59767ce which is a broader modification than just MPRIS support? |
wisp3rwind
left a comment
There was a problem hiding this comment.
Thanks for moving this forward!
I've looked through the PR superficially; here are a few comments. I didn't really check the actual implementation of MPRIS commands.
| .optopt( | ||
| POSITION_UPDATE_SHORT, | ||
| POSITION_UPDATE, | ||
| "Update position interval in ms", |
There was a problem hiding this comment.
Maybe explain that this is about player events and thereby also MPRIS? I feel like without that context, it be quite unclear what the purpose of this option is?
Personally, I'd prefer the option to be --position-update-interval to make it really explicit, but it does become very long that way...
There was a problem hiding this comment.
Do we need it configurable? I'm not so sure if we want that...
There was a problem hiding this comment.
Either we want it configurable or we should choose a sensible default value other than 0 (which prevent position to be updated).
|
|
||
| let position_update_interval = opt_str(POSITION_UPDATE).as_deref().map(|position_update| { | ||
| match position_update.parse::<u64>() { | ||
| Ok(value) => Duration::from_millis(value), |
There was a problem hiding this comment.
Maybe also add a lower bound here? Either by returning an error if lower, or maybe better by simply taking the value.min(MIN_INTERVAL)? (Not sure what value should be, a few ms at least?) Or at least require it to be nonzero?
There was a problem hiding this comment.
Any idea, what sensible value should we use? 100ms seems quite sensible lower bound.
Thanks! I saw you processed quite a bit of comments after, so ping me when you feel you're ready for another round of review.
What'd be the negatives? 😄 No honestly. It seems like an improvement to me, but let me know if I should be aware of any downsides. |
|
Don't forget to add a changelog entry. |
Done in 0a5c3aa |
AFAICT I've rework everything raised by comment. Should be ok for a last review.
If you don't see any negatives then let's go with that :) |
|
Small reminder that everything seems ready for last review here |
|
👍 I’m away for a few days before I can review, but if anyone wants to beat me to it… |
roderickvd
left a comment
There was a problem hiding this comment.
Almost there I think!
Sorry for the late response.
There was a problem hiding this comment.
It's getting pretty large. I know it's tedious but if you'd have time to split it up into sub-modules then that'd be great.
| Stopped { | ||
| play_request_id: u64, | ||
| track_id: SpotifyUri, | ||
| play_request_id: Option<u64>, |
There was a problem hiding this comment.
Please tag in the changelog that this is a breaking change.
| "sync", | ||
| "process", | ||
| ] } | ||
| time = { version = "0.3", features = ["formatting"] } |
There was a problem hiding this comment.
Only used for MPRIS, so to be made optional.
| "Knee width (dB) of the dynamic limiter from 0.0 to 10.0. Defaults to 5.0.", | ||
| "KNEE", | ||
| ) | ||
| .optopt( |
There was a problem hiding this comment.
Could we do without this feature flag? Because it seems pretty advanced. What would it matter to users of we did or did not drop it?
There was a problem hiding this comment.
Removed option in favor of a 400ms default
- which is just a tokio::sync::mpsc sender, so this should be safe - prep for MPRIS support, which will use this to control playback
- preparation for MPRIS support - now that the data is there, also yield from player_event_handler
- preparation for MPRIS support
- following the spec at https://specifications.freedesktop.org/mpris-spec/latest/ - some properties/commands are not fully supported, yet
|
@andriyor could you check the following command on both dbus services (org.mpris.MediaPlayer2.librespot and the one used by official client).
If you don't know the official spotify client service name use the following command:
|
|
spotify: librespot: I found that unofficial widget supports it:
|
|
@paulfariello Thanks so much for your work on this. Do you have a sense of what it would take for someone to get this across the finish line? |



Taking back on @wisp3rwind work for adding MPRIS support #1341.
Not sure about: