[DaveBlake]

Ryan I will continue our discussion, I just have a quick skin related question

Anyone expecting Container.Art to work for Song/Album/Artist Info dialog?
I convinced myself I had it working for Song Info Dalog, only to discover that actually it is just picking up the art from the underlying song on the library window. Happens to be the same, so hard to spot an issue.

I know that skins use Container.Content on Song/Album/Artist Info dialog. That also works by picking up what is on the library window despite what looks like attempts in the code to get it from the dialog. It would create odd effects when bringing up the album info from that of a song (as lib window is songs). Anyway I have got that working from the dialog item now.

If Container.Art is wanted too for those dialogs I will need to make deeper changes to GUIInfoManager, to stop it always looking at the music library window. The art is in ListItem.Art on these dialogs, so is the duplication needed?

Then again ListItem.DBTYPE could be used instead of Container.Content, so it all seems rather confusing as an interface.
______
EDIT: it seems that https://forum.kodi.tv/showthread.php?tid=299107 has the answer
Skins should be using ListItem.DBTYPE

[DaveBlake]

Ryan, I think I now have a better undertanding of what you are saying. Thanks for sticking with it.

When manually choosing art for an item you want to be able to refresh the list of available remote art for it, without changing anything else about the item info or art.

That seems a reasonable idea

It seems to me that the list available remote art could be unchanging (my experience of music), or more frequently updated (your, and respected others, experience of video), and with cloud sourced material there really is no way to know in advance. But the user is the one that knows if they want to try to refresh it e.g. because they just added new art to the cloud, or they hate the art listed, or it is totally blank.

If it is the user initiating this intelligently (?) and manually, then my concerns about some automated refresh process hammering the resource sites (until they lock Kodi out completely) all go away.

As developers we have completely different ideas about how to approach implementing this idea. Your unpleasent experiences with submitting core changes, and knowledge of Python, encourages you think that Python cache is the obvious route. My knowledge of core/db and the Kodi philosophy of having core control of certain things, and lack of Python, I see it as a core issue. There are probably solutions both routes, I guess practically it comes down to who has the time to do the work and get it approved.

I can't decide whether to hijack this thread further with a discussion of how, or to start a new thread. It does come under the thread title, but this is also a long thread. I guess I can always split later, so lets talk.

Implementation of a manual refresh of available art only
I think it right that the initial music scraping for additional artist and album info and remote art, fetches and keeps a list of available art urls in addition to that taken as the default images. Keeping a list somewhere, rather then requering every time the user wants to manage it, is both server efficient and means the user can make some art choices/changes without being online.

Currently that somewhere is the music DB. It is item (artist or album) specific data, keeping it in the DB is a no brainer for me, even if it gets refreshed tomorrow (db has dynamic data i.e. playcounts, last watched, even rating). And API provided shelf-life/request limit could easily be held too. It would take fairly small changes to be able to rerun the scraper (even an XML one) just for available art for an item and not update anything else.

It would be a big change to have the music library GUI e.g. artist / album info dialog, access addon cache for some data while getting the rest from the DB. Also what about client/server installations, common addon cache across platforms?
 

I see this data as cache and placing it in the database adds an unnecessary bit of complication to Kodi...

I wanted to simplify Kodi a bit by moving this to scrapers, which do need to be responsible for their caching behavior anyway (sources other than HTTP will have their own rules for caching, and Kodi can't know about them all).

Having the data off with the scraper does not seem a simplifcation to me at all.
In effect the GUI would ask the scraper for available art every time, but the scraper decides if it gets it locally out of Python cache or if it sends remote requests to fetch it.

I remain unconvinced. A db based solution that everything can access sounds best to me.

 

I do what I can, though I'm iffy on core changes. However, I am a poor communicator (you got pretty much the opposite of what I was trying to say), and enough of the gatekeepers of Kodi have made it clear that they don't respect my contributions unless I communicate in a specific way, so contributing to Kodi core is not a very pleasant experience for me. 

Sorry about that, every one loses

[rmrector]

One last thought on why I don't like available art in the database: it's the only information for a media item in the database that is just alternatives to assigned data, it is not directly assigned and usable outside of picking an alternative. Different than other dynamic data because those are both usable and frequently used.

We're on the same page for user experience, though, so we're golden either way. I don't really want to suggest any changes beyond the final bits of what you are working on now*, but thanks for the discussion. I have some new ideas to ponder.

* This followup to the currently testing changes may have gotten lost a few posts back: Took a look at your last build and it looks good, except songs in the current playlist aren't being updated when artwork for their (album)artist or album is changed. Changing the song artwork directly is reflected in the playlist.

[DaveBlake]

I don't really want to suggest any changes beyond the final bits of what you are working on now*, but thanks for the discussion. I have some new ideas to ponder.

I am happy to discuss ideas Ryan, especially if you can back that up with testing and maybe even some code. I do need to do some non-art releated work sometime soon for my own sanity, but that doesn't mean I won't come back to it if there are well formed things to implement.
 

This followup to the currently testing changes may have gotten lost a few posts back: Took a look at your last build and it looks good, except songs in the current playlist aren't being updated when artwork for their (album)artist or album is changed. Changing the song artwork directly is reflected in the playlist.

I did catch your feedback, thanks.  That was an early version, I have the current playing song updating with album and artist changes now. I'll get another build up soon, but I need to do some rebasing and track down the addon issue that ronie reported.

[DaveBlake]

Well progress on this has been delayed since a message processing chnage in the song info dialog PR, has brought a separate issue to light. It was an accident waiting to happen, but looks on the surface that my PR is causing crashes , so I have been lost trying to debug a crash I couldn't repeat.

[docwra]

Well progress on this has been delayed since a message processing chnage in the song info dialog PR, has brought a separate issue to light. It was an accident waiting to happen, but looks on the surface that my PR is causing crashes , so I have been lost trying to debug a crash I couldn't repeat.

 Looked like a fundamental threading issue to me, so you should probably be congratulated with finding it

[DaveBlake]

Finally got that PR raised, and a new test build available Test build PR13672 (for windows, I can set builds of other platforms if required) that has an improved album/artist info dialog.

- This supports the manual setting of any type of artwork (via choose art button) for an artist or album.
- On artist info dialog the discography has been enhanced to always include the albums in the library in addition to any scraped list.
- All data access happens in a separate thread from UI, so cancel during refresh works etc.
- Art changes to album/artist of currently playing song get passed to the player GUI

Test feedback welcome (but keep in mind I already have a long list of other features to add when it comes to asking for anything new).

[docwra]

Thanks! Just tested and adding new landscape and clearart worked as expected. Still feels very wierd to click cancel though.
My first reaction after completing and setting the artwork was to hit "Add Art Type" as its where I thought OK should be.

Nice work anyway, I can now add custom music artwork to any artist

[docwra]

Also appears to be very reliable when the new artwork is changed to refresh the screen. In the past this didn't always happen.

So another fantastic fix!

[Karellen]

I am playing a bit of catchup here @DaveBlake. I just downloaded your Test build PR13672.

It loaded all the usual folder and fanart artwork. But none of the extended artwork like Banners, Logos etc were loaded from local files. Are we still using add-ons like Artwork Beef to load those extended artwork types?

Reading docwra's post above, sounds like we are still setting manually. Just double checking.

[DaveBlake]

But none of the extended artwork like Banners, Logos etc were loaded from local files. Are we still using add-ons like Artwork Beef to load those extended artwork types?

Reading docwra's post above, sounds like we are still setting manually. Just double checking.

Correct. Getting the internal scraping process to automatically pick up other art types is yet another phase, as is setting first scraper results of other art as a default. This gets Kodi as far as addons can do it, and user can do it manually, and that manual selection will offer extened art if the scraper has fetched URLs of art of that type.

I know it is slow, but it does move forwards

[scott967]

I did a bunch off regression testing on the PR13672 build.  Everything so far seems to work as expected.  I also installed Artwork Beef into the build and used it to add some discart and clearlogo from local files into the db.  That seemed to go as expected (from reviewing the db art table afterwards).  The new arttypes showed up in the get artwork select dialog as expected.

From looking at the commits it seems like there is a change on the artist info container 50 for discography?  It wasn't obvious looking at some of my test cases if there was anything different there.

While reviewing things, I see that for music video artist library node, Container.Content is "actors" rather than "artists", if it makes a difference.  I noticed in Estuary I was not getting listitem.property(artist_description) for the MV artists but the artist info dialog had all the correct infolabels populated.  For music video album library node, Container.Content is null.

scott s.
.

[DaveBlake]

Thanks for testing Scott

From looking at the commits it seems like there is a change on the artist info container 50 for discography?  It wasn't obvious looking at some of my test cases if there was anything different there.

The discography change is to always include all the albums that are in the library, previously it only showed those albums that the scraper returned. Users may not notice this if all their albums are in Musicbrainz too, or the artist has less than 25 albums. With classical music in particular e.g. albums by Beethoven, previously you could end up with a long scraped discography and none of your library albums in the list. It also means that you get a minimal discography (of your library albums) even if the scraping a discography has been disabled.
 

While reviewing things, I see that for music video artist library node, Container.Content is "actors" rather than "artists", if it makes a difference.  I noticed in Estuary I was not getting listitem.property(artist_description) for the MV artists but the artist info dialog had all the correct infolabels populated.  For music video album library node, Container.Content is null.

No idea how much I will be able to tidy up the music video hack, it awaits a new dev to champion it.

[rmrector]

Great, works well.

I'm still seeing the old artwork on items in the playlist when changing artwork for the album or an artist, though. Using Estuary, I cue up all songs in an album, then navigate back to the artist and change their fanart, but the old fanart is still shown for the songs on the playlist window. Same result when I change album thumb. Same result when changing art with JSON-RPC, which also doesn't change "Player.Art(*)".

And all "Container.Art(*)" has disappeared when navigating into a list of albums from an artist, or songs from an album or artist. I think I noticed this with the previous build, but forgot to mention it.

[DaveBlake]

Thanks for testing Ryan

I'm still seeing the old artwork on items in the playlist when changing artwork for the album or an artist, though. Using Estuary, I cue up all songs in an album, then navigate back to the artist and change their fanart, but the old fanart is still shown for the songs on the playlist window. Same result when I change album thumb.

This is confounding me
It works.... then sometimes it doesn't, and I don't see why.[Edit: the navigation route seems to be making a difference?] [Edit2: ah ha! Got it]
I am going to suggest that I merge the PR even with the current playlist not always taking the art changes immediately, there are enough gains. But it is frustrating not to be able to see what causes the variation. Will fix and produce another test build.
 

Same result when changing art with JSON-RPC, which also doesn't change "Player.Art(*)

That makes sense, the JSON just changes the DB and notifies the active media window to refresh, but does not communicate with the player (hence player GUI).
 

And all "Container.Art(*)" has disappeared when navigating into a list of albums from an artist, or songs from an album or artist. I think I noticed this with the previous build, but forgot to mention it.

 Er... that may or may not have been me.