Add boxsets ability

[the-black-eagle]

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• None of the above (please explain below)

Checklist:

• My code follows the Code Guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have updated the documentation accordingly
• I have read the Contributing document
• I have added tests to cover my change
• All new and existing tests passed

[DaveTBlake]

OK, decided to make some comments on this as it is. More to come I'm sure

[DaveTBlake]

Unnecessary change

[DaveTBlake]

My error system/library/music/boxsets.xml

[DaveTBlake]

All changes to CLibraryDirectory can be dropped, they are in PR 16626

[DaveTBlake]

Add a new music lib setting to enable(default)/disable navigation from multi-disc albums to discs rather than directly to songs, and use it here to allow albums > discs navigation to be optional. That way we can give users original behaviour if they want it.

A search for musiclibrary.showcompilationartists may help you find example of how to do a new setting

[the-black-eagle]

OK, I'll give it a go.

[the-black-eagle]

Done that. Default is false and navigation is as it was. Toggling it on enables disc type navigation. Incidentally, extraart still works in original navigation if you have set any up, so the covers change as you scroll up and down the tracks between the discs.

[DaveTBlake]

Users won't see the new grouping as discs feature that way around. Make default having a "discs" level, and they can change the setting to get old behaviour back

[the-black-eagle]

Done. Not 100% sure that it should be turned off for boxsets at all, given that's where we started with all this, but as I can't see a particularly clean way of doing that I'll go with it.

[DaveTBlake]

Humm, yes when viewing boxed sets you do always want album > disc...
If I think of a way to do that nicely I'll let you know. At least on by default means users with boxed sets will see the immediate advantage.

[DaveTBlake]

CDirectoryNodeBoxsetDiscs needs renaming CDirectoryNodeDiscs throughout because it applies to navigation generally not just boxed stes

[DaveTBlake]

Cosmetic whitespace change

[the-black-eagle]

Grrr, thought I had fixed 'em all !!

[DaveTBlake]

GetBoxsetDiscs should be renamed GetDiscsByWhere as used for any multi-disc album not just boxed sets.
Also consider effect of other filter/options e.g. say showing just albums that have songs of genre = X (and nothing says all the songs on an album have the same genre), or have an artist filter and showing those albums with songs by an artists including guest appearences and on compilations. Navigation to discs should just show those discs with songs that staify the criteria. Look at how GetAlbumsByWhere uses GetFilter to do that for hints (or ask)

[the-black-eagle]

Do you mean, if an album has four discs and three discs contain tracks of genre 'pop' and one disc tracks of 'rock', you want to just display either the three discs or the one, depending upon the filter ?? And similar for an artist or any other filter criteria ?

[DaveTBlake]

Sorry for leaving you hanging, my limited Kodi time has been eaten by other things.
Yes, to make the navigation similar to other nodes (but may need some consideration what behaviour is wanted). Artists is a better example: say you pick an artist from "artists" node, albums are listsed including an album that is a boxed set where the artist makes a guest appearance on 2 songs on different discs. When you click on that album what is displayed - discs or songs? You could say just a few song go directly to them, but we don't skip the album level when the artist only has one album so should we skip discs?

[the-black-eagle]

We all have lives and limited time, so no worries on that score. I won't have much time myself over the next three days or so....
Anyway, I have thought about this quite a lot. I'm not convinced myself that it's actually needed, however that's probably for a different day. In the meantime, from an implementation point of view, GetFilter() currently only understands a type of 'albums' 'artists' or 'songs'. I have contemplated adding a 'discs' type (which might come in useful further along the line ??) but haven't done so yet because basically we are actually filtering the songs that are on a particular disc. Discs themselves don't have artists or genres or roles and there is no structure to link them to such things.
With that in mind, I have been playing with making GetFilter always use a type of 'songs' if 'disc' is present as an option in the url. This makes it include 'discid' and 'disctitle' (both of which are song options anyway) and then apply any other filter rules (for songs) in the same way they get applied for other routines that call it.

If I don't do it like that, GetFilter sees a type of 'albums' but trying to filter discs by album doesn't make sense as you have to have an album first before you can get any discs for it.
I'm still not convinced that I want to be able to return say 15 individual discs that have an artist of 'blah' and a genre of 'foo'. If I get back discs with no actual titles (the most common), I could end up with a list of 'disc 1, disc 1, disc 3, disc 2, disc 1, disc 1, etc' .

[DaveTBlake]

OK, I'll see if I can give more clarity on what I had in mind here too. But that will be later over the weekend :)

[DaveTBlake]

Unused function

[DaveTBlake]

CGUIWindowMusicNav::GetQuickpathName is deprecated function so can remove this change - see PR16626

[DaveTBlake]

Again this area is changed is in PR16626, so no need here. Watch out when rebasing!

[DaveTBlake]

Darn, 3 more commits while I was posting. Oh well I'm sure we will manage to unravel it

[DaveTBlake]

OK, looks like most of my previous commenst still hold, so here are a few more

[DaveTBlake]

Not just boxed sets - "subtitle of a disc."

[DaveTBlake]

Not just boxed sets - "subtitle of a disc."

[DaveTBlake]

What are you doing hard coding an art type???? Anyway "Thumb" is the cover.
Need to look at more but this doen't seem right

[the-black-eagle]

Yeah, perhaps my bad, but "thumb" is hard-coded. Is fanart1, fanart2 etc not hardcoded then also ?

[DaveTBlake]

Originally all art types were hard coded, v18 brought revolution that liberated art (meaning that skinners and addons could had a lot more freedom without needing future core changes) but had to leave some things for backwards compatibility. "Thumb" and "fanart" are still there, and there is more to do which will get rid of much of the separate treatment for "fanart". It is all historic.

You have done pretty much what I did (and probably every dev before us) attempted to copy what is already there. Unfortunately some bits need not to be copied, but nothing is documented so you have no way to know until someone with more experience says "not like that".

I need to take a bit more time over it and see if I can point you in the right direction. It just came as a surprize give all the things in my head, but really no so surprize because there is no reason for you to know otherwise.

EDIT
So not "your bad" in any way, but hopefully my replacement art commit will make sense of how I was seeing this (with my knowledge of "art mysteries"

[DaveTBlake]

Commented out to get it to compile (on Windows) - investiate when JSON implemented

[DaveTBlake]

Humm...... use of extraart approach (rather than per folder) is something to be considered generically for more than just box sets. I need to check deeper.

[DaveTBlake]

Wondering if all this manipulation of this tag value to get boxed set flag should happen when saving to the db (so in CMusicDatabase or CSong/CAlbum rather than here in the tag reading process.

[DaveTBlake]

OK, moving discussion generally to this PR so things don't get lost.

I don't think you have done the art right, although we may need some more work skin side to do art in the modern approach. But I have not had time yet to get into it and it would be best for me to comment in one go than drip feed you hints.

The use of disc title for grouping discs isn't as bad as I theorised it would be (so much for holding all the design in my head). That is a thaw in my view, but not quite a win for you yet!

Item "*all discs" does give you all the songs (yes, I know why not call it "*all songs", well historically it have always been "All [content name]" not "all [child thing you are going to when clicked]) which is consistent with the other uses, but seeing "discs " and knowing that isn't really discs in the list beneath niggles me a little.

We end up with paths like
musicdb://albums/1429/2/?albumid=1429&disctitle=SubtitleBoth
where the discnum in the first part of the url e.g. 2 is only the number of the first disc with the title given in the options e.g. "SubtitleBoth". We need to take care when that number used - getting the disc title, that is fine and (at the moment) for getting art which also OK, but need to check when doing other filtering etc. it doesn't bite us.

What about if the discs with matching titles are not adjacent in number e.g. disc 1 and 3 have the same title but disc 2 is different? Should grouping still apply?

[DaveTBlake]

Another aspect entirely: what about the album infomation dialog?
At the moment bringing this up from the list of discs/disc groups gives the album information. Is that good, or do you want a disc specific variant?
This info includes a list of songs (not all skins show it), should album info dialog give a list of discs instead? Should a new disc info dialog show the list of songs? Or are these lists on the dialog a total waste of time since the normal node navigation soon shows lists of songs and with more details?

[the-black-eagle]

What about if the discs with matching titles are not adjacent in number e.g. disc 1 and 3 have the same title but disc 2 is different? Should grouping still apply?

Good question and one I hadn't considered! If I say "no" here, then that messes up what I've done because it needs a lot more logic. On the other hand, my gut reaction was indeed "no". Tricky !! Is it likely though that that would occur ? Possibly, given the state of some users tags. Have to think about that a bit more.

Album info dialog. It shows what it should show I think. I don't know if you could show information about individual discs because I'm really not sure that there is any where to scrape that information from. Having checked 3 different sets on theaudiodb, last.fm and allmusic the only info available was for one set on last.fm which merely mentioned that it was a remaster of the original album (although there are 3 other discs with it). Of course there are always nfo files that could add that information if there was somewhere to display it. More info regarding a disc could be useful (remaster of original album, live version recorded at wherever, etc etc) but I don't think you need a list of discs because you get that when you open the album anyway. Besides, its cumbersome navigating that dialog with a remote. - - - - to see the tracks. Easier to press enter and have done with it. Album description is on the left in any case.

Art. I may well have done it wrong. I'd prefer that I didn't have a great long list of stuff to add in advancedsettings but being able to add extra art types that way seemed reasonable so I just picked a type to use.

[the-black-eagle]

Actually, having thought about it a little more, grouping won't apply if you try to group 1&3 together if disc 2 has a different title as it works on title == preceding title. All three should be split but 1&3 should still show the same title. I'll test that at some point tomorrow.

[DaveTBlake]

OK, here is my suggestion for handling disc art
Drop your art commit (rebase with skip), and just add the following bits of code to CMusicThumbLoader::FillLibraryArt instead

diff --git a/xbmc/music/MusicThumbLoader.cpp b/xbmc/music/MusicThumbLoader.cpp
index 909b614..a60f97b 100644
--- a/xbmc/music/MusicThumbLoader.cpp
+++ b/xbmc/music/MusicThumbLoader.cpp
@@ -270,6 +270,7 @@ bool CMusicThumbLoader::FillLibraryArt(CFileItem &item)
     std::string fanartfallback;
     bool bDiscSetThumbSet = false;
     std::map<std::string, std::string> artmap;
+    std::map<std::string, std::string> discartmap;
     for (auto artitem : art)
     {
       /* Add art to artmap, naming according to media type.
@@ -289,6 +290,21 @@ bool CMusicThumbLoader::FillLibraryArt(CFileItem &item)
         artname = artitem.prefix + "." + artitem.artType;
       }
 
+      // Pull out art for this specific disc e.g. "thumb2", skip art for other discs
+      if (tag.GetType() == MediaTypeAlbum && tag.GetDiscNumber() > 0 && tag.GetType() == artitem.mediaType)
+      {
+        // Find any trailing digits
+        size_t startnum = artname.find_last_not_of("0123456789");
+        std::string digits = artname.substr(startnum + 1);
+        int num = atoi(digits.c_str());
+        if (num > 0 && startnum < artname.size())
+        {
+          if (num == tag.GetDiscNumber())
+            discartmap.insert(std::make_pair(artname.substr(0, startnum+1), artitem.url));
+          continue;
+        }        
+      }
+
       artmap.insert(std::make_pair(artname, artitem.url));
 
       // Add fallback art for "thumb" and "fanart" art types only
@@ -319,6 +335,16 @@ bool CMusicThumbLoader::FillLibraryArt(CFileItem &item)
     if (!fanartfallback.empty())
       item.SetArtFallback("fanart", fanartfallback);
 
+    // Insert or replace album art with specifc disc art when we have some
+    for (auto discart : discartmap)
+    {
+      std::map<std::string, std::string>::iterator it = artmap.find(discart.first);
+      if (it != artmap.end())
+        it->second = discart.second;
+      else
+        artmap.insert(discart);
+    }
+
     item.AppendArt(artmap);
   }

I'll upload to my repo too if that is easier (I warned my Git use is a little rough'n'ready)

[DaveTBlake]

Disc specific cover art is already automatically fetched from folder.jpg held local to the disc music files and held as album art of type "thumb1", "thumb2", "thumb3" etc. not "poster".

[the-black-eagle]

Estuary is looking for a "poster" value in some of it's views. This is why art shows in 2 of the views (but not the disc subtitle, just the original album title), not at all in the widelist view (just text, shows the disc title as text) but not the last. If I set the 'poster' value, it does. At the time, this seemed like a better (easier ?) option than faffing around with the skin although that could well have been the wrong approach when I think about it now. Still, it worked !! I tried various different approaches to try to get the custom art into 'thumb' without much success. Similarly, there doesn't appear to be a way (not that I can see anyway) to alter or overwrite an album title so that it shows in all the different views. It's not a big thing, and I can navigate alright with the info thats there but it isn't 100% correct in some views and that niggles me. Guess it might need either another info label, some skin changes or a bit of both.

[DaveTBlake]

Estuary is simply wrong (it was done by people that never use music lib and didn't give an dam). They even opened a forum thread for user bug feedback during beta while privately in team saying they had no intention of looking at any of it. And got praised! I have never forgiven them for such attitudes, (most) users deserve better. [Friday night honesty!]

I understand your desire and happiness at getting something that worked, and I'm sure what you did had reasons given what you could see, but yes some of it needs to be done in the skin. Also Confluence is much easier to follow and mod if you want to mock up something for testing.

[the-black-eagle]

I can switch to using Confluence on my Dev system. My actual 'production' systems use a heavily customised version of Estuary done by a forum member. This version was never released publicly, but has some major modifications to the PVR section. As we can only watch TV via Kodi & tvheadend the pvr part needs to be user friendly and this members mod is awesome. I also rolled out a build of my branch to a libreelec install on one of my Pi's and to my main instance so that I can use the discs feature. Both of these are using this modded skin (which I further modded to work with my radio helper) so that's why I looked at Estuary. That and the fact that if something gives unexpected results in a skin, the first question is always "What happens if you use Estuary ?"

There are people that never use the music library !!?? Clearly they are missing out then. It does go some way though to explaining why most views are set up with video library variables and some don't even contain any references to the music library, despite the same view being used for both.

[DaveTBlake]

I always suggest users try a simpler skin like "Estuary or Confluence" ;)
You were right to test with Estuary, but wrong (understandably) to assume what it did was correct. Skinning isn't my thing so to test the art stuff I somethimes ask a team member to make me a skin hack, or I use debug a lot. If I can see that the art is there in the item artmap then that is good enough for me!

[DaveTBlake]

Need to handle any type of disc specific art not just discart. OK for now that is the only thing available, but avoidng hard coded art types in core.
But anyway by doing all the art handling in CMusicThumbLoader::FillLibraryArt can avoid adding discid parameter

[DaveTBlake]

OK, my art suggestion is in this repo https://github.com/DaveTBlake/xbmc/commits/AddBoxedsets4Art - dropped your art commit, added my own. Hopefully with my comments here you can see what this is about.

Kodi will already (with the correct as.xml settings) pick up local art of all types from the CD folders and that CMusicThumbLoader::FillLibraryArt change is all that is needed to get it to show on the discs view. No idea if scraping disc specific art is a possility in the future, or get an art addon to fetch it.

We can talk more about getting multi-disc art from a single mixed disc folder (for now you could do that manually).

[the-black-eagle]

Clever. I wonder if we could use strings.po xbmc#427 (which is 'disc') to localize that and pick up any art that starts with that string ? so 'disc.png' or 'discart.png' would work, regardless of any language used ?

[DaveTBlake]

I would not bring strings.po into art types, users are confused enough (and some change language settings during use, imagine what thay would do).

Art types need to be in settings, something configurable not hard coded, somewhere all addons and skins can access, common settings for all (music) scrapers and for automatic pick up of local art. At the moment that place is a whitelist in advancedsettings.xml, but that is not user friendly. It was just an initial step to get this functionality started in v18. For v19 I need to do more (you can help if you like).

I know users can set a variety of file names for thumbs, but once there is also a infinite list of possible art types having an infinite list of filenames for those types was all too much! Hence the simple map of filename to type. Add "thing" to as.xml, and locate thing.jpg appropriately (or use addon/scraper that fetches thing art), use a skin that shows "thing" art, and away you go. Core code changes never needed.

The hope is that between skins and art addons the authors will manage to standardise on one name for round (disc) images - "discart" (is my suggestion) - but core does not impose that, it is up to the external community to sort it out. Or they can divide into garthering a varity, core doesn't care.

The legacy is "thumb" and "fanart", which for backwards compatibilty will probably always have more options, although internaly fanart will be handled less specifically than it currently is.

Sorry, hasty description before bed. I probably missed something or assumed something.

[DaveTBlake]

Have updated my repo https://github.com/DaveTBlake/xbmc/tree/AddBoxedsets4Art with new art processing in Thumbloader specifically DaveTBlake@276920c replacing my previous attempt. See what you think, meanwhile I'm away a couple of days

[DaveTBlake]

Hard coded art type again. What is needed muct work regardless of what the disc art gets called by scraper, skin or user, and even support multiple types of disc specific art (OK I can only think of a CD image, but who knows what furture things we could have??)

Also the fallback mechanism is historic, this only expected to be applied to thumbs/fanart. Yes it looks tempting approach, but can be achieved more obviously I think

[DaveTBlake]

Take a look at my reworking, we possibly cross posted.

[the-black-eagle]

Yeah, I will. In regard to the hard-coding, I can see no other way with what there is at the moment to set a specific 'something%i', where %i may or not exist, into Player.Art(discart), which is what a skin is expecting. In the art table, it is of type 'Album' and AS.xml has artistextraart and albumextraart. If however, there was a type of 'Disc' and discextraart, then one could distinguish discart from anything else by the type. Otherwise, I don't see how to separate out discart from thumb or cover or any other string that could have a numerical suffix and is of type Album without having a hard-coded string in there. Thats before I've looked at what you've done though, so perhaps I'm talking without my book !!

I didn't think that you would go for this anyway :)

Just out of interest, is there any way of deleting art from the art table when something changes, without editing the db ? In testing I ended up with discart and discart1 for a couple of albums and found that discart was always displayed (as it is found first) and the only way to change that was edit the db and drop that row......or at least, I think it was the only way.

[the-black-eagle]

Ha, well looks like I was completely wrong then. Just tested with 4 different skins with albums with multiple and single discs and all displayed correctly. Now I need to go and pick your code apart to see how you've done it.

[DaveTBlake]

Yes, pick that code apart, see if you can make sense of it or find more design omissions or mistakes :)

Try to think of art generically. When I say disc art I mean art associated with subpart of a release, an obvious example is the round CD image which was "cdart" for some addons/skins but we are suggesting standardising on "discart". However parts can also have inner sleave covers (taken from folder.jpg and held as "thumb") and could have other stuff in the future - wild example could be sleave back. Core is stuck with thumb (a defualt image of some kind), but does not want to hard code anything more.

Current skins may look for "discart", and scraper may fetch "discart" and local files called discart.png will be picked up as "discart" if the right seeing is added to as.xml. The same core processing needs to work for "foobar" if that is what users/skins/scraper choose to implement as an art type.

Further we are treating a disc like a subset of an album with all the same properties plus a number and subtitle, hence same goes for art. Any art types an album can have ("thumb" + whatever is configured in as.xml) a disc can have, and are held as if for the album with disc number appended e.g. "thumb2", "foobar3" etc. So if a type of album art has a numberic suffix then it is disc (album part) specific. What you reminded me was that the song needed its specific part of album art too.

is there any way of deleting art from the art table when something changes,

From album info dialog, select art, or rather deselect that type. Of course all the possible art types (based on what types you have for some albums) get shown even when that specific item does not have that art as a way to allow users to select manually an use art type consistently. It is all still rather raw as a UI.

[DaveTBlake]

Just a heads up that the next rebase against master there are a couple of merged PRs from me that will mean paying attention PR16626 and 16657. I have found it helps to know that the trip hazzard exists, sorry about that, soon make a Git Ninja of you :-)

[the-black-eagle]

Oooh, lovely !!! I updated my master branch yesterday and then spent an (un)pleasant hour or so squashing, fixing, dropping and picking to get a nice fresh clean PR for you (#3). Dropped the Estuary stuff along the way as I can do that later. It also includes some code to allow a user to turn off disc navigation but not for boxsets. I'm now looking at how smartplaylists work, but need to have a serious look at filtering and get that working at the same time.

[github-actions]

This pull request is now marked stale because it has been open over a year without activity. Remove the stale label or add a comment to reset the stale state.