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

[videolibrary] Assign extra artwork from file system #13859

Merged
merged 1 commit into from Oct 21, 2018

Conversation

@rmrector
Copy link
Contributor

commented May 7, 2018

with a whitelist configuration in AS.xml, and apply the whitelist to scraper results.

Description

Find and assign extra artwork like 'clearlogo' and 'landscape' from the local file system. Configured with a whitelist in advancedsettings.xml, which also applies to artwork from scrapers. Scrapers may not need switches for each individual art type, leaving that up to this configuration.

Files must be named as the name used in the library, so 'clearlogo.png' or 'movie file name-clearlogo.png', not 'logo.png'. Kodi exports the artwork to these file names.

"banner" is still hard-coded for TV shows and seasons. That's still wired up as a fallback "thumb" in some places that will be best handled in another PR. And "fanart" is still hard-coded for most media items, as that is treated differently than other artwork in many parts of Kodi.

Movie set artwork isn't exactly supported; when a new movie set is added Kodi still copies all artwork from one of the movies, which can be limited by movieextraart.

White list configuration modeled on Dave's work on #13848 for the music library.

<advancedsettings>
  <videolibrary>
    <tvshowextraart>
        <arttype>clearlogo</arttype>
        <arttype>landscape</arttype>
    </tvshowextraart>
    <movieextraart>
        <arttype>landscape</arttype>
    </movieextraart>
  </videolibrary>
</advancedsettings>

Plus tvseasonextraart, episodeextraart, and musicvideoextraart.

How Has This Been Tested?

Manually, testing movies and TV shows with a couple configurations. Could use more eyes.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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

@rmrector rmrector force-pushed the rmrector:videolibrary-extraart branch from fc5b3a3 to 43fdc30 May 9, 2018

@sualfred

This comment has been minimized.

Copy link

commented May 12, 2018

@rmrector

Please take a look at this conversation in the board regarding the filenamings:
https://forum.kodi.tv/showthread.php?tid=331822&pid=2734211#pid2734211

@rmrector rmrector force-pushed the rmrector:videolibrary-extraart branch from 43fdc30 to 97ca003 May 17, 2018

@rmrector rmrector changed the title [WIP] [videolibrary] Assign extra artwork from file system [videolibrary] Assign extra artwork from file system May 17, 2018

@rmrector rmrector force-pushed the rmrector:videolibrary-extraart branch from 97ca003 to d4a507d May 25, 2018

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jun 10, 2018

i am against adding this to as.xml
Whatever was done for music was the wrong approach.

@rmrector

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2018

Do you have any suggestions on how to add this to the standard settings? I took a look through the settings implementation and I just don't see how to go about it.

I'm also not sure how to present the options in the GUI; the best I can think of is a simple multiselect dialog populated with art types already in the library plus a button to add an additional type with the keyboard, but that will still be awkward with many artwork types configured for each media type.

@zag2me

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

I seem to remember reading that advancesettings was just the first step as the GUI settings wasn't ready for this type. I may be totally making that up though @DaveTBlake will know ;)

My own personal opinion (if anyone cares) is advancedsettings is perfect for this type of thing. The idea that we remove advancedsettings would imo make our GUI settings far too complicated so its worth a debate rather than saying its wrong. Anyway this is a discussion for the forum I guess: https://forum.kodi.tv/showthread.php?tid=331555

@zag2me

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2018

I have created some new Estuary views to take advantage of the landscape views for TVShows. It looks awesome but doing it manually, or with artwork beef is a pain. @MartijnKaijser any ideas on how to move this forward so we can get this on a par with the new music artwork functionality? That has been very positively received in the artwork creators community.

@Rechi Rechi force-pushed the rmrector:videolibrary-extraart branch from d4a507d to 82df06a Sep 26, 2018

@rmrector rmrector force-pushed the rmrector:videolibrary-extraart branch from 82df06a to 32888ac Oct 8, 2018

ret.push_back("poster");
ret.push_back("fanart");
ret = { "thumb" };
extraart = g_advancedSettings.m_videoEpisodeExtraArt;

This comment has been minimized.

Copy link
@Rechi

Rechi Oct 8, 2018

Member

g_advancedSettings is gone in master

@rmrector rmrector force-pushed the rmrector:videolibrary-extraart branch from 32888ac to 9c81d82 Oct 8, 2018

artworkMap.push_back(arttype->FirstChild()->ValueStr());
arttype = arttype->NextSibling("arttype");
}
}

This comment has been minimized.

Copy link
@garbear

garbear Oct 9, 2018

Member

Newline at end of file

@@ -1480,7 +1477,7 @@ namespace VIDEO
{
for (auto& it : pItem->GetVideoInfoTag()->m_coverArt)
{
if (art.find(it.m_type) == art.end())
if (find(artTypes.begin(), artTypes.end(), it.m_type) != artTypes.end() && art.find(it.m_type) == art.end())

This comment has been minimized.

Copy link
@garbear

garbear Oct 9, 2018

Member

std::find

This comment has been minimized.

Copy link
@garbear

garbear Oct 9, 2018

Member

And needs #include <algorithm>

[videolibrary] Assign extra artwork from file system
with a whitelist configuration in AS.xml, and apply the whitelist
to scraper results.

@rmrector rmrector force-pushed the rmrector:videolibrary-extraart branch from 9c81d82 to f7775b4 Oct 12, 2018

@rmrector

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

@garbear Thanks for the code reviews. I've had some difficulties compiling this week, but I'll submit updates to the others soon.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

This brings local video art inline with music. Both are a first step, providing new functionality via advanced settings, but improved GUI route to these settings needed for v19

@DaveTBlake DaveTBlake merged commit 2b8d606 into xbmc:master Oct 21, 2018

1 check passed

default You're awesome. Have a cookie
Details

@rmrector rmrector deleted the rmrector:videolibrary-extraart branch Oct 21, 2018

@rmrector rmrector referenced this pull request Oct 22, 2018
5 of 13 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.