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

Sort by version name before resolution sorting #12621

Merged
merged 6 commits into from
Sep 18, 2024

Conversation

gnattu
Copy link
Member

@gnattu gnattu commented Sep 9, 2024

Changes

We need to sort by resolution in descending order so that higher resolutions are listed first in the alternate version menu. However, we currently do not consider sorting when the resolutions are the same, leading to unwanted descending order for supplemental titles. This sorts the titles additionally after the resolution, providing a more natural listing.

Issues

Closes #12620
Fixes #12618

Signed-off-by: gnattu <gnattuoc@me.com>
@gnattu gnattu requested a review from a team September 9, 2024 20:37
Signed-off-by: gnattu <gnattuoc@me.com>
This reverts commit ad9afab.
Signed-off-by: gnattu <gnattuoc@me.com>
Signed-off-by: gnattu <gnattuoc@me.com>
Co-authored-by: Dmitry Lyzo <56478732+dmitrylyzo@users.noreply.github.com>
Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably be simplified further: if ResolutionRegex returns an empty string for resolution-less versions, we can remove the grouping.
But it LGTM.

@theguymadmax
Copy link
Contributor

Just tested these changes. Results:
Screenshot from 2024-09-10 16-06-08

Filenames:
Screenshot from 2024-09-10 15-54-53

Resolutions:
All file resolutions correspond to the resolutions specified in their filenames. For files lacking resolution information in their filenames, the following resolutions were applied:
X-Men Apocalypse (2016).mp4 -> 720p file
X-Men Apocalypse (2016) - Directors Cut -> 2160p file
X-Men Apocalypse (2016) - Theatrical Release -> 1080p file

@gnattu
Copy link
Member Author

gnattu commented Sep 10, 2024

I wonder why the sorting result is different from your instance and the unit test... it does not make many sense to me tbh

@theguymadmax
Copy link
Contributor

videoInfos provides resolution metadata, which is utilized regardless of whether the resolution is in the filename. The sorting is based on resolution, whether it's specified in the filename or not. #12129

@gnattu
Copy link
Member Author

gnattu commented Sep 10, 2024

So current sorting is correct and the intended behavior right?

@theguymadmax
Copy link
Contributor

Two separate issues here

  1. Sorting does not follow current documentation: Movie versions are presented in an alphabetically sorted list. An exception applies to resolution names, which are sorted in descending order from highest to lowest resolution. A version name qualifies as a resolution name when ending with either a p or an i.

  2. Sorting is reversed when resolutions are in the file name. Current sorting behavior:
    Screenshot from 2024-09-10 16-23-02

@gnattu
Copy link
Member Author

gnattu commented Sep 10, 2024

Then probably we should not hijack this PR to extend its scope because what you pointed out is something that having a much deeper impact. When we are fetching mediastreams for everything we always sort by width if available and that is in the very cursed BaseItem.cs:

.ThenByDescending(i =>
{
var stream = i.VideoStream;
return stream is null || stream.Width is null ? 0 : stream.Width.Value;
})

Which means the prober correctly sorted things, but the reader just sorted it again. I prefer to make a separate PR for that because that one may contain more unwanted side effects.

@theguymadmax
Copy link
Contributor

This PR does fix the issue, thank you. Hopefully, there will be a PR to address the other issue as well, rather than just changing the documentation, which is understandable. As it stands now, there is no way for users to sort alternative versions without considering video resolution when the resolutions vary between files.

@gnattu
Copy link
Member Author

gnattu commented Sep 11, 2024

This PR does fix the issue, thank you. Hopefully, there will be a PR to address the other issue as well, rather than just changing the documentation, which is understandable. As it stands now, there is no way for users to sort alternative versions without considering video resolution when the resolutions vary between files.

It up: #12626

@crobibero crobibero merged commit 9015734 into jellyfin:master Sep 18, 2024
18 checks passed
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.

Versions are sorted in reverse alphabetical order when filename includes resolution
5 participants