[Music]Fix change of information provider for albums and artists #12597

DaveTBlake · 2017-07-31T17:53:22Z

A follow up from #12120, the improvements to scraping additional artist and album information, that fixes flaws in how changes to artist and album information providers are selected and applied.

Music DB schema changes include:

Remove albuminfosong table, as storing scraped tracks is deprecated.
Adding the release group MBID, scrapedMBID flag and lastScraped to album and artist views. This fixes loss of release group MBID when refreshing with a scraper that does not return release group id e.g. TADB scraper or from NFO files, and needed for JSON API exposure of properties.
Replacing the content table with (more accurately named) infosetting table and store idSetting in artist and album tables.

The "Change Info provider" context menu action was truely broken, especially on saying saying yes to the subsequent "Do you want to refresh information for all items within this path?". For example:

doing so for an album it used the song ids as if they were album ids and refreshed the info for those.
doing it on "*all albums" set the artist scraper, and again used song ids.
doing it on a parent folder could hang.
from genres node, a path and settings were stored, but these settings were not applied when adding artists, or albums with songs of that genre, or "Query info for all".
Also "content" (if anyone did use this feature) was not cleaned up when orphaned.

The user access to set/clear these settings was also so limited as to be useless.

If a user had set scraper settings for an artist or album, then there was no way to reset them to default. The only way to know they were non-default was to look at the settings for each item at a time, and then they could not be reset, only set something else specifically.
There was no practical way to set the setting for more than one artist or album at a time (genre did not work, and anyway what or those with more than one genre).
The only way to set the default settings themselves was to go to System > Settings > Media > Music and change the addon used, then go to System >addons > My Addons > Information Providers > Album (or artist) and change the addon settings.
I suspect that all users have done in the past is to set the default settings, and then apply them to everything uniformly.

This PR addresses both aspects, to do this there is a UX/UI change

"Change information provider" is only on the context menu when the current item is an album or artist (from any "artists" or "albums" node or smart playlist)
The action is followed by submenu offering to set information provider for
a) current item,
b) all items on node, filtered by criteria
c) default (for all).
This is followed by the CGUIDialogContentSetting dialog to select the info provider addon to use and change settings. Then a comfirmation message is displayed before the setting change is applied. Finally the user is asked if they want to query for additional information using these settings immediately (otherwise they can use "Query Info For all" later).

This gives the facility to set information provider for multiple artists or albums, and to reset to default, without cluttering ordinary music library use.

Rechi · 2017-07-31T18:10:57Z

jenkins build this please

DaveTBlake · 2017-08-02T17:22:08Z

Test build available as 9095c13, so for win32 in particular
http://mirrors.kodi.tv/test-builds/windows/win32/KodiSetup-20170801-9095c13-HEAD-x86.exe

(Spotted a rebasing error that left repeated tag in exported NFOs. Fixed but not in test build)

DaveTBlake · 2017-08-05T15:19:59Z

Some discussion on Slack if "Set for this artist", "Set for all artists shown", and "Set default information provider" are acceptale on a menu. That latter is long enough to need scrolling on Estuary, and translations will probably be longer.

Meanwhile getting some test feedback on the forum.

Build errors not related to this PR

DaveTBlake · 2017-08-09T17:40:10Z

New test build available as 20170809-ec166d8, but for win32 in particular (had to kick off another build) it is http://mirrors.kodi.tv/test-builds/windows/win32/KodiSetup-20170809-a130a8e139-InfoSettings-x86.exe

scott967 · 2017-08-13T02:56:42Z

Created about a dozen test cases, also did some free-play testing. All tests passed (DB state consistent with info provider changes made via the UI).

DaveTBlake · 2017-08-14T10:47:55Z

This change has received thorough testing, see https://forum.kodi.tv/showthread.php?tid=319356 so I would like to merge it. Any team member care to review it?

Paxxi

some nitpicks and the find("?") needs a fix

xbmc/filesystem/MusicDatabaseDirectory.cpp

@@ -121,7 +121,8 @@ void CMusicDatabaseDirectory::ClearDirectoryCache(const std::string& strDirector

 bool CMusicDatabaseDirectory::IsAllItem(const std::string& strDirectory)
 {
-  if (StringUtils::EndsWith(strDirectory, "/-1/"))
+  //Last query parameter, ignoring any appended options is -1
+  if (StringUtils::EndsWith(strDirectory.substr(0, strDirectory.find("?")), "/-1/"))


xbmc/music/MusicDatabase.h

@@ -429,8 +425,9 @@ class CMusicDatabase : public CDatabase
  /////////////////////////////////////////////////
  // Scraper
  /////////////////////////////////////////////////
-  bool SetScraperForPath(const std::string& strPath, const ADDON::ScraperPtr& info);
-  bool GetScraperForPath(const std::string& strPath, ADDON::ScraperPtr& info, const ADDON::TYPE &type);
+  bool SetScraper(int id, const CONTENT_TYPE &content, const ADDON::ScraperPtr& scraper);


xbmc/music/windows/GUIWindowMusicNav.cpp

@@ -253,6 +254,138 @@ std::string CGUIWindowMusicNav::GetQuickpathName(const std::string& strPath) con
  }
 }

+bool CGUIWindowMusicNav::ManageInfoProvider(const CFileItemPtr &item)


MusicDB changes: Add release group MBID, scrapedMBID flag and lastScraped to views. GetAlbum and GetArtist now fetch these values, so remove separate queries for values. This fixes loss of release group MBID when refreshing with a scraper that does not return release group id e.g. TADB scraper or from NFO files, and needed for JSON API exposure of properties. Rework info provider settings to be artist and album id specific rather than path. Replace content with infosetting table and store idSetting in artist and album tables. Add clean up of orphaned settings. Remove albuminfosong table, storing scraped tracks is deprecated. Fix having "Change information provider" on context menu for parent folder and *all items. IsAllItem() handles paths with options at the end. Fix refresh of information for all items within this path after info provider change for a single artist or album. Add facility to set information provider for multiple artists or albums. Unlike the path based attempt before this works. From context menu set information provider for a) current item, b) all items on node, c) default (for all).

DaveTBlake · 2017-08-15T08:56:10Z

@Paxxi have fixed your comments, thanks for those. Could you approve, please check you are happy with CMusicDatabase::GetScraper

Paxxi

looks good to me

DaveTBlake added Component: Database Type: Fix non-breaking change which fixes an issue Component: Music v18 Leia labels Jul 31, 2017

DaveTBlake added this to the L 18.0-alpha1 milestone Jul 31, 2017

DaveTBlake force-pushed the InfoSettings branch from 9aee9ad to ae0486d Compare July 31, 2017 22:27

DaveTBlake force-pushed the InfoSettings branch from ae0486d to c761c43 Compare August 3, 2017 15:07

DaveTBlake force-pushed the InfoSettings branch from c761c43 to a130a8e Compare August 9, 2017 16:06

Paxxi requested changes Aug 14, 2017

View reviewed changes

DaveTBlake force-pushed the InfoSettings branch from a130a8e to 873c1a3 Compare August 15, 2017 08:50

Paxxi approved these changes Aug 17, 2017

View reviewed changes

DaveTBlake merged commit c63e767 into xbmc:master Aug 17, 2017

DaveTBlake deleted the InfoSettings branch August 17, 2017 08:39

DaveTBlake mentioned this pull request Nov 1, 2017

[Music]Information Provider dialog #12987

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Music]Fix change of information provider for albums and artists #12597

[Music]Fix change of information provider for albums and artists #12597

DaveTBlake commented Jul 31, 2017

Rechi commented Jul 31, 2017

DaveTBlake commented Aug 2, 2017 •

edited

Loading

DaveTBlake commented Aug 5, 2017 •

edited

Loading

DaveTBlake commented Aug 9, 2017 •

edited

Loading

scott967 commented Aug 13, 2017

DaveTBlake commented Aug 14, 2017 •

edited

Loading

Paxxi left a comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

DaveTBlake commented Aug 15, 2017

Paxxi left a comment

[Music]Fix change of information provider for albums and artists #12597

[Music]Fix change of information provider for albums and artists #12597

Conversation

DaveTBlake commented Jul 31, 2017

Rechi commented Jul 31, 2017

DaveTBlake commented Aug 2, 2017 • edited Loading

DaveTBlake commented Aug 5, 2017 • edited Loading

DaveTBlake commented Aug 9, 2017 • edited Loading

scott967 commented Aug 13, 2017

DaveTBlake commented Aug 14, 2017 • edited Loading

Paxxi left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

DaveTBlake commented Aug 15, 2017

Paxxi left a comment

Choose a reason for hiding this comment

DaveTBlake commented Aug 2, 2017 •

edited

Loading

DaveTBlake commented Aug 5, 2017 •

edited

Loading

DaveTBlake commented Aug 9, 2017 •

edited

Loading

DaveTBlake commented Aug 14, 2017 •

edited

Loading