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

MPRIS: Restructuring and cleanup #1515

Merged
merged 4 commits into from
Sep 19, 2024
Merged

Conversation

haruInDisguise
Copy link
Contributor

@haruInDisguise haruInDisguise commented Sep 11, 2024

Describe your changes

My pr addresses some of the inconsistencies in ncspot's mpris
implementation. While the previous logic is technically good enough, it
is inflexible and reports redundant information.
I've tried to address those issues to make it easier for software, that processes mpris events in
some way, to accurately react to player updates.

  • 'Metadata' and 'Playback' updates have been separated into their own
    command
  • Mpris commands are only emitted from within spotify.rs
  • Some parts of the application creation logic has been
    restructured to allow for mpris events to be emitted upon startup
    • The initial song loading code has been moved from 'Queue::new'
      into 'Application::new'.

Issue ticket number and link

This closes #1514

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

Edit: An example of software that requires strict MPRIS compliance would be the MPRIS module for yambar that I am working on.

My pr addresses some of the inconsistencies in ncspot's mpris
implementation. While the previous logic was technically good enough, it
was inflexible and reported redundant information.
This will make it easier for software, that processes mpris events in
some way, to accurately react to player updates.

- 'Metadata' and 'Playback' updates have been separated into there own
  command
- Mpris commands are only emitted from within spotify.rs
- Some parts of the application creation logic has been
  restructured to allow for mpris events to be emitted upon startup
    - The initial song loading code has been moved from 'Queue::new'
      into 'Application::new'.
@haruInDisguise haruInDisguise force-pushed the mpris_restructure branch 2 times, most recently from 75c2aa8 to 099a934 Compare September 16, 2024 15:52
Copy link
Owner

@hrkfdn hrkfdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR ❤️ I only have a minor nitpicky comment regarding the naming. WDYT?

src/mpris.rs Outdated Show resolved Hide resolved
src/application.rs Show resolved Hide resolved
I've added a clippy exception so it does not complain to us about enum
variants starting with the same prefix.
@haruInDisguise
Copy link
Contributor Author

I've chosen my naming convention (:p) and also added an exception so clippy does not complain to us about enums starting with the same prefix.

@hrkfdn hrkfdn enabled auto-merge (squash) September 19, 2024 21:28
@hrkfdn
Copy link
Owner

hrkfdn commented Sep 19, 2024

Looks good, let's get it in + sorry for the delay! :)

@haruInDisguise
Copy link
Contributor Author

No worries! Thanks for taking the time!

@hrkfdn hrkfdn merged commit 3893a0e into hrkfdn:main Sep 19, 2024
5 checks passed
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.

MPRIS behavior is inconsistent
2 participants