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

[CONTAINERART] Fix Container.Art property for artist directories #7877

Merged
merged 1 commit into from Aug 28, 2015

Conversation

pbureau
Copy link
Contributor

@pbureau pbureau commented Aug 23, 2015

As detailed in this thread:
http://forum.kodi.tv/showthread.php?tid=235413&pid=2087854#pid2087854

I believe there are some issues with the current Container.Art(artist.fanart) property implementation.

The proposed patch adds a new method called in CGUIWindowMusicBase::GetDirectory() to make the property functional for artists directories and fixes the GetAlbumId() return value test

@mkortstiege
Copy link
Member

Shouldn't GetArtForItem(params.GetArtistId(), MediaTypeArtist, artistArt) work?

@pbureau
Copy link
Contributor Author

pbureau commented Aug 23, 2015

You are right, it should definitely work.

@pbureau
Copy link
Contributor Author

pbureau commented Aug 23, 2015

ok I pushed a new commit with the suggested change.

@ronie
Copy link
Member

ronie commented Aug 23, 2015

could you add some before/after screenshots of wwhat this is supposed to fix?

tbh, i don't quite understand your problem report on the forum.

@pbureau
Copy link
Contributor Author

pbureau commented Aug 23, 2015

Well I am not sure a screen shot will help here. The issue I have is that $INFO[Container.Art(artist.fanart)] will not return anything when looking at an artist path such as musicdb://artists/5

To me, it seems logical this property should return art for the current artist when looking at the artist page. This is the behavior I was expecting.

@mkortstiege
Copy link
Member

I've not runtime tested this yet, but the actual changes are looking good to me. Could you please squash the commits to one?

@pbureau
Copy link
Contributor Author

pbureau commented Aug 23, 2015

ok done.

@ronie
Copy link
Member

ronie commented Aug 23, 2015

Well I am not sure a screen shot will help here. The issue I have is that $INFO[Container.Art(artist.fanart)] will not return anything when looking at an artist path such as musicdb://artists/5

well, sometimes pictures say more than a thousand words :-)

$INFO[Container.Art(artist.fanart)] is only supposed to work at album and song level,
as it returns the fanart image for the parent container.

on artist level, there's no parent container. well, the root listing of the music library maybe..
you should use $INFO[ListItem.Art(fanart)] there.

@pbureau
Copy link
Contributor Author

pbureau commented Aug 24, 2015

The problem with $INFO[ListItem.Art(fanart)] is that it returns the fanart for the currently selected item, and:

  1. Sometimes there is no selected item, then the property returns nothing
  2. When an artist view contains albums from different artists (think about featuring), the result changes depending on the item selected

Which means there is no way to consistently display fanart for the current artist in the artist view, which is a shame, and is solved by the proposed patch :-)

@Hitcher
Copy link
Contributor

Hitcher commented Aug 24, 2015

Use a variable to fallback to Container.Art(artist.fanart) when ListItem.Art(fanart) is empty.

@ronie
Copy link
Member

ronie commented Aug 24, 2015

i'd like to request those screenshots again, as i still don't get whether your problems are at artist level (the main list of all artists in your library) or at album level (the list of all albums by a certain artist).

if this PR fixes stuff at album level, then that's accepted for sure.

@pbureau
Copy link
Contributor Author

pbureau commented Aug 27, 2015

Sorry for the delay. Here is a screen shot:
screen1
In this view, the background is the fanart corresponding to the current artist (Jay-Z). In this particular situation $INFO[ListItem.Art(fanart)] would return nothing as the up item is selected, and it would return fanart for eminem if the 8 miles item is selected.

@ronie
Copy link
Member

ronie commented Aug 27, 2015

great, i remember that issue, it's been around for a long time.
http://trac.kodi.tv/ticket/14290

before we had the container.art() stuff, skinners could use the $INFO[Fanart.Image] infolabel for this purpose, but it broke at one point. that bug was probably carried over to the container.art() implementation.

@mkortstiege ok to merge?

@Hitcher
Copy link
Contributor

Hitcher commented Aug 27, 2015

Isn't this covered by using a variable as I posted above though?

@phil65
Copy link
Contributor

phil65 commented Aug 27, 2015

nope, that wouldnt solve it completely. A container property is correct for this case.

@ronie
Copy link
Member

ronie commented Aug 27, 2015

nope, the problem is that Container.Art(artist.fanart) returns empty when you focus the parentdir (..) item.

@Hitcher
Copy link
Contributor

Hitcher commented Aug 27, 2015

But that's the same for season and episode views, so can we get them fixed as well?

@ronie
Copy link
Member

ronie commented Aug 27, 2015

it works fine on my end there.
Container.Art(tvshow.fanart)

@Hitcher
Copy link
Contributor

Hitcher commented Aug 27, 2015

Of course it does, sorry.

@hudokkow
Copy link
Member

jenkins build this please

@mkortstiege
Copy link
Member

+1

@MartijnKaijser MartijnKaijser added this to the Jarvis 16.0-alpha2 milestone Aug 28, 2015
@ronie
Copy link
Member

ronie commented Aug 28, 2015

@pbureau thx!

ronie added a commit that referenced this pull request Aug 28, 2015
[CONTAINERART] Fix Container.Art property for artist directories
@ronie ronie merged commit bfd7bd4 into xbmc:master Aug 28, 2015
@phil65
Copy link
Contributor

phil65 commented Aug 28, 2015

I think the only Container.Art() missing now is for movie sets. @HitcherUK could you confirm this? Container.Art(fanart) didnt seem to work for me in that case.

@Hitcher
Copy link
Contributor

Hitcher commented Aug 28, 2015

Has it ever worked for sets?

@phil65
Copy link
Contributor

phil65 commented Aug 28, 2015

not yet afaik, but it should be added for consistency.

@mkortstiege
Copy link
Member

not yet, but it should be added for consistency.

#7914 takes care of that.

@pbureau
Copy link
Contributor Author

pbureau commented Aug 30, 2015

thx for the merge, it certainly feels like an accomplishment :)

By the way I noticed the second test should actually be an "else if"... I will correct this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants