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

[Music]Prompt skip artist or album nfo files and refresh from remote sites #15791

Merged
merged 1 commit into from Mar 26, 2019

Conversation

DaveTBlake
Copy link
Member

Make refresh of artist or album additional information and artwork from music info dialog more consistent with video info dialog behaviour by allowing user to skip existing nfo files.

Currently, if an nfo file is found that is always applied so user has to notice this happened, and go and delete the nfo if they want to fetch new remote data to replace it. This PR adds the display of the following prompt when an nfo file is found (matches the video equivalent).
Imgur

@KarellenX sorry this took so long for me to get around to, should make the manual management of additional music data a bit easier.

Tested for artists and albums both with and without nfo files, and that it does not impact automated scraping. Fixing an inconsistency in bejaviour and an improvement so would like to get into v18.2

@DaveTBlake DaveTBlake added Type: Improvement non-breaking change which improves existing functionality Component: Music v18 Leia labels Mar 21, 2019
@DaveTBlake DaveTBlake added this to the Leia 18.2-rc1 milestone Mar 21, 2019
…file and fetch metadata and art from remote sites
@KarellenX
Copy link
Member

KarellenX commented Mar 22, 2019

Artrists works as expected.

@KarellenX
Copy link
Member

KarellenX commented Mar 22, 2019

Was unable to complete the test with Albums as I receive Integral Scraper Addon error.

@ronie Maybe one for you?
My log with error... https://paste.kodi.tv/ekunudivug.kodi Line 905

  1. Select Refresh from Album Information Page
  2. Select No to the question Locally stored information found. Ignore and refresh from Internet?
  3. Scraper error warning displays

Selecting Yes does not cause the error.

Changing to Universal Album Scraper also causes an error...
Downloading album information failed

@ronie
Copy link
Member

ronie commented Mar 22, 2019

no idea what's going on there, perhaps @DaveTBlake can make sense of it.

on line 800, your local .nfo is passed to metadata.integral.albums (so far so good)
but then on line 813 suddenly metadata.albums.universal scraper is being used ???

@DaveTBlake
Copy link
Member Author

DaveTBlake commented Mar 22, 2019

The errors do not relate to this PR. Having album.nfo files created by exporting metadata that had been scraped by an earlier version of the beta Python album scraper caused scraper errors. Specifically the syntax of the available art urls was preventing the nfo scraper from running correctly. The following tag in the album.nfo

<thumb spoof="" cache="" aspect="thumb">http://coverartarchive.org/release/59b5a40b-e2fd-3f18-a218-e8c9aae12ab5/5270052042.jpg</thumb>

when parsed by the NfoURL xml scraper resulted in a request to Musicbrainz
https://musicbrainz.org/ws/2/release/59b5a40b-e2fd-3f18-a218-e8c9aae12ab5/5270052042.jpg
which of course failed.

That accounts for the xml scraper error at least, and I can repeat it at will. Not so sure why the log shows the Python scraper running immediately before that - maybe fast fingers from Karellen?
EDIT - ah it seems that the clever scraper core will loop through the scrapers it has to get the nfo read, so maybe the Python scraper (not sure what version was running) failed so it tired the xml one.

Test build for this PR here http://mirrors.kodi.tv/test-builds/windows/win64/KodiSetup-20190321-b207fa6d-PR15791-merge-x64.exe Retesting with older NFO files (no Python scraper involvement) this is PR worked correctly.

@KarellenX
Copy link
Member

I have tested the new build using NFO Files created from data scraped by the Universal XML scrapers.

Artists & Albums

  1. The new option to ignore or not ignore local data works correctly for NFO Files
  2. Artwork is not refreshed. The same images are displayed, even when the local images are replaced with different images. The following image displays the discrepancy between correct artwork in the Choose Art dialogue and the Information page image in the background still displaying the old image.

Performing the same action in the Video library will update the images.

refreshartwork

@KarellenX
Copy link
Member

KarellenX commented Mar 23, 2019

A little bit more testing for Item 2, and it has become apparent that the Video Library also does not refresh artwork either. Artwork changing on the first test run may have been coincidental and changed due to cache timing.

@DaveTBlake
Copy link
Member Author

@KarellenX thanks for testing and confirming that the change of PR is working correctly.

The lack of refresh of image cache (so changed local image file content gets applied immediately rather than a day later) is a separate and historic issue.

@arnova
Copy link
Member

arnova commented Mar 24, 2019

Good work. Actually I forgot to implement this for music back then when I did it for video.

@ronie
Copy link
Member

ronie commented Mar 24, 2019

ah it seems that the clever scraper core will loop through the scrapers it has to get the nfo read, so maybe the Python scraper (not sure what version was running) failed so it tired the xml one.

can we make it even more clever and make it stop doing that?
xml and python scrapers are not compatible.
you can't pass the 'nfourl' result of an xml scraper to a 'getdetails' call of a python scraper.

@DaveTBlake
Copy link
Member Author

can we make it even more clever and make it stop doing that?

If my guess at what happened is correct then obviously we need to do that, but not part of this PR :)
Can yoy make a comment on the Python scraper PR to check what happens when scraping from NFO with Python scraper fails so we either switch to xml scraping correctly or don't switch.

Anyway this ssems ready to merge unless there are objections

@DaveTBlake DaveTBlake mentioned this pull request Mar 25, 2019
6 tasks
@DaveTBlake DaveTBlake merged commit d60f2ff into xbmc:master Mar 26, 2019
@DaveTBlake DaveTBlake deleted the IgnoreNFORefresh branch March 26, 2019 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Music Type: Improvement non-breaking change which improves existing functionality v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants