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

Fix backdrop image fetching to prioritize non-language images #8982

Merged
merged 2 commits into from Feb 6, 2023

Conversation

bradbeattie
Copy link
Contributor

@bradbeattie bradbeattie commented Dec 30, 2022

Context

#8946 accidentally introduced a side effect of all backdrop images preferring the explicit language of the library. The purpose of backdrop images should actually deprioritize language.

We could additionally modify OrderByLanguageDescending, but how this gets used with both manual image searches and during metadata refresh makes it a little slippery to refactor properly. The proposed solution here is fairly simple and gets us the behaviour we're looking for.

Changes

Image sorting prioritizes non-language images for backdrop metadata refreshes.

@bradbeattie bradbeattie changed the title Backdrops prefer no language images Fix image searching to prioritize non-language backdrops Dec 30, 2022
@bradbeattie bradbeattie changed the title Fix image searching to prioritize non-language backdrops Fix image searching to prioritize non-language images for backdrops Dec 30, 2022
@bradbeattie bradbeattie marked this pull request as draft January 1, 2023 02:18
@bradbeattie bradbeattie marked this pull request as ready for review January 2, 2023 03:27
@bradbeattie bradbeattie changed the title Fix image searching to prioritize non-language images for backdrops Fix backdrop image fetching to prioritize non-language images Jan 2, 2023
@crobibero
Copy link
Member

I think a better change for this is to update DownloadMultiImages:

- foreach (var image in images.Where(i => i.Type == imageType))
+ foreach (var image in images.OrderBy(i => string.IsNullOrWhiteSpace(i.Language)).Where(i => i.Type == imageType))

@bradbeattie
Copy link
Contributor Author

I think a better change for this is to update DownloadMultiImages:

- foreach (var image in images.Where(i => i.Type == imageType))
+ foreach (var image in images.OrderBy(i => string.IsNullOrWhiteSpace(i.Language)).Where(i => i.Type == imageType))

My concern with that change is that it binds DownloadMultiImages to specifically favour non-language images. True, it's only used for that purpose at the moment, but if imageType is an option, best to leave the sorting to calling function.

@crobibero
Copy link
Member

Ok, I would still change the sort method to what I suggested

@bradbeattie
Copy link
Contributor Author

Ah! So the intent behind listWithNoLangFirst is to say "I'll observe whatever ordering is defined in OrderByLanguageDescending, but otherwise prefer non-language images first". Because https://github.com/jellyfin/jellyfin/blob/master/MediaBrowser.Model/Extensions/EnumerableExtensions.cs#L19 might have complexity unknown to listWithNoLangFirst.

I'm unclear if C# ordering with .OrderBy(i => string.IsNullOrWhiteSpace(i.Language)) will preserve the ordering of elements in its list that are equal in the lambda evaluation, but otherwise unique.

@Bond-009 Bond-009 added the regression Regression from previous build label Jan 27, 2023
@Bond-009 Bond-009 merged commit 151aa0f into jellyfin:master Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Regression from previous build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants