ncspot icon indicating copy to clipboard operation
ncspot copied to clipboard

Add cross platform media control keys support

Open ptixed opened this issue 1 year ago • 10 comments

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

#703

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.)

ptixed avatar Oct 29 '23 13:10 ptixed

If media_control supports the MPRIS features, I don't think there needs to be a separate mpris feature, so it can be removed.

Bettehem avatar Oct 29 '23 16:10 Bettehem

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.

ptixed avatar Oct 30 '23 19:10 ptixed

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 avatar Oct 31 '23 02:10 Bettehem

@Bettehem I can't really work on souvlaki as I have no access to mac so I went with your second idea.

ptixed avatar Nov 03 '23 11:11 ptixed

So excited to finally see this merged!

jacksongoode avatar Nov 11 '23 01:11 jacksongoode

I've been using windows version for a week without issues. Anything else you need to accept the PR?

ptixed avatar Nov 11 '23 10:11 ptixed

Sorry, currently moving to a different place, so super busy at the moment.. hope to have a look at this soon.

hrkfdn avatar Nov 12 '23 11:11 hrkfdn

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 avatar Dec 09 '23 10:12 hrkfdn

@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.

veloek avatar Feb 26 '24 13:02 veloek

I'm with @veloek here - this would be a great QoL improvement! :D

bogdan-calapod avatar May 08 '24 06:05 bogdan-calapod