Skip to content
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

Add cross platform media control keys support #1327

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ptixed
Copy link

@ptixed ptixed commented Oct 29, 2023

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

  • Documentation was updated (i.e. due to changes in keybindings, commands, etc.)
  • Changelog was updated with relevant user-facing changes (eg. not dependency updates,
    not performance improvements, etc.)

@Bettehem
Copy link
Contributor

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

@ptixed
Copy link
Author

ptixed commented Oct 30, 2023

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.

@Bettehem
Copy link
Contributor

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

@ptixed
Copy link
Author

ptixed commented Nov 3, 2023

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

@jacksongoode
Copy link

So excited to finally see this merged!

@ptixed
Copy link
Author

ptixed commented Nov 11, 2023

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

@hrkfdn
Copy link
Owner

hrkfdn commented Nov 12, 2023

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

@hrkfdn
Copy link
Owner

hrkfdn commented Dec 9, 2023

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.

@veloek
Copy link

veloek commented Feb 26, 2024

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

@hrkfdn hrkfdn changed the title Add cross platform media control keys support (#703) Add cross platform media control keys support Feb 26, 2024
@bogdan-calapod
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants