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 Seeked signal to Mpris interface #1494

Merged
merged 3 commits into from
Sep 21, 2024
Merged

Conversation

elParaguayo
Copy link
Contributor

@elParaguayo elParaguayo commented Aug 4, 2024

NB I am new to Rust so may have made some horrible mistakes in here!

Describe your changes

The Mpris2 spec includes a Seeked signal which should be fired when the track position changes in an unexpected way i.e. when the user seeks to a different part of the track.

This PR implements this signal on seek events and also when a new track begins. The latter is not strictly required but has been observed in other players (e.g. VLC).

Issue ticket number and link

Closes #1492

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

@hrkfdn
Copy link
Owner

hrkfdn commented Aug 10, 2024

Thanks ❤️, tested it quickly and it looks pretty good to me! Could you update the Changelog and check on the Clippy errors? After that I'll happily merge it :)

@elParaguayo
Copy link
Contributor Author

Thanks. I've looked at those 2 cargo clippy errors and I'm struggling to see how they were caused by my changes.

error: unnecessary structure name repetition
  --> src/mpris.rs:78:6
   |
78 | impl MprisPlayer {
   |      ^^^^^^^^^^^ help: use the applicable keyword: `Self`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#use_self
   = note: requested on the command line with `-D clippy::use-self`

I don't see how adding a new function caused that. I also don't understand how this gets fixed. The link refers to a new function but we don't have that here - is that the problem?

error: all variants have the same prefix: `Notify`
   --> src/mpris.rs:470:1
    |
470 | / pub enum MprisCommand {
471 | |     /// Notify about playback status and metadata updates.
472 | |     NotifyPlaybackUpdate,
473 | |     /// Notify about volume updates.
...   |
476 | |     NotifySeekedUpdate(i64),
477 | | }
    | |_^
    |
    = help: remove the prefixes and use full paths to the variants instead of glob imports
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#enum_variant_names
    = note: `-D clippy::enum-variant-names` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::enum_variant_names)]`

Again, this should have been an issue before this PR as the two names had the same prefix (and suffix) - but maybe it needs at least 3 members before it fires. To fix this, I'd need to rename existing values. I'm fine to do that if you're ok with it.

@ThomasFrans
Copy link
Contributor

ThomasFrans commented Aug 23, 2024

That first one is quite simple to solve generally. I think it means you used the literal name MprisPlayer somewhere inside an implementation on MprisPlayer (in a function inside the impl MprisPlayer block). In that case it's better to use Self to refer to MprisPlayer. It helps when searching the codebase, so you don't get 100s of results when looking for an identifier, but only the definition.

For the second one I'd imagine the lint only triggers when you have 3 or more variants with the same prefix (https://dev-doc.rust-lang.org/nightly/clippy/lint_configuration.html#enum-variant-name-threshold). I also don't understand what the suggestion means in this case. I've never seen that one either.

@elParaguayo
Copy link
Contributor Author

Thanks for the comments.

That first one is quite simple to solve generally. I think it means you used the literal name MprisPlayer somewhere inside an implementation on MprisPlayer (in a function inside the impl MprisPlayer block). In that case it's better to use Self to refer to MprisPlayer. It helps when searching the codebase, so you don't get 100s of results when looking for an identifier, but only the definition.

Unless I'm blind, I haven't done this at all. There an MprisPlayer reference in the impl MprisManager block but I can't use Self there.

For the second one I'd imagine the lint only triggers when you have 3 or more variants with the same prefix (https://dev-doc.rust-lang.org/nightly/clippy/lint_configuration.html#enum-variant-name-threshold). I also don't understand what the suggestion means in this case. I've never seen that one either.

This is probably just solved by renaming the variants but I'll leave this until I can fix the first one.

@haruInDisguise
Copy link
Contributor

Hey, @elParaguayo. A PR of mine (#1515) has been merged that restructures how MPRIS events are emitted, making this PR (in its current state) incompatible with the main branch.

Unless I'm blind, I haven't done this at all. There an MprisPlayer reference in the impl MprisManager block but I can't use Self there.

From what I can tell, this is an issue causes by zbus, upon expanding the interface(...) procedural macro. You could add a clippy exception (#![allow(clippy::use_self)]) to the start of mpris.rs.

@elParaguayo
Copy link
Contributor Author

@haruInDisguise thanks for letting me know. I'll rebase this PR and fix as necessary.

The Mpris2 spec includes a `Seeked` signal which should be fired when
the track position changes in an unexpected way i.e. when the user
seeks to a different part of the track.

This PR implements this signal on seek events and also when a new track
begins. The latter is not strictly required but has been observed in
other players (e.g. VLC).

Closes hrkfdn#1492
@hrkfdn hrkfdn enabled auto-merge (squash) September 21, 2024 09:51
@hrkfdn
Copy link
Owner

hrkfdn commented Sep 21, 2024

Thank you :) Merged with some very minor changes

@hrkfdn hrkfdn merged commit 40644e1 into hrkfdn:main Sep 21, 2024
5 checks passed
@elParaguayo
Copy link
Contributor Author

Thanks for the changes. Beyond my current skill level so appreciate you improving my PR.

@elParaguayo elParaguayo deleted the mpris-add-seeked branch September 21, 2024 10:19
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.

Add support for Seeked signal on mpris interface
4 participants