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

noRoleFilter should mean noRoleFilter! #1204

Open
darrell-k opened this issue Oct 31, 2024 · 2 comments
Open

noRoleFilter should mean noRoleFilter! #1204

darrell-k opened this issue Oct 31, 2024 · 2 comments

Comments

@darrell-k
Copy link
Contributor

However, when using the combined list, even if noRoleFilter is true (so no role_id is passed), albumsQuery still restricts to active combined list roles:

if ($roleID) {
@roles = split /,/, $roleID;
push @roles, 'ARTIST' if $roleID eq 'ALBUMARTIST' && !$prefs->get('useUnifiedArtistsList');
}
elsif ($prefs->get('useUnifiedArtistsList')) {
@roles = Slim::Schema::Contributor->activeContributorRoles(1);
}
else {
@roles = Slim::Schema::Contributor->contributorRoles();
}

Of course the PR for this would take about 5 minutes, but is it controversial? Possibly!

@michaelherger
Copy link
Member

michaelherger commented Oct 31, 2024

From looking at how noRoleFilter et al. is implemented it seems to completely rely on the browse menu code. And Queries then would assume "standard roles", rather than "no filter". I believe that's still the old assumption that there's basically one kind of artist, "the" artist. And what that artist is we did make some assumptions. The pref therefore has got a slightly different meaning with the arrival of more roles. Now the question is: should the pref be redefined, or the behaviour adjusted?

The problem would be that all callers of the albums query would have to pass the roles they want. That would be a breaking change. We should probably introduce a new param to tell when a caller really doesn't want any role filter at all.

@darrell-k
Copy link
Contributor Author

Good point, and stuff to think about. At least it's now captured here in the issues.

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

No branches or pull requests

2 participants