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] Removed refresh on artist/album info #8057

Merged
merged 1 commit into from Oct 4, 2015

Conversation

evilhamster
Copy link
Contributor

There is a problem (thread about it: http://forum.kodi.tv/showthread.php?tid=238853) where when you check for artist/album information and the scraper can't find any information it will get stuck in a loop where you get to reenter the name of the album and/or artist and unless you change the album and/or artist to something the scraper can find it will never show the information page for the album or artist. Since you need to be able to get to the information page to be able to select "get thumb", you are unable to set (or change in the case one was found locally during scanning) for an item that the scraper can't find.

This change does the following change, it disables refresh when showing the information page (for an artist and/or album that hasn't been scraped). The advantage with doing it like this is that just checking the information on an artist and/or album wont change any information without a direct action from the user.

@razzeee @DaveTBlake @zag2me
What do you think?

@razzeee
Copy link
Member

razzeee commented Sep 15, 2015

+1
Nothing wrong with that.

@zag2me
Copy link
Contributor

zag2me commented Sep 15, 2015

Perfect! thank you, this bug has been around for years!

@akva2
Copy link
Contributor

akva2 commented Sep 15, 2015

In fact i fixed it in 2007 but it was brought back as peeps (or vampires - not sure if elan qualifies as a peep) insisted on lookups for files/folders. So 8 years.

@evilhamster
Copy link
Contributor Author

A golden oldie then :)

Made a small change to the commit, in the original I made it did "if (refresh && !m_musicdatabase.HasArtistBeenScraped(params.GetArtistId()))" but since the refresh was set to true in the same place as ClearArtistLastScrapedTime was executed it really didn't make sense. I kept the ClearArtistLastScrapedTime since I think there could be other places in the code that depends on lastscraped beeing null (what the clear function does), but I'm not sure. does anyone know?

If it's a requirement that it should try and scrape material that hasn't been scrape, let me know and I'll try to adapt this pr so that it at only tries to scrape once on checking information to avoid the loops. But from a ui perspective I think I prefer it like this.

@DaveTBlake
Copy link
Member

Will be good to see this bug fixed, was intending to look at it myself but you beat me to it! I would like to check things through on my dev system when I get back to it at the weekend (busy with family stuff at the moment).

My view of UI is that initiating scraping should be a separate action from looking at info. This would require a context menu change, but the hidden automatic scraping is not something all users actually want, especially if the scraper gets it wrong.

@DaveTBlake
Copy link
Member

@evilhamster good stuff!
Finally this checked over. I was looking to fix this in a different place, but this fixes the bug and removes the inadvertent scraping as well.

Can we get this built and merged please.

@razzeee
Copy link
Member

razzeee commented Sep 19, 2015

jenkins build this please

@MartijnKaijser
Copy link
Member

Ignore osx32 build error

@MartijnKaijser MartijnKaijser changed the title [wip][music] Removed refresh on artist/album info [music] Removed refresh on artist/album info Sep 20, 2015
@mkortstiege
Copy link
Member

jenkins build this please.

@stefansaraev
Copy link
Contributor

ignore Llinux64 build error

razzeee added a commit that referenced this pull request Oct 4, 2015
[music] Removed refresh on artist/album info
@razzeee razzeee merged commit 4173817 into xbmc:master Oct 4, 2015
@razzeee razzeee added this to the Jarvis 16.0-alpha4 milestone Oct 4, 2015
@razzeee razzeee added the Type: Fix non-breaking change which fixes an issue label Oct 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue v16 Jarvis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants