[NfoFile] Fix for nfofile with URL #1121

Merged
merged 2 commits into from Jul 11, 2012

Projects

None yet

4 participants

@Karlson2k
Team Kodi member

.nfo files wasn't processed, as it should.
If .nfo file contains scrapper URL that scrapper can be found only if it have same language as default scrapper.
Now it's fixed so it's possible to set default movie scrapper to TMDB (for example) and use kinopoisk for individual files as described here: http://wiki.xbmc.org/index.php?title=Import-export_library#Video_nfo_files_containing_an_URL

@Memphiz
Team Kodi member

No offense ment dude. But you know that we all get an email on each edit you do on your PR texts? ^^

@Karlson2k
Team Kodi member

Yep, didn't take this into account.
Next time I'll do my best. :)

@ghost

@vdrfan you stuff

@mkortstiege
Team Kodi member

Hm, The fallback was disabled and removed from GUI due to the fact it's 100% not transparent for the users. Other than that the code looks wrong as it will blindly add all available scrapers, not just those which are compatible to the users language settings.

@Karlson2k
Team Kodi member

@vdrfan this code is add all available scrappers only for test to math crapper URL in nfo file not for blindly scraping info.
If user put kino.de URL into movie.nfo that should mean that user explicitly want kino.de scraper for that movie even if user set imdb scraper for all movies.
It's described here: http://wiki.xbmc.org/index.php?title=Import-export_library#Video_nfo_files_containing_an_URL, but was broken a long time ago.

@mkortstiege
Team Kodi member

Well, that would have been a complete feature change. Before disabling it by default we used to fallback on the language specified in the assigned scraper. The order was: current scraper, scrapers that match by language | multi, default scraper (in case it's not the current one).

TBH, i am against re-enabling this code by default. I'd like to hear what the others think @cptspiff, @jmarshallnz, @Montellese.

@Karlson2k
Team Kodi member

@vdrfan The current order is: current scraper, default scraper. Scrapers match by language is never used as there is no way to set "scrapers.langfallback" to "true".
Many scrapers is multilingual, but marked as "en". Moreover, even if user set somehow "scrapers.langfallback", there is no warranty that "multi" is include needed language.
So current logic is broken already.
And last but not least argument: this is only test for match scraper URL from nfo file. If user explicitly want specific scraper for specific movie, why should we prevent he/her from this?
And what can be broken for user, that don't use movie.nfo? If no movie.nfo is present than only default scraper is used.

@jmarshallnz typos are corrected.

@Karlson2k
Team Kodi member

@vdrfan And I can't understand about feature change.
This it just implementation of http://wiki.xbmc.org/index.php?title=Import-export_library#Video_nfo_files_containing_an_URL
Again, right now code is not working as described in wiki!

@mkortstiege
Team Kodi member

This is changing the behavior as you're adding all installed/enabled scrapers with passing the "override" bool. AFAIK we never did and I am not sure who published the information in the wiki.
Before the gui setting was disabled, we used to fallback on same and multi-language scrapers only.

The language based fallback was added by me to ensure that we do not scrape information in a foreign language back in the days we shipped with all scrapers.

That said, i feel highly uncomfortable with re-enabling this half-arsed fallback code as-is.

@Karlson2k
Team Kodi member

Again, scraper is only used if it match url in movie.nfo. If user specified explictly scraper for movie, why XBMC should prevent it?
In all other cases only default scraper is used.
Currently, in one hand XBMC allow user to specify alternative specific scraper for movie, but in other hand disallow use of alternative scraper. I can't understand this logic.
For what reason in this case URL in movie.nfo could be used?

You should ether completely disallow scraper selection by movie.nfo or allow user to choose scraper that he/she wants by movie.nfo.

@ghost

^^^ i completely agree with the last sentence there. i always thought this translation nonsense broke that feature - the ability to use a different, specified scraper for a particular item. but you know the old jungle word; never get between the germans and their unstoppable desire to rape every movie there is in the name of their language ;D so i never spoke up..

@mkortstiege
Team Kodi member

Well, if we nuke the language based fallback and go for a try all, do it. Now that the users have to enable the addons, it might work.

@Karlson2k
Team Kodi member

Language based fallback was already nuked some time ago, when it was cut from GUI.
If users don't enable the addons then movie.nfo with URL just will not work.
No harm at all.

@mkortstiege
Team Kodi member

Then you should drop the entire logic and nuke the AddScrapers function.

@Karlson2k
Team Kodi member

We could do so, but it's nice to have directory default scraper at first, than type default scraper at second and all other after them.
So scraper eats Urls from other scrapers. For example TMDB can be used on IMDB url. So order of scrapers processing is does matter.

Then you should drop the entire logic and nuke the AddScrapers function.

@ghost

i won't pull anything with the word 'scrapper' in the commit msg. one p! :)

@ghost

imo this should simply
1) try configured scraper
2) try the rest of the enabled scrapers

@mkortstiege
Team Kodi member

I still think the default should be on the end of the list. Please nuke at least the extra bool introduced and remove the language checks. They don't make sense any longer now.

Oh, and it's scraper :) not scrapper.

@Karlson2k
Team Kodi member

Already notices my typo. I'll fix it :)

@Karlson2k
Team Kodi member

Before any other code change let's define logic at first.
@cptspiff there are two default scrapers: directory default and global type (movie) default. I think that we could process defaults scrapers at first and after them - the rest.

@vdrfan Why do you want to put default one to the end of the list? From my point of view it should have priority on other scrapers as it's selected as default.
I use movie.nfo to avoid any searching - I just put movie URL in it and want to be sure that correct scraper will be used.

@ghost

hence my use of the term selected. imo the default scrapers serve no purpose than being the preselected one when you set content..

@mkortstiege
Team Kodi member

Default at end was to ensure we scrape at least some information when the "community" scrapers fail. Now that we do not fallback by language anymore, we should try the scrapers enabled by the user before pushing the "maybe unconfigured" defaultscraper. That way the chance is high to end up with english meta information instead of none.

@Karlson2k
Team Kodi member

@vdrfan OK. Agree with that logic.
Global default scrapped isn't selectable by user, so it should be checked at last position.

@Karlson2k
Team Kodi member

@cptspiff @vdrfan Done. Rewritten, typos fixed.

@mkortstiege
Team Kodi member

Looks good to me.

@Karlson2k
Team Kodi member

Hopefully last typo (in commit description) fix. :)

@Karlson2k
Team Kodi member

@cptspiff @vdrfan @jmarshallnz Fine?

@jmarshallnz
Team Kodi member

Looks ok - needs a rebase on master is all.

Karlson2k added some commits Jul 5, 2012
@Karlson2k Karlson2k Fix for CNfoFile is looking only for same language scrapers as defaul…
…t scraper and only if "scrapers.langfallback" is true when processing url .nfo files

Seems that this logic was from old builds and wasn't updated.
fb5c8e2
@Karlson2k Karlson2k Get rid of unused "scrapers.langfallback" setting 13a34b0
@Karlson2k
Team Kodi member

@jmarshallnz Done.

@Karlson2k
Team Kodi member

@jmarshallnz Ready to go :)

@jmarshallnz jmarshallnz merged commit eb76c09 into xbmc:master Jul 11, 2012
@tru tru added a commit to RasPlex/plex-home-theatre that referenced this pull request Apr 24, 2014
@tru tru Add support for ListItem.CompositeImage(args)
This accepts the composite image call arguments.

Fixes #1121
16d6a46
@tru tru added a commit to RasPlex/plex-home-theatre that referenced this pull request Apr 24, 2014
@tru tru Add support for ListItem.CompositeImage(args)
This accepts the composite image call arguments.

Fixes #1121
2862a3e
@MilhouseVH MilhouseVH referenced this pull request in FernetMenta/xbmc Dec 6, 2015
Closed

Audio Passthrough Sync issue #339

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment