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: can_go_previous should always be true #1346

Merged
merged 3 commits into from
Dec 9, 2023
Merged

Conversation

eulerfan271
Copy link
Contributor

@eulerfan271 eulerfan271 commented Dec 8, 2023

Describe your changes

Currently, the MPRIS previous does nothing when the song is the first in the queue, when it should restart the song (as is the case if one uses the previous keyboard shortcut in the TUI). This can fixed by making the MPRIS function can_go_previous identically true, rather than checking if there's a previous song in the queue.

Issue ticket number and link

One might consider this is somewhat related to #1110?

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 Dec 8, 2023

Hey! Thanks for the PR. Should we at least check whether a track is loaded or similar, to disable the previous command if the queue is completely empty, for instance?

Also curious how other plays handle this. The specs aren't very specific about it..

@eulerfan271
Copy link
Contributor Author

Good point, thanks for the quick review! I've updated it to be true only if something is in the queue; it now matches many of the other MPRIS functions (play, pause, etc).

As for other players, I think this behavior (calling "previous" on the first song in the queue repeats that song) is pretty common, and it's present in both ncspot (when using the commands in the TUI) and the official Spotify app. In my opinion, it's also more useful/intuitive than the previous key doing nothing.

@hrkfdn hrkfdn enabled auto-merge (rebase) December 9, 2023 10:46
@hrkfdn
Copy link
Owner

hrkfdn commented Dec 9, 2023

LGTM, let's get it in!

@hrkfdn hrkfdn disabled auto-merge December 9, 2023 10:47
@hrkfdn hrkfdn enabled auto-merge (squash) December 9, 2023 10:48
@hrkfdn hrkfdn merged commit a826115 into hrkfdn:main Dec 9, 2023
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.

2 participants