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] fix missing lyrics from tag when playing from musicdb url #7887

Merged
merged 1 commit into from
Aug 26, 2015

Conversation

mkortstiege
Copy link
Member

This fixes an issue where the lyrics from tags are not properly shown when playback was started from a musicdb:// url.

This is caused by using the wrong item path when calling the infotag loader. While at it, I've removed the dupe property handling and injected the lyrics to the current item info tag instead.

http://trac.kodi.tv/ticket/16216

@Montellese, @Paxxi

@mkortstiege
Copy link
Member Author

jenkins build this please.

@mkortstiege mkortstiege added this to the Jarvis 16.0-alpha2 milestone Aug 25, 2015
@Paxxi
Copy link
Member

Paxxi commented Aug 25, 2015

This bypasses the database loading entirely so anything downloaded won't show. Can't we load from db as usual and do the extra loading in infomanager::setitem ? That way we won't hit every file on disk and only the ones actually played.

@mkortstiege
Copy link
Member Author

This bypasses the database loading entirely so anything downloaded won't show. Can't we load from db as usual and do the extra loading in infomanager::setitem ? That way we won't hit every file on disk and only the ones actually played.

This only happens when the infomanager requests additional info for he currently playing item. The block is only there to fetch the lyrics from the actual tag (not database). The already known tag information are not overwritten. Unless i read the code utterly wrong i think it's a valid fix.

@mkortstiege
Copy link
Member Author

Fix is confirmed working. @Montellese comments?

// since we're going to load additional tag information we
// need to replace the library path with the actual file path.
// this has to be done before creating the infotag loader.
pItem->SetPath(pItem->GetMusicInfoTag()->GetURL());

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@mkortstiege
Copy link
Member Author

Updated. @Montellese, instead of modifying the actual item pointer i've changed it to create a new item based on the translated path. Also adjusted the comment.

@Montellese
Copy link
Member

I'll just repeat my previous comment:
Maybe blindly using CFileItem::GetPath() in CMusicInfoTagLoaderFactory::CreateLoader is not the best approach and we should first check if the CMusicInfoTag of the CFileItem has a valid path using CMusicInfoTag::GetURL() and only fall back to CFileItem::GetPath() if it's empty?

@mkortstiege
Copy link
Member Author

It's not blindly using it. CMusicInfoTagLoaderFactory is not about the real tag loading only. As for musicdb:// items it's using the CMusicInfoTagLoaderDatabase. In this specific case we need to grab the lyrics from the actual tag since we're not storing them in the library.

@Montellese
Copy link
Member

Ah right I didn't know about CMusicInfoTagLoaderDatabase and thought it was simply about getting the proper tag loader based on the file extension.

unique_ptr<IMusicInfoTagLoader> pLoader (CMusicInfoTagLoaderFactory::CreateLoader(*pItem));
// we load up the actual tag for this file in order to
// fetch the lyrics and add it to the current music info tag
CFileItemPtr item(new CFileItem(path, false));

This comment was marked as spam.

This comment was marked as spam.

@mkortstiege
Copy link
Member Author

Updated.

@Montellese
Copy link
Member

Looks good.

mkortstiege added a commit that referenced this pull request Aug 26, 2015
[music] fix missing lyrics from tag when playing from musicdb url
@mkortstiege mkortstiege merged commit ea4cda2 into xbmc:master Aug 26, 2015
@mkortstiege mkortstiege deleted the fix-musicdb-lyrics branch August 30, 2015 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport: Done Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants