ncspot
ncspot copied to clipboard
Add cross platform media control keys support
Describe your changes
Added new media_control
feature making use of souvlaki lib.
Works on debian and windows 10. I will not be able to check on mac.
I kept the old mpris
feature but it is not compatible with media_control
. Should it be removed?
Code has been mostly adapted from current mpris
implementation and https://github.com/aome510/spotify-player/.
Issue ticket number and link
Checklist before requesting a review
- [x] Documentation was updated (i.e. due to changes in keybindings, commands, etc.)
- [x] Changelog was updated with relevant user-facing changes (eg. not dependency updates, not performance improvements, etc.)
If media_control
supports the MPRIS features, I don't think there needs to be a separate mpris
feature, so it can be removed.
So I looked at mpris.rs
again and found some features that are not available in souvlaki:
-
supported_uri_schemes
-
has_tracklist
-
loop_status
- metadata:
mpris:trackid
,xesam:albumArtist
,xesam:discNumber
,xesam:trackNumber
,xesam:url
,xesam:userRating
-
shuffle
-
volume
-
can_go_next
,can_go_previous
,can_play
,can_pause
,can_seek
,can_control
In souvlaki these mostly have hardcoded values or are nonexistent: https://github.com/Sinono3/souvlaki/blob/master/src/platform/mpris/zbus.rs
I only need to undo my last commit if we want to keep mpris
as an option.
Indeed, it seems like the Souvlaki MPRIS implementation is incomplete, so it won't work as a replacement for the current mpris
feature as is.
I have two suggestions: Either implement MPRIS support in souvlaki properly, or use media_control
only for Windows/macOS and mpris
for others. I think the former is preferrable though, so that we won't have multiple libraries doing the same thing
@Bettehem I can't really work on souvlaki as I have no access to mac so I went with your second idea.
So excited to finally see this merged!
I've been using windows version for a week without issues. Anything else you need to accept the PR?
Sorry, currently moving to a different place, so super busy at the moment.. hope to have a look at this soon.
I would really like to merge this, but I think we should fix the souvlaki implementation first as @Bettehem said, so we can avoid having duplicate MPRIS logic in the codebase.
@hrkfdn, please don't let perfect be the enemy of good. This would be a great improvement and cleaning up dependencies could be a follow-up for when the souvlaki implementation is improved. Just my two cents.
I'm with @veloek here - this would be a great QoL improvement! :D