Skip to content

ADD: No-Op scraper support to allow nfo based only library #1192

Merged
merged 1 commit into from Apr 1, 2013

5 participants

@koying
Team Kodi member
koying commented Jul 21, 2012

As discussed here: http://forum.xbmc.org/showthread.php?tid=127896&pid=1152628#pid1152628

Allows to have a nfo-based only library.

@mkortstiege
Team Kodi member

In case other scrapers are enabled you'll still fallback and scrape from the web. You should add a noop check in NfoFile.cpp:104+ to prevent this - in case its the desired behavior?

Mind moving the Scraper.cpp returns after the DEBUG log blocks so we can see what scraper is used in the log?

@koying
Team Kodi member
koying commented Jul 22, 2012

Thanks. Updated.

@ghost
ghost commented Jul 24, 2012

if we absolutely need such a mode it should be done by code, not by a butchered scraper + id checks.

some tick in the content dialog 'fetch online data' or thereabout. if not enabled on the source, scanner handles appropriately.

@koying
Team Kodi member
koying commented Jul 24, 2012

That was my first idea, but won't that break the skins (or require updating tall of hem)? I'm not familiar on how UI stuff works in XBMC.

@ghost
ghost commented Jul 24, 2012

no, the buttons in that dialog are filled from code.

@koying
Team Kodi member
koying commented Jul 25, 2012

Not quite, unless I'm mistaken.

CGUIDialogContentSettings defines:

#define CONTROL_CONTENT_TYPE        3
#define CONTROL_SCRAPER_LIST        4
#define CONTROL_SCRAPER_SETTINGS    6

If we want it to do the proper way, "CONTROL_SCRAPER_LIST" & "CONTROL_SCRAPER_SETTINGS" should be under another container which would also contain the "Use local info only" checkbox.
The CONTROL_SCRAPER_... should be disabled if the checkbox is ticked.

So, IMHO, that would require skin rework.

Another solution would be to put the checkbox under CONTROL_SCRAPER_SETTINGS. That would prevent skin issues and make the code prettier, but would be a usability nonsense ;)

@ghost
ghost commented Jul 25, 2012

Then skin changes it is

@koying
Team Kodi member
koying commented Jul 26, 2012

Nevermind.
Scrapers are assumed everywhere so, besides butchering the skins, implementing a "no-scraper" checkbox would take a lot of time, be error-prone and butcher the code for zero functional advantage over my "butchered scraper".

I'd suggest to leave the pull request open so that if some need the functionality, they can use this easily mergeable patch.

@jmarshallnz
Team Kodi member

You could do it without skin changes by having an internal metadata.null or similar as you've already done, but I don't think you'd need a noop addition to the scraper XML - rather you could just set the library XML to empty and assume XML empty == noop ?

@koying
Team Kodi member
koying commented Aug 5, 2012

Sure. Would the patch be deemed acceptable without the "noop" attribute, even withe the "butchered" scrapper?
If so, I'll dig into it.

@SlrG
SlrG commented Aug 25, 2012

+1 for integrating this into XBMC. In my opinion it is a desperately needed feature for people using external scrapers like Ember Media Manager to have full control of the sraping process and make sure only local data is used.

Thank you very much for your work Koying!

@XBMC Team: Please reconsider this pull-request. :)

@jmarshallnz
Team Kodi member

@koying: IMO if library="" then we can assume the scraper is a noop and other stuff can flow from there, yes.

@cptspiff: is that an acceptable compromise?

@ghost
ghost commented Aug 26, 2012

yes anything without a useless butchered scraper will fly.

@koying
Team Kodi member
koying commented Aug 26, 2012

@jmarshallnz I might be wrong, but seeing:

CAddon::CAddon(const AddonProps &props)
  : m_props(props)
  , m_parent(AddonPtr())
{
  if (props.libname.IsEmpty()) BuildLibName();
  else m_strLibName = props.libname;

... I understand that, if the library name is blank, it means use the default name ("default.xml"?), so this couldn't be used to detect an "noop" addon.

@cptspiff
Would

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<scraper framework="1.1">
</scraper>

be acceptable?
IMO, in this instance, an empty scrapper is not "butchered useless". It is just a scrapper doing nothing, which is what we seek and a valid case.
The surrounding code is just a mean to handle this case of empty scrapper.

@koying
Team Kodi member
koying commented Jan 31, 2013

@jmarshallnz @cptspiff Is it ok for merge in the current state?

@jmarshallnz
Team Kodi member

I guess the only thing I don't like about it is that it's a special case (i.e. we have to remember to take care of IsNoop() in a bunch of places). How many of those are actually required - i.e. what happens currently with a scraper without CreateSearchURL() et. al. ?

Hmm, maybe it's just the naming I don't like (maybe scraper->CanScrape()?) ?

@koying
Team Kodi member
koying commented Feb 2, 2013

IIRC, without the noop(), the import fails and nothing is imported, which is logical.
There was a loophole at one point, because a badly written scrapper I used added "empty" items even if it was not found in its database.
Would you prefer something like that? Seems even dirtier to me ;)

@jmarshallnz
Team Kodi member

I think the key is to have as little special-casing as possible. If what you have is minimal, then that's fine.

@MartijnKaijser
Team Kodi member

@koying
please rebase

@koying koying merged commit 9fdb68a into xbmc:master Apr 1, 2013
@koying koying deleted the koying:noop-scraper branch Apr 1, 2013
@LongChair LongChair added a commit to plexinc/plex-home-theater-public that referenced this pull request Jun 13, 2014
@LongChair LongChair Fix <selectedcolor> doesn't work for ListItem.IsPlaying item in Playe…
…rControls #1192

Seemed to be due to selected state of items being incorrect due to the fact that listgroups selected states are not being propagated in sub listgroups controls.
e1afd1c
@LongChair LongChair added a commit to RasPlex/plex-home-theatre that referenced this pull request Aug 21, 2014
@LongChair LongChair Fix <selectedcolor> doesn't work for ListItem.IsPlaying item in Playe…
…rControls #1192

Seemed to be due to selected state of items being incorrect due to the fact that listgroups selected states are not being propagated in sub listgroups controls.
063c419
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.