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

[VideoInfoScanner][GUIDialogVideoInfo] Update blank movie sets with artwork from Movie Set Information Folder #24131

Closed
wants to merge 1 commit into from

Conversation

78andyp
Copy link
Contributor

@78andyp 78andyp commented Nov 22, 2023

There are now three ways the movie set artwork can be updated:

  1. On a library update - good if you have multiple sets with no artwork.
  2. Right click (just the same as choose art)
  3. Through the video information dialog (just the same as choose art)

After 3 the library display isn't automatically updated - this is a different bug - see #24176.

Description

When a movie set is added by the scraper, sometimes there is no associated artwork, for example:

image

If, when the movie is added, there is an entry in the Movie Set Information Folder (as per https://kodi.wiki/view/Movie_set_information_folder) then any artwork from there is used.

Motivation and context

Obviously, you are't going to know if there is no set artwork until you've added the movie. Even if you then add artwork in the Movie Set Information Folder - it is never scanned again - you would have to add the artwork manually through the context menu.

If you have a number of sets to update - this is slow.

This PR rescans any movie sets with no atwork, when the library is updated, to see if there is any artwork in the Movie Set Information Folder and adds it automatically.

How has this been tested?

On my library in Win 11 x64.

What is the effect on users?

Easier to add movie set artwork - just need to setup the Movie Set Information folder (as per Wiki above) and then place a poster.jpg in a subdiretory with the name of the movie set. It will then be used automatically at next library update.

Screenshots (if appropriate):

Using above example:

image

After library update:

image

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • [ X ] Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

I'm sure it's probably not in the right place in the code!

@ksooo
Copy link
Member

ksooo commented Nov 23, 2023

@78andyp thanks for the PR. Need to say that this code area is not something I'm familiar with, so cannot say anything about correctness of the implementation, especially not, whether the place you added the code is appropriate. I hope somebody else can pick this up for review.

@ksooo ksooo added Type: Improvement non-breaking change which improves existing functionality Component: Video labels Nov 23, 2023
@KarellenX
Copy link
Member

Nice feature. Thanks!!

Is there an option to update individual sets instead of a full library update?

@78andyp
Copy link
Contributor Author

78andyp commented Nov 24, 2023

Nice feature. Thanks!!

Is there an option to update individual sets instead of a full library update?

Do mean like as a context menu option for the individual set?? I can certainly look.

@ksooo
Copy link
Member

ksooo commented Nov 24, 2023

More like video info dialog, add support for button „refresh“. This is how it works for other items as far as I know.

@KarellenX
Copy link
Member

Do mean like as a context menu option for the individual set??

Actually, like ksooo mentions, the Refresh button. Here is an example i've photoshopped...
My concern would be for users with large libraries having to run an extensive global Library Update to update one or two movie sets.

setsrefresh

@78andyp
Copy link
Contributor Author

78andyp commented Nov 24, 2023

Thanks both.

I'd actually started playing with a context menu option.

With the next commit, the option 'Use local art' will appear when the set has no art currently AND there is a directory in the MSIF.

image

I will look at a refresh button as well.

@hurricane51
Copy link

So it will only populate BLANK movie sets? I did a clean re-install of KODI 19, but my media all had set information in their NFO files. The sets were thus recreated. but KODI generated the set art. I already have a fully populated MSIF folder. How do I reassign the art in my MSIF folder to the existing sets?

@78andyp
Copy link
Contributor Author

78andyp commented Nov 25, 2023

So it will only populate BLANK movie sets? I did a clean re-install of KODI 19, but my media all had set information in their NFO files. The sets were thus recreated. but KODI generated the set art. I already have a fully populated MSIF folder. How do I reassign the art in my MSIF folder to the existing sets?

Can I just check a couple of things:

You have the MSIF set in Settings -> Media -> Videos -> Movie Set Information Folder??
You have Local Information Only set in the Choose information provider option of the Sources??

If both of those are true then it feels like Kodi should look in the MSIF. I'll see what I can recreate here.

@hurricane51
Copy link

hurricane51 commented Nov 25, 2023 via email

@ksooo
Copy link
Member

ksooo commented Nov 25, 2023

I'd actually started playing with a context menu option.

I'm not sure we want/need this, tbh. AFAIK. for all other media types we are managing this using the video info dialog's "refresh" button. I don't see why we should do this differently for movie sets or why we need another way in addition to the dialog.

@78andyp
Copy link
Contributor Author

78andyp commented Nov 25, 2023

I'd actually started playing with a context menu option.

I'm not sure we want/need this, tbh. AFAIK. for all other media types we are managing this using the video info dialog's "refresh" button. I don't see why we should do this differently for movie sets or why we need another way in addition to the dialog.

Because 'Choose art' option is there already so it makes sense (to me at least) to have the 'Use local art' option there as well.

@ksooo
Copy link
Member

ksooo commented Nov 25, 2023

Because 'Choose art' option is there already so it makes sense (to me at least) to have the 'Use local art' option there as well

I see it more the other way round. Having „choose art“ in the context menu is superfluous and should be removed, because we have the dialog for that. Why add same functionality at different places? All this code needs to be maintained…

@78andyp
Copy link
Contributor Author

78andyp commented Nov 25, 2023

Yes to both of those. I have been having an exchange on the KODI forum, and here’s what I posted: **************************************** I did a new install of KODI 19. I configured everything before adding any content, and that included specifying the MSIF folder. The MSIF had been created with my previous installation, and I saw no need to re-create it for the same content. My movies all had NFO files along with the video in their individual folder. These NFO's all had set information. I wasn't creating any new sets. I initially specified Use Local Information as my information provider to grab the info from the local files. After the movies were in the library, I changed the scraper to Universal Movie scraper for future additions. I found that KODI had generated/assigned random art (from the included movies) to most of the existing sets. Others, it left blank. It didn't use the MSIF folder art. I would like to avoid having to go through the movie sets one-by-one to assign the correct art. If that's not possible, then using MSIF is not much use to me other than providing a storage place for movie sets, to which the user had no access previously, at least to my knowledge. Not an insignificant feature, and I'm glad to have it. I just thought I would be able to replace the KODI-assigned art with my previous assignments. Apparently, that is not the case. If I can delete the libraries and add the content in some other way so that it would grab the set artwork from the MSIF, it would probably be faster than manually assigning set artwork. From: 78andyp @.> Sent: Saturday, 25 November, 2023 7:25 PM To: xbmc/xbmc @.> Cc: hurricane51 @.>; Comment @.> Subject: Re: [xbmc/xbmc] Update blank movie sets with artwork from Movie Set Information Folder… (PR #24131) So it will only populate BLANK movie sets? I did a clean re-install of KODI 19, but my media all had set information in their NFO files. The sets were thus recreated. but KODI generated the set art. I already have a fully populated MSIF folder. How do I reassign the art in my MSIF folder to the existing sets? Can I just check a couple of things: You have the MSIF set in Settings -> Media -> Videos -> Movie Set Information Folder?? You have Local Information Only set in the Choose information provider option of the Sources?? If both of those are true then it feels like Kodi should look in the MSIF. I'll see what I can recreate here. — Reply to this email directly, view it on GitHub <#24131 (comment)> , or unsubscribe https://github.com/notifications/unsubscribe-auth/AETWG32HY3BMQEBYF4J2QVDYGHPQFAVCNFSM6AAAAAA7W2YJZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRWGI4TMMRSGQ . You are receiving this because you commented.Message ID: @.***>

Have you tried it with the latest beta of Kodi 21?? What OS are you using??

I've just deleted local database completely and manually generated 3 NFOs for Predator, Predator 2 and Predators - naming the Set in the NFO as Predator Collection. Each directory is just the movie MKV and movie.nfo.

I then set the source to local, set single movie in each directory, set scraper to local.

I then set MSIF directory and created subdirectory Predator Collection with just poster.jpg in it.

I then ran scraper for first time and it found the set artwork automatically.

@KarellenX
Copy link
Member

@hurricane51

The conversations in this PR is strictly for feedback on a new feature, NOT your support requests. Please stop hijacking this conversation.

@78andyp

Please ignore hurricane51. He was being offered support on the forum, but he discontinued... https://forum.kodi.tv/showthread.php?tid=375147
He does not even have this PR installed and is seeking general help.

@78andyp
Copy link
Contributor Author

78andyp commented Nov 26, 2023

Because 'Choose art' option is there already so it makes sense (to me at least) to have the 'Use local art' option there as well

I see it more the other way round. Having „choose art“ in the context menu is superfluous and should be removed, because we have the dialog for that. Why add same functionality at different places? All this code needs to be maintained…

Agree, should probably be both or neither. I personally tend to use context menus (probably as come from a windows centric background).

@78andyp
Copy link
Contributor Author

78andyp commented Nov 26, 2023

Do mean like as a context menu option for the individual set??

Actually, like ksooo mentions, the Refresh button. Here is an example i've photoshopped... My concern would be for users with large libraries having to run an extensive global Library Update to update one or two movie sets.

setsrefresh

I've used the existing refresh button - it now should appear if there it's a blank set and a folder in the MSIF.
I also noticed that changing the art using the Choose Art button doesn't update the library thumbnail when you exit the information dialog (you have to exit the library and go back in) - so I've added a fix for that as well.

@@ -1672,7 +1673,7 @@ namespace VIDEO
CFileItemList availableArtFiles;
CDirectory::GetDirectory(path, availableArtFiles,
CServiceBroker::GetFileExtensionProvider().GetPictureExtensions(),
DIR_FLAG_NO_FILE_DIRS | DIR_FLAG_READ_CACHE | DIR_FLAG_NO_FILE_INFO);
Copy link
Contributor Author

@78andyp 78andyp Nov 26, 2023

Choose a reason for hiding this comment

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

I changed this (removing DIR_FLAG_READ_CACHE) as I found that if you add a file to the MSIF once Kodi has started it never gets found on subsequent searches. GIven the MSIF will only contain a small number of files I would have this doesn't have a performance impact.

@scott967
Copy link
Contributor

Have a requirements issue with the use of "refresh". In every other context "refresh" finds a local nfo or configured scraper to update the info. Here, "refresh" won't update the set overview or set membership making it a one-off.

@78andyp
Copy link
Contributor Author

78andyp commented Nov 30, 2023

Have a requirements issue with the use of "refresh". In every other context "refresh" finds a local nfo or configured scraper to update the info. Here, "refresh" won't update the set overview or set membership making it a one-off.

OK, thanks. I'll add an update button.

@ksooo
Copy link
Member

ksooo commented Dec 1, 2023

Yet another button in the dialog that is already crowded with (way to much) buttons? And will users know the functional difference between update and refresh. It’s just different words for same thing basically. For example, in German both words would even be the same.

@ksooo
Copy link
Member

ksooo commented Dec 1, 2023

If this „refresh“ introduced with this PR is functionally different from the usual „refresh“ functionality it should have its own button, but this button should have a label that is not generic, but hitting the point so that users have a chance to understand what it actually does.

@78andyp
Copy link
Contributor Author

78andyp commented Dec 3, 2023

I've added an 'Update Local Art' button

image

Once Jenkins has done it's thing, i'll merge the diff and resubmit one commit as it's all over the place now.

I've also taken the non-refresh of the library screen out and will submit that as a seperate PR as it is a bug that effects the existing choose art as well - see #24176.

@78andyp 78andyp closed this Dec 3, 2023
@78andyp 78andyp reopened this Dec 3, 2023
@78andyp 78andyp changed the title Update blank movie sets with artwork from Movie Set Information Folder… Update blank movie sets with artwork from Movie Set Information Folder Dec 3, 2023
@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Dec 25, 2023
@jenkins4kodi jenkins4kodi added Rebase needed PR that does not apply/merge cleanly to current base branch and removed Rebase needed PR that does not apply/merge cleanly to current base branch labels Dec 25, 2023
@78andyp 78andyp force-pushed the movie_set_art branch 2 times, most recently from 765aa12 to a742184 Compare December 31, 2023 16:53
@78andyp 78andyp changed the title Update blank movie sets with artwork from Movie Set Information Folder [VideoInfoScanner][GUIDialogVideoInfo] Update blank movie sets with artwork from Movie Set Information Folder Dec 31, 2023
@jenkins4kodi jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Dec 31, 2023
@CrystalP
Copy link
Contributor

CrystalP commented Jan 1, 2024

First of all thanks for your enthusiasm and many PR! It's just bad timing for reviews with the video versions things and (hopefully) close to a release.

Reading the previous comments, I'd also prefer a "Refresh" button in the info screen, consistent in placement and behavior (in spirit at least) with the other uses of the dialog.

To me it means that the art would be set from the MSIF when blank, and updated from the MSIF when not blank.
A user can change his mind about the art in the MSIF for a set, in the same way that he can change the art in a movie folder and use Refresh to grab the new art,

The processing triggered by library scan (all library or single path) doesn't have to change because of the above, setting only the missing art would be consistent with the way movies are scraped as far as I know.

@78andyp
Copy link
Contributor Author

78andyp commented Jan 1, 2024

First of all thanks for your enthusiasm and many PR! It's just bad timing for reviews with the video versions things and (hopefully) close to a release.

Reading the previous comments, I'd also prefer a "Refresh" button in the info screen, consistent in placement and behavior (in spirit at least) with the other uses of the dialog.

I guess the only difference would be this is not a true refresh - we're not going back to scraper source and updating all the set info, just the art. Also, the languate 'use local art' is used on the context menu - so there would be a discrepancy there.

To me it means that the art would be set from the MSIF when blank, and updated from the MSIF when not blank. A user can change his mind about the art in the MSIF for a set, in the same way that he can change the art in a movie folder and use Refresh to grab the new art,

The code currently doesn't change the art if there is art already present, only if the set is blank.

The wiki says 'If you do not use local artwork, and rely on the scrapers to provide your artwork, then there is no requirement for you to use this feature.' - so this would seem to imply that scrapers should take priority over local art (unless in this case there is no scraped art).

The processing triggered by library scan (all library or single path) doesn't have to change because of the above, setting only the missing art would be consistent with the way movies are scraped as far as I know.

@CrystalP
Copy link
Contributor

CrystalP commented Jan 1, 2024

ah yes you're right, a true refresh would attempt to grab the online source first.

edit:

The wiki says 'If you do not use local artwork, and rely on the scrapers to provide your artwork, then there is no requirement for you to use this feature.' - so this would seem to imply that scrapers should take priority over local art (unless in this case there is no scraped art).

You're not required but can press Refresh, and in the case of movies, will receive a popup asking whether to use local or remote info

@CrystalP
Copy link
Contributor

CrystalP commented Jan 1, 2024

ok forget the idea, maybe as a phase 2 if you feel up to it in the future :-)

@78andyp
Copy link
Contributor Author

78andyp commented Jan 1, 2024

As far as I can see there are really only 3 elements to a set - the title of the set, the overview of the set and the artwork of the set.
I guess the first two are only likely to change if an additional movie is added, and I assume then that it will be updated if/when that movie is added.

You are right, remote artwork could be added in the future even without a movie content change and a proper refresh should take that into account, but as you say, it would need a popup as it seems local should always trump remote and if this PR sets local artwork now - does it matter if remote artwork is updated in the future.

Happy to look at that for a future PR.

@CrystalP
Copy link
Contributor

CrystalP commented Jan 30, 2024

Thinking out loud here, not requesting changes but looking for feedback on the idea. Now that I am more familiar with artwork handling, I'm thinking that instead of adding a context menu button / button in info dialog, maybe the function could be integrated in the existing Choose art dialog (after the selection of the art type)?
By adding a shortcut there to the MSIF of that movie set. Would be available only for movie sets and when there is a subfolder for the movie set being edited:

image

The shortcut would lead to the MSIF of the set, not the top level MSIF folder.
The shortcut could alternatively be in the "Browse..." button, but that would be much less discoverable.

The example screenshot in the wiki shows multiple instances of each art type, I don't know how representative of the reality that is - and is your code prepared for that situation?

m_database.SetArtForItem(setDetails.m_iDbId, MediaTypeVideoCollection, movieSetArt);
}
m_database.Close();
return movieSetArt;
Copy link
Contributor

@CrystalP CrystalP Jan 30, 2024

Choose a reason for hiding this comment

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

It doesn't look like the returned art is actually used by the callers and a much cheaper true/false boolean would be enough?

m_database.GetArtForItem(setDetails.m_iDbId, MediaTypeVideoCollection, setArt);
if (!isArt || !setArt.count("poster"))
{
// Movie set currently has no artwork poster (other art types do not seem to be used)
Copy link
Contributor

Choose a reason for hiding this comment

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

Estuary uses the fanart when defined in addition to poster. I don't know about the other skins or about the automatic download.

@78andyp
Copy link
Contributor Author

78andyp commented Jan 31, 2024

Thinking out loud here, not requesting changes but looking for feedback on the idea. Now that I am more familiar with artwork handling, I'm thinking that instead of adding a context menu button / button in info dialog, maybe the function could be integrated in the existing Choose art dialog (after the selection of the art type)? By adding a shortcut there to the MSIF of that movie set. Would be available only for movie sets and when there is a subfolder for the movie set being edited:

image

The shortcut would lead to the MSIF of the set, not the top level MSIF folder. The shortcut could alternatively be in the "Browse..." button, but that would be much less discoverable.

The example screenshot in the wiki shows multiple instances of each art type, I don't know how representative of the reality that is - and is your code prepared for that situation?

Hi

Thanks for the thoughts.

I was thinking of going down a different route - but I think we can combine the two.

Now we have potential refresh functionality I'm thinking that you anbd @ksoo were right at the start - refresh is better than a separate art update (which I agree with you - would probably be better in the choose art dialog with MSIF added).

I was then going to extend the refresh set PR to make sure when a new set is added the MSIF is checked for art as well. I was also going to add support for a set.nfo that could contain an overview tag to set the overview (so that it can be changed independent of a movie.nfo which may screw up the movie metadata).

Then this PR could be closed.

Looking to the future I was wondering if the whole set/movie version/(cinematic universe) could be simplified to nested sets. Each (nested) set could contain either further nested sets or streams (linking back to our previous discussions about playlists - a stream would either be 1 stream in a file or a playlist in an ISO). In theory seasons and episodes could be viewed as sets as well (where the overarching term set is really just referring to a collection), as could music videos.

In essence then there would only be 2 significant entities - a stream or a set - at a database level. A set could be designated to be a season, a universe, a version collection, or just a set of sets (or whatever else we want) and then there would be GUI 'page' assocaited with each set type.

Most set management would still be handled by the scrapers but the MSIF could then become just a set information folder where, for example, you could have within set.nfo XML for the cinematic universe order, or the start trek tv series order etc..

@KarellenX
Copy link
Member

Thought I might ping @rmrector as he implemented the MSIF and he had ideas at the time for expanding its functionality.

@78andyp
Copy link
Contributor Author

78andyp commented Feb 5, 2024

Superceded by #24557.

@78andyp 78andyp closed this Feb 5, 2024
@78andyp 78andyp deleted the movie_set_art branch March 13, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Video Type: Improvement non-breaking change which improves existing functionality Wiki: Needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants