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

Feature: randomize queue #1270

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Feature: randomize queue #1270

wants to merge 3 commits into from

Conversation

t-nil
Copy link

@t-nil t-nil commented Sep 2, 2023

Hey,

so the motivation for this feature is that adding songs to play next breaks when you are on shuffle. Spotify solves this with a "second queue" which is immune to shuffle. However, going for less complexity, I have implemented a feature I knew from foobar2000: The ability to randomize the queue, that means, shuffling the elements of the backing queue Vec.

I tested the feature on my system in both debug and release mode. It seems to work.

Design choices:

  • The playback is interrupted. If ncspot is playing when you execute the command, a stop() is executed and playback is restarted from the top of the queue as soon as randomizing is completed. Alternatives would have been to keep the current song playing, and either move it to the start, keep its position fixed or update the internal playback state, as after randomization the GUI was showing the title of the song which then occupied the same id. But stopping playback (and restarting after) seemed like the simplest, cleanest case.
  • You can randomize the queue as often as you like. Another option would have been to save the old state and restore it, making randomization a toggle, but I assume that quickly gets messy when new songs are added in between etc.
  • Shuffle mode gets deactivated when you randomize. My view is that the two features combined wouldn't make much sense.

TODO

  • Support DBus property / Fix mpris
    When I naively added the code in mpris.rs (which is now commented out), I got a lot of incomprehensive compiler errors, probably caused by complex macro expansion. One hypothesis of mine is that properties which have neither parameters nor return values are not supported. Since I have almost no experience with DBus I hope you can help me here @hrkfdn (or someone else ofc).
  • Implement randomization also for playlists.
    I don't know how much work that would be, and would probably be in favor of first accepting this PR and making a new one for that extra feature. But the odds of me finding time for that aren't too bad :)

@hrkfdn, would you be, in principal, willing to merge the feature described in this PR, and are there any changes needed for new commands I have overlooked? I just grep'd for shuffle and modified the same places (where it made sense).

EDIT: Maybe this even helps with #1075 since randomization is correctly shown and doesn't add any complexity. I want to stress again that foobar2000 which I used for a long time had both features, and both were useful in its own.

@t-nil t-nil marked this pull request as ready for review September 2, 2023 23:47
@hrkfdn hrkfdn force-pushed the main branch 5 times, most recently from 4c6ec86 to c4e3121 Compare October 26, 2023 16:06
@wdoekes
Copy link

wdoekes commented Mar 28, 2024

Randomize queue makes sense. I would prefer if you reconsidered the design decision of stopping/restarting play. A sudden stop in audio can be very noticable.

Putting the currently played song at pos 1 sounds like the logical thing to do to me.

@t-nil
Copy link
Author

t-nil commented May 10, 2024

Thanks for your input!

I would prefer if you reconsidered the design decision of stopping/restarting play. A sudden stop in audio can be very noticable.

I agree, my gut tells me that's a very sensible design direction. I'd be down to implementing it.

Unfortunately, since my pull request is pretty old and I have not received any feedback from the maintainers, I would wait with committing further resources until that changes.

@t-nil t-nil marked this pull request as draft May 10, 2024 17:28
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