-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 replace logic #11743
Fix replace logic #11743
Conversation
if (replaceData || targetItem.LinkedChildren.Length == 0) | ||
{ | ||
targetItem.LinkedChildren = sourceItem.LinkedChildren; | ||
} | ||
else | ||
{ | ||
targetItem.LinkedChildren = sourceItem.LinkedChildren.Concat(targetItem.LinkedChildren).Distinct().ToArray(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is correct.
I haven't studied the code, but I have the following questions; |
First provider wins I think? (Otherwise you get duplicates if there are small variations in spelling, which I've seen happen with actor names) Would be nice if you could disable those in the provider configuration, I think for some of those you already can. I know you can set genre limits for anidb and anilist, so you could just tell it not to add any I think. Although I am not sure the next one in line will get a chance to add those if the first one has that disabled. (Maybe warrants a feature request?) |
NFO/Local provider metadata is always preferred, after that providers only add metadata not already there (in the order you defined)
The new logic merges them from the different providers while removing duplicates
Purely provider dependant |
I also tested the moving part mentioned in issue #11294. The user did not provide a clear reproduction so I tested the moving of a show between 2 libraries. This resulted in metadata being pulled freshly again, even when originally in the original library it being set to locked. In my opinion this is working as intended. I also tested moving a folder from the root in the library, to it's own folder. This also resulted in the movie metadata being pulled again. I can see why someone would think that this is not working as intended as the movie stayed within the same library, just with a different file path (see bottom of comment, this is working as intended). To clarify what I did with the movie: Firstly I added a movie (violet evergarden) to the root directory of what the movie library uses: I then did a scan and edited the metadata of the movie. I then removed the ratings and the overview description and the original title. After that i locked the metadata fields/ item and saved it. The next step I did was creating a directory for the movie and moved the movie inside it. See After that I performed a replace all refresh on the movies library. The result is that the movie has all it's metadata again: What is our view on this behavior? |
IIRC jellyfin has all the metadata tied to a path, so moving the media == loosing the lock would be expected. I'm pretty sure this is also why the same path in two libraries share metadata. |
I can confirm that #11773 is solved with this PR. I identified a movie as another movie. After that I performed a replace all on the library and on the movie itself. It is still showing as the movie I manually identified it with. |
@TimGels Test case 4 should now work too. |
@Shadowghost |
Can confirm that everything is working as mentioned in the PR |
@@ -1048,6 +1021,10 @@ private bool HasChanged(BaseItem item, IHasItemChangeMonitor changeMonitor, IDir | |||
{ | |||
target.Studios = source.Studios; | |||
} | |||
else if (source.Studios.Length >= 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's equal to 0, it doesn't change anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're oly in this check if we either not replace or the target already has studios this is intended. If souce has studios, we append, if it does not we ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless target.Studios
contains duplicates, then target.Studios.Concat(source.Studios).Distinct().ToArray()
is the same as target.Studios
when length == 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I can just remove the check, that should fix the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just wondering why the check was >= 0
and not > 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removing the checks to delete any duplicates is sensible regardless
2ddb15c
into
jellyfin:release-10.9.z
Fix replace logic Original-merge: 2ddb15c Merged-by: joshuaboniface <joshua@boniface.me> Backported-by: Joshua M. Boniface <joshua@boniface.me>
Was this merged into 10.9.4? I still have the issue |
For me, replacing works now with 10.9.4...mostly. I does grab description and ratings again now. But it doesn't get season names, despite that tick being on in the settings. So that's still a bug. However, it also now doesn't break season names anymore when they've been manually changed in the nfo file. Which is good, because I kinda only want fix season names in my library and keep the other info as-is. I wrote myself a small Python script that goes over my library, gets the season names from TMDB and patches the nfo files. In case that's useful for anyone else, it's here: https://github.com/DrMcCoy/fix-season-names . No warranties, use at your own peril, etc., though, of course. |
OK, to be very specific, here's the test I perform:
and it doesn't change the metadata like it used to in 10.8.x Logs from the operation
Incidently, I'm not sure why the msg " Trailer URL uses a deprecated format" shows up as I never enabled/used Trailers. |
@nodje if you remove the NFO, does it work then? I think the issue is that NFOs take precedence over anything else and since they are language-independent, they'll fill up the metadata before the remote providers do. |
But should the NFO really take precedence when Replace All Metadata is checked? Previous behaviour was that the NFO content was replaced in that case. |
I think this kind of is an apple and egg problem since technically NFOs are metadata sources to query for metadata and nothing more. They are the ultimate source of truth in my opinion. I agree and know (since I changed it) that before remote provider results overrode the NFOs if you replaced all metadata, but I'm unsure what to do about it. Regardless, I think this conversation should be moved to an issue. |
You're right, that doesn't work for me either. I add a new TV show, it defaults to English (because most of what I have is English, so that's okay). But this show is a German one and I want German metadata. So I change it to German and Germany, then replace all metadata. That used to work, but now it doesn't. I can delete all the nfos now, but that's a bit cumbersome? Also, isn't the language selection saved to the tvshow.nfo in the first place? |
I disagree that this is a 10.10 thing (I mean, that's going to take, what, another 2 years?) or that deleting the NFO is a "good" solution, even in the meantime. You basically broke adding items to mixed-language metadata libraries. Since there is no way to signal "I want this new stuff to be in $LANGUAGE instead of the default", "replace all metadata" basically the only way to do this. You shouldn't need fuzz around with the generated file in the filesystem afterwards, that's just...bad. |
There 100% needs to be a “fetch metadata from remote sources” vs “refresh data from local NFO”. A checkbox will do. One looks past the NFO. One looks only to the NFO. I do modify NFO files on occasion and want it read. That said, most of the time I want to refresh a show that got uploaded before the metadata was available and once the NFO is done, there’s no way to achieve it without manually deleting files |
This was not the case before 10.9, and before 10.9 it was intuitive and worked. This new behaviour should have been a separate feature. And on top of that, they are never the ultimate source of truth in my opinion since the the remote source is likely to have better, more up-to-date data in > 99% of the cases because at most one person contributes to the NFO file, while potentially thousands contribute to the remote source. |
#11921 should take care of it - skipping local NFOs when replacing and saving is enabled. Please test if you have the time. |
It does work to remove the NFO. Unrelated to the case but I'd appreciate a pointer to an explanation: what puzzles me with NFO, is that I get two of them for existing movies after the 10.9 upgrade. The new movie.nfo one an the old one that bears the name of the movie file.
Shall I delete all NFO with the old format? |
Agreed. And with that logic, the way to replace files with data from NFO is on initial import- so remove and re-add if it's already there. You'd likely rarely want to import from NFO unless it's the initial addition. Agree with that logic. |
We never passed the flag for metadata replacement to the remote provider logic, hence the remote result would never replace what's already there.
Additionally, the logic did not repect if an item was locked or not
Changes
Issues
Fixes #8532 (potentially)
Fixes #11294 (the part where it is repopulated)
Fixes #11656
Fixes #11717
Fixes #11773 (potentially)
Fixes #11831
Testing