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: keep current season name if no override exists #11674

Closed
wants to merge 1 commit into from

Conversation

revam
Copy link
Contributor

@revam revam commented May 16, 2024

Changes
TL;DR: Keep the season name if the no override exists for the season.

Longer version:
So my plugin uses custom season names — which have worked fine throughout all of 10.8's lifecycle — but after the new NFO named season parsing was added in 10.9 (21 months ago no less), then the support for custom season names provided by remote metadata plugins partially broke as a result. This simple change fixes the issue by keeping the current season name for the existing seasons but allows the NFO season names to override the name if needed/desired, thus not breaking the support for custom season names provided by remote metadata plugins that we've loved Jellyfin for so far.

Also, I did the digging and debugging to fix this before checking for existing issues, and also doesn't think the other PR addressed the root issue here, so I decided to submit this PR since I believe it fixes it.

Issues
#11655 ← Fixes this.
#11656 ← Fixes this.
#11647 ← I felt this didn't address the issue I was having, as I've been debugging Jellyfin to find out why it changed it back, and saw that PR didn't address that issue.

So my plugin uses custom season names — which have worked fine throughout all of 10.8's lifecycle — but after the new NFO named season parsing was added in 10.9 (21 months ago no less), then the support for custom season names provided by remote metadata plugins partially broke as a result. This simple change fixes the issue by keeping the current season name for the existing seasons but allows the NFO season names to override the name if needed/desired, thus **not** breaking the support for custom season names provided by remote metadata plugins that we've loved Jellyfin for so far.
@gnattu
Copy link
Member

gnattu commented May 16, 2024

I don't think your change will fix the problem here either, as you are not addressing the root cause. #11647 is doing the correct thing.

Your code will pass in a null or empty string and still get the season name reset because you are not addressing the root cause: the current season names are not present in the series.SeasonNames at the time of scanning.

In fact, the line you changed won't be executed if that season has a custom season name in the first place. If that line is ever executed, it means the season name is lost and needs to be fixed by #11647.

@revam
Copy link
Contributor Author

revam commented May 16, 2024

image
image

@revam
Copy link
Contributor Author

revam commented May 16, 2024

The fact that they're not in the dict is the issue. it's resetting it because they're not in the dict.

@revam
Copy link
Contributor Author

revam commented May 16, 2024

In fact, the line you changed won't be executed if that season has a custom season name in the first place. If that line is ever executed, it means the season name is lost and needs to be fixed by #11647.

I don't know if this is intended behavior or not, but it will be reset to "Season X" if a series NFO doesn't contain a name for the season even if a custom name was previously set by a remote metadata provider. If that is the intended behavior, then I will rest my case here and close this PR.

@Shadowghost
Copy link
Contributor

No that's not intended, I'm still debugging this use case. Maybe we just need to combine both PRs, need to check the implications

@Shadowghost
Copy link
Contributor

@revam since I can't debug it with the shoko plugin, would you mind checking my updated PR?

@revam
Copy link
Contributor Author

revam commented May 16, 2024

I'll test the other pr later today when i have some free time. @Shadowghost

@revam
Copy link
Contributor Author

revam commented May 16, 2024

@Shadowghost That seems to have done the trick for my use case at least. So should I close this PR then?

image

@revam
Copy link
Contributor Author

revam commented May 16, 2024

I'll just close it.

@revam revam closed this May 16, 2024
@Shadowghost Shadowghost mentioned this pull request May 17, 2024
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.

3 participants