Permalink
Browse files

get rid of what should be an unnecessary assigning of fanart

  • Loading branch information...
1 parent 3978ce6 commit dbf85d5dd420517fb9e20aa0d619e7d4f4624734 Jonathan Marshall committed Jan 26, 2012
Showing with 0 additions and 5 deletions.
  1. +0 −5 xbmc/video/windows/GUIWindowVideoNav.cpp
@@ -469,11 +469,6 @@ void CGUIWindowVideoNav::LoadVideoInfo(CFileItemList &items)
pItem->m_bIsFolder = match->m_bIsFolder;
}
}
- else
- {
- if (CFile::Exists(match->GetCachedFanart()))
- pItem->SetProperty("fanart_image", match->GetCachedFanart());
- }
}
else
{

16 comments on commit dbf85d5

Member

mkortstiege replied Jan 30, 2012

This is wrong or at least revealed another bug :) We fail to show fanart in files node now even though we previously exported it.

Member

jmarshallnz replied Jan 30, 2012

In what case? It shows just fine here.

Owner

Montellese replied Jan 30, 2012

The general problem is that in files view the fanart of a directory/item does not show up if it is named -fanart.jpg and every movie is in it's own folder. "fanart.jpg" etc work just fine but XBMC exports fanarts as -fanart.jpg independent of whether every movie is in it's own folder or not. So in the end XBMC does not show the fanarts it previously exported. But I just tested the problem after I reverted this commit and it didn't solve it (and looking at the changes in this commit) I didn't really expect anything else. The problem must be somewhere in CFileItem::GetLocalFanart() because https://github.com/xbmc/xbmc/blob/master/xbmc/FileItem.cpp#L2869 does not make any sense if strFile points to a directory.

Actually I just noticed that there are two problems: The first one is that -fanart.jpg is not considered as a valid fanart image in CFileItem::GetLocalFanart for directories. The second one is that for files -fanart.jpg is chosen as the correct fanart in CFileItem::GetLocalFanart but then isn't displayed in the GUI because the caching failes. The call to CThumbnailCache::GetFanart for certain items returns an empty string because in https://github.com/xbmc/xbmc/blob/master/xbmc/ThumbnailCache.cpp#L235 both CVideoInfoTag::m_strPath and CVideoInfoTag::m_strFileNameAndPath are empty. m_strPath in CFileItem has the correct value though.

Owner

Montellese replied Jan 30, 2012

http://pastebin.com/GdtHcvvS fixes the second issue but last time I changed something in CThumbnailCache::GetFanart it messed up quite a few things so I'm reluctant to change anything in there.

Member

jmarshallnz replied Jan 30, 2012

Ok, first the CThumbnailCache::GetFanart() - my guess is this happens to movies all in the same folder that hasn't been scanned, and is because the item has it's watched state set, which creates the videoinfotag, but the videoinfotag doesn't have the path information? We don't want to just remove the path.IsEmpty() line as that's handling things like categories in the library (for which obviously there is no fanart). We could, however, test whether the infotag is "empty" or not (this is true if title, path and file are all empty) and if so, don't do that first block.

If the user hasn't scanned and all they have is -fanart.jpg, then I think we've never shown anything for the folder. If they have scanned, then we should show the fanart regardless, as I believe it should be cached on the movie item's path, which should be available to us in the infotag object, thus the checks in CThumbnailCache::GetFanart() should succeed.

Thus, I think the only case that fanart won't show is unscanned movies_as_folders that contain -fanart.jpg OR unscanned movies_as_files that contain fanart and who's watched status is set (not sure it actually needs to be watched or not).

SideNote: IMO if movies are folders then the ONLY thing we should support is fanart.jpg. If movies are not in folders than -fanart.jpg is fine. Reason is that we want to be able to test quickly (rather than retrieving the directory as is done currently in GetLocalFanart) for fanart files, and having to fetch the directory so that we grep for -fanart.jpg is silly. Obviously this has a slight problem in that XBMC doesn't export stuff in this form.

Member

jmarshallnz replied Jan 30, 2012

I don't think that fix is going to fly, as I think that videodb://2/2/ type paths will now try and set an invalid fanart?

I think the fix is

if (item.IsVideoDb() || (item.HasVideoInfoTag() && !item.GetVideoInfoTag().IsEmpty()))
{
// carry on...
}

Owner

Montellese replied Jan 30, 2012

The test item I use to reproduce this problem is scanned and is part of the movie library. I just removed it and rescanned it to make sure it's scanned and the issue remains. In library view the fanart is displayed perfectly fine but in the files node it doesn't display neither for the directory nor the file itself.

With your !item.GetVideoInfoTag()->IsEmpty() suggestion the fanart is displayed correctly for the file but still not for the directory. I didn't remember that there is an IsEmpty() check in CVideoInfoTag but knowing that now I agree that this approach is better suited than mine.

Member

jmarshallnz replied Jan 30, 2012

Hmm, this seems odd to me. If the directory is scanned (and I assume scanned with "use foldernames for movie lookups" enabled) then the item itself should have metadata assigned, and thus the videoinfotag should have correct m_strFileNameAndPath available.

Does metadata display for the item in question?

Owner

Montellese replied Jan 30, 2012

Yes it was scanned with "Use foldername for lookup" enabled. For the directory there is metadata available (but no fanart unless it's named "fanart.jpg") but for the file itself there is no metadata at all (but that's also the case for all the other items in my test library).

Member

jmarshallnz replied Jan 30, 2012

Hmm, so you have metadata (and I presume if you stacked down it will play the item when clicking on the "folder") but you have no fanart? This seems strange, as CThumbnailCache::GetFanart() should use the movie path.

Owner

Montellese replied Jan 30, 2012

Yeah when I enable stacking everything looks perfectly fine (fanart, metadata etc). What I did was I used the clean version of the test FS you uploaded (and got from pike) and renamed the "fanart.jpg" in the "Serenity" directory to "Serenity-fanart.jpg". After that I removed my MyVideosXX.db database and deleted the Thumbnails directory. Then I rescanned all the movies into the library.

Member

mkortstiege replied Jan 30, 2012

The test fs contains single media files or just opticals? Only tested with simple files per folder.

Owner

Montellese replied Jan 31, 2012

It contains both optical and single media files but I only tested with single media files (per folder) as well.

Owner

Montellese replied Jan 31, 2012

OK I just checked and in Dharma 10.1 the fanart doesn't show for the directory either so that's no (new) bug and should probably be that way. But in Dharma the fanart shows fine for the single media file (which currently doesn't work but would be fixed with jmarshallnz's comment).

Owner

Montellese replied Jan 31, 2012

After another set of test runs I can confirm that reverting this commit makes the fanart appear for the directory (which is how it worked in Eden B2). Once again this doesn't fix the fanart not being displayed for the file itself which would need an extra fix in form of !item.GetVideoInfoTag()->IsEmpty() in CThumbnailCache::GetFanart().

Member

jmarshallnz replied Feb 1, 2012

Ok, the problem is that the item is a folder, so we lookup using CVideoInfoTag::m_strPath. This is plainly wrong, as the item represents a file. The fix is changing the item.m_bIsFolder check to item.GetVideoInfoTag().m_strFileNameAndPath.IsEmpty().

Need to check for potential issues with shows + seasons.

EDIT: None that I can see - will test and do a pull req

Please sign in to comment.