Add MPRIS support #1596
Add MPRIS support #1596
Conversation
paulfariello
force-pushed
the
mpris
branch
7 times, most recently
from
f4a3b30 to
5e8fc7d
Compare
paulfariello
force-pushed
the
mpris
branch
2 times, most recently
from
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? |
src/main.rs
Outdated
| .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 :) |
- 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
Taking back on @wisp3rwind work for adding MPRIS support #1341.
Not sure about: