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

Feature1499 #1561

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Feature1499 #1561

wants to merge 8 commits into from

Conversation

Alzamer
Copy link
Contributor

@Alzamer Alzamer commented Feb 26, 2024

This PR adds a blacklist. Blacklisted tracks are excluded from the play queue. The user can delete tracks from the blacklist in the preferences.

@nukeop
Copy link
Owner

nukeop commented Feb 26, 2024

Hi, and thanks for the PR. Seeing as this is a pretty complex feature, could you please let us have a look at some screenshots? And would you mind adding a test or two for the happy path and some edge cases? Also, the areas where you've added this are already covered by snapshots which will need to be updated. I'll have some more remarks regarding style later on.

Regarding the functionality, the way you wrote it so that whenever the queue changes, it's scanned, and the blacklisted items are removed. Since the point of this feature is to ensure that the autoradio doesn't add blacklisted songs to the queue, this can cause a situation, where it adds a blacklisted song anyway, and it's removed afterwards, causing playback to stop (because it won't retry adding another song). A better way to handle this would be to modify the way autoradio picks songs to play, and blacklist them at that stage. For example, they could be filtered out, and if there are no songs left, it could search one more time with increased craziness until it finds something that's not blacklisted.

@nukeop nukeop added the needs changes The author needs to make changes to this pull request. label Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changes The author needs to make changes to this pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants