Merge pull request xbmc#27321 from damienruscoe/nfofile_refactor

[NFO] File parsing refactor

Author: garbear

xbmc/NfoFile.cpp

@@ -27,105 +27,141 @@
 using namespace XFILE;
 using namespace ADDON;
 
-CInfoScanner::InfoType CNfoFile::Create(const std::string& strPath,
-                                        const ScraperPtr& info,
-                                        int index)
+// Forward declarations
+namespace
 {
-  m_info = info; // assume we can use these settings
-  m_type = ScraperTypeFromContent(info->Content());
-  if (Load(strPath) != 0)
-    return CInfoScanner::InfoType::NONE;
+std::vector<ScraperPtr> GetScrapers(AddonType type, const ScraperPtr& selectedScraper);
 
-  CFileItemList items;
-  bool bNfo=false;
-  bool overrideNfo{false};
+} // unnamed namespace
 
-  if (m_type == AddonType::SCRAPER_ALBUMS)
+CInfoScanner::InfoType CNfoFile::TryParsing(ADDON::AddonType addonType) const
+{
+  using enum CInfoScanner::InfoType;
+  using enum ADDON::AddonType;
+
+  if (addonType == SCRAPER_ALBUMS)
   {
     CAlbum album;
-    bNfo = GetDetails(album);
+    return GetDetails(album) ? FULL : NONE;
   }
-  else if (m_type == AddonType::SCRAPER_ARTISTS)
+  if (addonType == SCRAPER_ARTISTS)
   {
     CArtist artist;
-    bNfo = GetDetails(artist);
+    return GetDetails(artist) ? FULL : NONE;
   }
-  else if (m_type == AddonType::SCRAPER_TVSHOWS || m_type == AddonType::SCRAPER_MOVIES ||
-           m_type == AddonType::SCRAPER_MUSICVIDEOS)
+  if (addonType == SCRAPER_MOVIES || addonType == SCRAPER_TVSHOWS ||
+      addonType == SCRAPER_MUSICVIDEOS)
   {
-    CVideoInfoTag details;
-    // Find nth <movie> tag (index is 1 based)
-    // If it doesn't exist then set bNfo to false
-    if (m_type == AddonType::SCRAPER_MOVIES)
-    {
-      m_headPos = m_doc.find("<movies>");
-      while (index-- > 0)
-      {
-        m_headPos = m_doc.find("<movie", m_headPos + 1);
-        if (m_headPos == std::string::npos)
-          break;
-      }
-    }
-    bNfo = m_headPos != std::string::npos ? GetDetails(details) : false;
-    overrideNfo = details.GetOverride();
+    if (CVideoInfoTag details; GetDetails(details))
+      return details.GetOverride() ? OVERRIDE : FULL;
   }
+  return NONE;
+}
 
-  std::vector<ScraperPtr> vecScrapers = GetScrapers(m_type, m_info);
-
-  // search ..
-  int res = -1;
-  for (const auto& scraper : vecScrapers)
-    if ((res = Scrape(scraper, m_scurl, m_doc)) == 0 || res == 2)
+bool CNfoFile::SeekToMovieIndex(int index)
+{
+  // Find nth <movie> tag (index is 1 based)
+  m_headPos = m_doc.find("<movies>");
+  while (index-- > 0)
+  {
+    m_headPos = m_doc.find("<movie", m_headPos + 1);
+    if (m_headPos == std::string::npos)
       break;
+  }
+  return m_headPos != std::string::npos;
+}
 
-  if (res == 2)
+CInfoScanner::InfoType CNfoFile::TryParsing(const CURL& nfoPath,
+                                            ADDON::ContentType contentType,
+                                            int index /* =1 */)
+{
+  if (Load(nfoPath) != 0) // Setup m_doc and m_headPos
     return CInfoScanner::InfoType::ERROR_NFO;
-  if (bNfo)
-  {
-    if (!m_scurl.HasUrls())
-    {
-      if (overrideNfo || m_doc.find("[scrape url]") != std::string::npos)
-        return CInfoScanner::InfoType::OVERRIDE;
-      else
-        return CInfoScanner::InfoType::FULL;
-    }
-    else
-      return CInfoScanner::InfoType::COMBINED;
-  }
-  return m_scurl.HasUrls() ? CInfoScanner::InfoType::URL : CInfoScanner::InfoType::NONE;
+
+  const AddonType addonType = ScraperTypeFromContent(contentType);
+
+  if (addonType == ADDON::AddonType::SCRAPER_MOVIES && !SeekToMovieIndex(index))
+    return CInfoScanner::InfoType::NONE;
+
+  return TryParsing(addonType);
+}
+
+CInfoScanner::InfoType CNfoFile::Create(const std::string& nfoPath,
+                                        const ScraperPtr& info,
+                                        int index)
+{
+  /* `TryParsing` creates a close approximation to the desired result.
+   * The desired result would be knowing if any valid URLs have been
+   * found in the NFO which could be interpreted by a scraper.
+   * Determining if any valid URLs exist in the NFO file is an expensive
+   * operation so should only be called if necessary. If the approximate
+   * result from `TryParsing` is sufficient then use that result.
+   *
+   * Below is a table to show the result from `TryParsing` and the
+   * potential desired results.
+   *
+   * | result   | Could be converted to:     |
+   * | -------- | -------------------------- |
+   * | NONE     | URL NONE                   |
+   * | OVERRIDE | COMBINED OVERRIDE          |
+   * | FULL     | COMBINED OVERRIDE FULL     |
+   *
+   * The following call to `SearchNfoForScraperUrls` will generate the
+   * desired result from this approximation.
+   *
+   * This call is expensive as it encodes the NFO file into a URL param
+   * and executes a python interpreter for each installed python scraper.
+  */
+  const CInfoScanner::InfoType result = TryParsing(CURL{nfoPath}, info->Content(), index);
+  if (result == CInfoScanner::InfoType::ERROR_NFO)
+    return CInfoScanner::InfoType::NONE;
+  return SearchNfoForScraperUrls(result, info);
 }
 
-// return value: 0 - success; 1 - no result; skip; 2 - error
-int CNfoFile::Scrape(const ScraperPtr& scraper, CScraperUrl& url, const std::string& content)
+CInfoScanner::InfoType CNfoFile::SearchNfoForScraperUrls(CInfoScanner::InfoType parseResult,
+                                                         const ScraperPtr& info)
 {
-  if (scraper->IsNoop())
-  {
-    url = CScraperUrl();
-    return 0;
-  }
+  using enum CInfoScanner::InfoType;
 
-  scraper->ClearCache();
+  SetScraperInfo(info); // assume we can use these settings
+  const AddonType addonType = ScraperTypeFromContent(info->Content());
 
-  try
-  {
-    url = scraper->NfoUrl(content);
-  }
-  catch (const CScraperError &sce)
+  for (const auto& scraper : GetScrapers(addonType, info))
   {
-    CVideoInfoDownloader::ShowErrorDialog(sce);
-    if (!sce.FAborted())
-      return 2;
+    if (scraper->IsNoop())
+    {
+      m_scurl = CScraperUrl();
+      break;
+    }
+
+    scraper->ClearCache();
+    try
+    {
+      m_scurl = scraper->NfoUrl(m_doc);
+    }
+    catch (const CScraperError& sce)
+    {
+      CVideoInfoDownloader::ShowErrorDialog(sce);
+      if (!sce.FAborted())
+        return ERROR_NFO;
+    }
+
+    if (m_scurl.HasUrls())
+      return (parseResult == FULL || parseResult == OVERRIDE) ? COMBINED : URL;
   }
 
-  return url.HasUrls() ? 0 : 1;
+  if (parseResult == FULL && m_doc.find("[scrape url]") != std::string::npos)
+    return OVERRIDE;
+
+  return parseResult;
 }
 
-int CNfoFile::Load(const std::string& strFile)
+int CNfoFile::Load(const CURL& nfoPath)
 {
   Close();
   XFILE::CFile file;
   std::vector<uint8_t> buf;
-  if (file.LoadFile(strFile, buf) > 0)
+  if (file.LoadFile(nfoPath, buf) > 0)
   {
     m_doc.assign(reinterpret_cast<char*>(buf.data()), buf.size());
     m_headPos = 0;
@@ -142,7 +178,10 @@ void CNfoFile::Close()
   m_scurl.Clear();
 }
 
-std::vector<ScraperPtr> CNfoFile::GetScrapers(AddonType type, const ScraperPtr& selectedScraper)
+namespace
+{
+
+std::vector<ScraperPtr> GetScrapers(AddonType type, const ScraperPtr& selectedScraper)
 {
   AddonPtr addon;
   ScraperPtr defaultScraper;
@@ -180,3 +219,5 @@ std::vector<ScraperPtr> CNfoFile::GetScrapers(AddonType type, const ScraperPtr&
 
   return vecScrapers;
 }
+
+} // unnamed namespace

xbmc/NfoFile.h

@@ -13,6 +13,7 @@
 //////////////////////////////////////////////////////////////////////
 
 #include "InfoScanner.h"
+#include "URL.h"
 #include "addons/Scraper.h"
 #include "utils/XBMCTinyXML.h"
 
@@ -33,7 +34,7 @@ class CNfoFile
   CInfoScanner::InfoType Create(const std::string&, const ADDON::ScraperPtr&, int index = 1);
 
   template<class T>
-  bool GetDetails(T& details, const char* document = nullptr, bool prioritise = false)
+  bool GetDetails(T& details, const char* document = nullptr, bool prioritise = false) const
   {
     CXBMCTinyXML doc;
     if (document)
@@ -51,17 +52,19 @@ class CNfoFile
   ADDON::ScraperPtr GetScraperInfo() { return m_info; }
   const CScraperUrl &ScraperUrl() const { return m_scurl; }
 
-  static int Scrape(const ADDON::ScraperPtr& scraper, CScraperUrl& url, const std::string& content);
-
-  static std::vector<ADDON::ScraperPtr> GetScrapers(ADDON::AddonType type,
-                                                    const ADDON::ScraperPtr& selectedScraper);
-
 private:
+  CInfoScanner::InfoType TryParsing(ADDON::AddonType addonType) const;
+  CInfoScanner::InfoType TryParsing(const CURL& nfoPath,
+                                    ADDON::ContentType contentType,
+                                    int index = 1);
+  CInfoScanner::InfoType SearchNfoForScraperUrls(CInfoScanner::InfoType parseResult,
+                                                 const ADDON::ScraperPtr& info);
+  bool SeekToMovieIndex(int index);
+
   std::string m_doc;
   size_t m_headPos = 0;
   ADDON::ScraperPtr m_info;
-  ADDON::AddonType m_type{};
   CScraperUrl m_scurl;
 
-  int Load(const std::string&);
+  int Load(const CURL&);
 };

xbmc/video/tags/VideoTagLoaderFFmpeg.cpp

@@ -191,6 +191,12 @@ CInfoScanner::InfoType CVideoTagLoaderFFmpeg::LoadMKV(CVideoInfoTag& tag,
     }
     else
     {
+      /*! @todo Investigate if this is correct.
+       * The first parameter to `CNfoFile::Create` is the **path** to the NFO file.
+       * `content` here appears to be the __contents__ of an NFO file.
+       * If this assumption is correct the parsing will always fail and
+       * `nfo.ScraperUrl()` will always return an empty URL.
+       */
       nfo.Create(content, m_info);
       m_url = nfo.ScraperUrl();
       return CInfoScanner::InfoType::URL;