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

feat: commands for next/previous album that respect shuffle settings #3007

Merged

Conversation

rsekman
Copy link
Contributor

@rsekman rsekman commented Oct 9, 2023

Feature

This pull request implements commands that can be bound to hotkeys: Playback/Play Next Album, Playback/Play Previous Album, Playback/Play Random Album. The latter goes to first track of a random album in the current playlist. The behaviour of the first two depends on the shuffle setting and is as follows.

  • DDB_SHUFFLE_RANDOM, DDB_SHUFFLE_TRACKS: same as Play Next Track and Play Previous Track.
  • DDB_SHUFFLE_OFF, DDB_SHUFFLE_ALBUMS:
    • Play Next Album is the same as calling Play Next Track until reaching a new album.
    • If not on the first track of the current album, Play Previous Track goes to the first track on the current album
    • If on the first track of the current album, Play Previous Track is the same as calling Play Previous Track until reaching the first track of the previous album
    • The reasoning behind this is mimicking pressing Back once to go the beginning of the current track and twice to go to the beginning of the previous track.

Implementation

While Play (Next/Previous) Album mimic repeated calls to Play (Next/Previous) Track, they are implemented with separate functions to avoid having to search the playlist repeatedly. This would be O(#tracks on album * #tracks in playlist) in DDB_SHUFFLE_ALBUMS; now they are O(#tracks in playlist).

Thus the pull request consists of 4 commits, viz., in order:

  1. Preparatory refactoring checking if two playlist items belong to the same album into a function. This was already implemented in three places in three different ways.
  2. Preparatory refactoring breaking searching the playlist for minimal notplayed or maximal played out into some helper functions.
  3. Implementation of the feature and a test suite. Note that 2/3 of the new code is in tests as there are quite a lot of cases to test.
  4. Trivial linting.

To discuss

I've tried to follow DeaDBeeF's style as best as I understand it. :^) But of course I'll be happy to lint before merging if I haven't been fully consistent.

Because the pull request introduces new signals DB_EV_PLAY_(NEXT|PREV|RANDOM)_ALBUM I think we need to bump the API version before merging. I've left it until then because I thought it best to receive signoff first.

@Oleksiy-Yakovenko
Copy link
Member

You don't necessarily need to bump API version, since there's still a possibility to get onto 1.9.6 train.
But you need to add version check to the new enum cases.

@Oleksiy-Yakovenko
Copy link
Member

The main concern for me is whether the function pl_items_from_same_album does the same thing as the code was doing before. I'll need to carefully read and understand the code to figure this out.

@rsekman
Copy link
Contributor Author

rsekman commented Oct 9, 2023

The main concern for me is whether the function pl_items_from_same_album does the same thing as the code was doing before. I'll need to carefully read and understand the code to figure this out.

The body is the same as the check in streamer.c:stop_after_album_check. Other than in the new code it's called in three places: stop_after_album_check, plt_insert_item, plt_reshuffle. I could write tests for the latter two?

@Oleksiy-Yakovenko Oleksiy-Yakovenko merged commit 03365d9 into DeaDBeeF-Player:master Oct 9, 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.

2 participants