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

Resolve #539 #599

Merged
merged 3 commits into from
Feb 20, 2023
Merged

Resolve #539 #599

merged 3 commits into from
Feb 20, 2023

Conversation

Colonial-Dev
Copy link
Contributor

@Colonial-Dev Colonial-Dev commented Feb 19, 2023

This PR corrects the behavior of the rewind/"previous song" action to match convention and official Spotify clients.

With this patch applied, rewinding will only jump to the previous track if the current track has been playing for no more than 2 seconds. Otherwise, playback simply seeks to the start of the current track.

Notes:

  • I copy-pasted the PositionMicros definition and impls from the MPRIS code; you could definitely hoist them out of their respective modules and avoid the duplication, but I figured it would be alright - rule of three and all that.
  • My code seems to hold up under testing, even under vigorous scrubbing and other player abuse. However, I'm not 100% clear on the exact effects of a Load event or the difference between a Seek and a SyncSeek, so there might be a logic error lurking in there.

Fixed bug that caused backwards skips to be accumulated

Placate rustfmt

Fixed another edge case when at the start of a playlist
Copy link
Owner

@xou816 xou816 left a comment

Choose a reason for hiding this comment

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

pretty clean and straightforward! thanks for the contribution!!

as for PositionMicros: I would just merge both eventually. but if you want to keep two versions that's fine -- in that case maybe modifying it to work with milliseconds is interesting; MPRIS requires microseconds but we might not want to be that specific

src/app/state/playback_state.rs Outdated Show resolved Hide resolved
src/app/state/playback_state.rs Outdated Show resolved Hide resolved
Copy link
Owner

@xou816 xou816 left a comment

Choose a reason for hiding this comment

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

LGTM! :)

@xou816 xou816 merged commit 99b448e into xou816:development Feb 20, 2023
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.

None yet

2 participants