Add the ability for Kodi to properly navigate Box Sets

[the-black-eagle]

Description

Kodi currently treats a box set as an album and lists all songs on that album in track and disc order. This makes it difficult to navigate in sets that have many discs.

Motivation and Context

This change adds the ability for Kodi to split an 'album' into it's separate discs and for each of those discs to be navigated independently.

How Has This Been Tested?

Building and testing locally with multiple albums, including single disc albums, two and three disc sets, compilations and proper box sets. Also tested with mis-tagged albums

Ubuntu 18.04 64bit, ddd for debugging. Multiple music sources.

It does not affect any other areas.

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)
• [X ] 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:

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

[DaveTBlake]

Marvellous, a PR. :)
I'll have a look tomorrow, meanwhile if you could rebase without all the whitespace changes it would make me happy person.

[the-black-eagle]

I will try Dave. I've already added support for TSST to CMusicInfoTagLoaderFFmpeg::Load and am about to address the db update.

[the-black-eagle]

OK, so I'm stuck on fixing this whitespace issue! I've rebased again, and git tells me that everything is up to date on both branches but it doesn't seem to have changed anything.

[DaveTBlake]

Discsubtitle in the artist table??? That is a boo boo for sure.

[DaveTBlake]

OK force a rescan, otherwise no one will see anything.
Although one idea was during development to allow the rescanning to be optional when possible (even though to test the new features they need to rescan) and then bump this for say alpha and apply all the tag additions in one go.
There are going to be more tag additions (I have a list ready to do), so may decide to remove this for the Kodi PR depending on how near to PR I get with those additions.

[DaveTBlake]

OK, so I'm stuck on fixing this whitespace issue! I've rebased again, and git tells me that everything is up to date on both branches but it doesn't seem to have changed anything.

The whitespace is edited in your original commit, so you need to create that commit without those changes

[DaveTBlake]

I am wondering what happens with CTagLoaderTagLib::SetReleaseType if the DISCSUBTITLE tag value is read after RELEASETYPE? I don't think there is any defined order for metadata. You need to gather all the tag data, and only then can you use them in combination.

This could be done in CMusicInfoScanner::FileItemsToAlbums and/or creator of CAlbum (where artist credits are gathered from several tags). Note there are two distinct paths to how an album gets identified from tags depending on having mbid tags or not. This is historic and not ideal because of the inconsitencies it introduces (over say misssing ALBUMARTIST tags), and something I should look at one day. Anyway I'm just giving a pointer where to look.

Also no tag reading doesn't just handle different separators. See how they are done in CAlbum::SetArtistCredits using StringUtils::SplitMulti

[the-black-eagle]

I don't think it matters in this instance which order the tags come in. My code should handle it in either case. If the disc subtitle has not yet been read, strDiscSubtitle will be empty, so it will get set to 'dummy' because of the 'boxset' in the tag. If the disc subtitle is read at a later point, 'dummy' will be overwritten by the correct title. If it's already been read then 'dummy' will be set only if it's empty. Either path leads to a value in strDiscSubtitle. IF the actual disc title is empty and it is read later (which will overwrite 'dummy') it doesn't matter because the boxset flag is true and it will just get set back to dummy again.

[DaveTBlake]

This only reached when !album.bBoxedSet anyway, setting does nothing?

[the-black-eagle]

Yes it does, because bBoxedSet could have been forced to true by setting the tag, in which case, we don't want to set it to false if there are less than 3 discs. If you take it out, you cannot make a twin disc set into a boxset. Not sure why you would want to, but someone might. It also messes up making boxsets from albums where the discs are in separate directories.

[DaveTBlake]

This only reached when !album.bBoxedSet anyway, setting does nothing?

[the-black-eagle]

Yeah, setting the bool to false again is pointless, it should just be continue. I'll take line 943 out. The check is important though, because without it, Kodi will start to create a boxset for a compilation (and it will show in the first node) but there won't be any discs associated with it.

[DaveTBlake]

Fair enough regarding tag order processing I guess I just didn't expect it to be in the tag processing. I do need to look more closely overall, I'm just chipping a little bits at the moment (dsitracted with other things).

But I still need you to banish those cosmetic whitespace changes that make it look like you have involved areas of cocde that are nothing to do with this PR. Did the Estuary changes really come down to so little after all, or is another commit going to appear?

[DaveTBlake]

Needed?

[the-black-eagle]

Nope. And yeah, the Estuary changes were only small although the initial commit included some other files that I had edited whilst trying to get it to display what I wanted.

[DaveTBlake]

Oh dear @the-black-eagle , I have a day off and when I get back the diff is even less readable. I think the problem is that you don't have a static clone of master branch in your repo or you rebased that to the current master but didn't rebase this branch. Either way it needs sorting out please.

[the-black-eagle]

I rebased both my master branch and this one to the current upstream master. If I close this and open a new PR against my current master, will that work ?

[DaveTBlake]

Revert to original text

[DaveTBlake]

Revert to original text/spaces

[DaveTBlake]

While it seems obvious to put this here near "disc" the 3-digit message text ids are old and impossible to search for, hence no new messages are to be added in this range. Move to a higher ID

[DaveTBlake]

Are you sure xbmc#427 isn't used somewhere else where adding " number" is incorrect? Need to take great care with the old 3-digit ID messages

[DaveTBlake]

"Box-sets", "Box sets".... hyphen consistency?

[DaveTBlake]

Revert cosmetic whitespace change

[DaveTBlake]

Revert cosmetic whitespace change

[DaveTBlake]

Revert cosmetic whitespace changes

[DaveTBlake]

Revert cosmetic whitespace change

[DaveTBlake]

Revert cosmetic whitespace change....
You can see all of them. Fix it!!

[the-black-eagle]

All fixed......I think.

[DaveTBlake]

I know just starting to look at JSON changes, but some early feedback.
Will want related stuff JSON in a separate commit, and skin changes separately too but most of the other commits can be squashed. In fact it may help to have them squashed now, and certainly get the master they are compared against and the master you started from to match.

[DaveTBlake]

I know you are just beginning to look at this.
I don't think boxset should be a separate parameter like this, but part of the general filtering instead (working the way that smart playlists work).

[DaveTBlake]

New fields must be added at the end, in case any existing API consumers are using position of fields rather than name.

[DaveTBlake]

New fields at end

[DaveTBlake]

At end....

[DaveTBlake]

I rebased both my master branch and this one to the current upstream master. If I close this and open a new PR against my current master, will that work ?

That will just sever all the comments already made, so don't do that just yet.
If the masters have been updated then it looks to me like you have not rebased your ADD_BOXSETS_ABILITY branch against them.
Rebase your local master, push that to your repo
git checkout master
git fetch upstream
git rebase upstream/master
git push -f origin master

Then rebase your local ADD_BOXSETS_ABILITY against local master, do some squashing while you are doing that. Finaly force push that to your repo, and this PR will update magically

[DaveTBlake]

Sorry about the whitespace comment rampage, but it was begining to get to me!

[DaveTBlake]

m_disc not m_idAlbum?

[DaveTBlake]

Since everything else is "box set" I guess bBoxSet would be more consistent.
Then again the field in the db is boxedset.....

[DaveTBlake]

Good you have sorted out the master.diff problem. :)
Unfortuneately I'm unlikely to be able to really dig into this until the weekend (although you may get some passing comments if I get time). I want to really think about it, and I just haven't had chance.

Meanwhile MusicDatabase.cpp and MusicInfoScanner.cpp still have distracting whitespace changes. They will need to go before a real PR (otherwise having cosmetic changes dotted all over a PR that adds a feature makes diffs and subsequent Git blame impossible to use). Yes MusicDatabase.cpp is in need of a big cosmetic clean-up of whitespace and out-of-date style (not to mention Doxygen comment documenting), but it needs to be a PR just for that. Anyway they need to go eventually, and it would make my life a whole lot easier if they went now.

[the-black-eagle]

Sure. There is no rush Dave, and by the weekend I should have cleaned up the whitespace issues, fixed a couple of silly errors I made today when tidying things and squashed the commits for that into one or two. I'm well aware that a real PR will have to be spot-on to even have a chance of being accepted.

[DaveTBlake]

Restore missing insert

[the-black-eagle]

Multiple disc is indeed more common than boxed sets with volumes. However, I don't see why we can't have the best of both worlds. In spite of a little inflexibility in the UI engine, it's perfectly possible to do both things because of the 'rules' of a boxset.

We decided a boxset has to be three or more discs and that all those discs have to have subtitles in the tags. If an album doesn't have the subtitle tags but a user thinks it should be a set, they can set the boxset tag which automatically generates a title for each disc that is missing one. Therefore, there are titles for any disc in a boxset, as opposed to most other albums which might only have a title for one disc (bonus disc, live version, original version etc etc).
Taking that further, if an album has disc titles then it's likely (but not always necessarily true) that it's a boxset. Obviously if there are no titles, it definitely isn't a set and splitting on disc number is ideal. If however there are titles then we can split on those. This keeps volumes together but splits up everything else. The only time I can see where there might possibly be an issue is if the first disc has a subtitle but the others do not. However, upon thinking about it, it wouldn't be an issue because your code generates a 'disc %i' title for any disc without one.
Anyway, I had a bash at implementing it! Turned out to not be too difficult really. Keeps discs with the same title grouped together, splits up multi-disc albums into individual discs. My Tree full of Secrets is now correctly showing two discs per volume and 9 volumes + the last disc (Volume 9 is only one disc). 2, 3, 4 and 5 CD sets that aren't boxsets also split correctly and show the right amount of discs as do the 'real' boxsets. Currently I've only tested with a small subset of around 60 albums but I'll be expanding that later. The test albums have one or two with disc names in only some of the discs, a couple with the boxset tag set, two that are mis-tagged on purpose and a bunch of 2, 3, 4 & 5 CD sets as well as some single CD albums (some with disc titles and some without).

[DaveTBlake]

It is not fetching the data that concerns me, that is quite doable, it is what is the concept this creates, what is the node content? Not "discs", despite being based on DISCSUBTITLE, because splitting into discs gives a different result. Does the "volume" then get split into actual discs? Calling it a volume just because the example you gave happend to use that in the DISCSUBTITLE tag isn't reasoned design either (just useful shothand for this discusson). It is arbitrary parts that might have meaning for a specific boxed set, or could be a tagging error. Even MB does n't list 9 volumes, it breaks the release like all the other releases into 18 discs. Adding an extra level of discs (based on disc number) to albums > songs is fine, taking DISCSUBTITLE as a well defined and well used tag that Kodi can base a node upon is not. Adding more than one new level to navigation with the intermediate level not having a generic definition and only aviallable is special circumstances is just not good.

Imagine explaining it to certain users. Nagivation genres > artists >albums > parts/volumes (sometimes) > discs (if more than one and option enabled) >songs

[the-black-eagle]

No, the 'volume' does not get split further. If you open a volume you get the contents of both discs in that volume. From a user point of view, that's what I'd expect to happen. So it goes albums > discs/volumes > tracks. I can happily agree that it's a niche area, but do users really want to look at all 18 discs at once or 9 volumes ? For me, the latter is the better option and I've got no problem with opening a volume and seeing the contents of two discs inside, because that's whats meant to be in a volume. In all other cases, the albums are split into individual discs. That includes all other albums that have more than 1 disc.
I guess my reasons for wanting to do it this way is that it's more aesthetically pleasing in the GUI whereas your reasons against doing it are more technical. There is no way I'd want to split up a very large set, say 30+ CD's into individual discs and navigate through those if I don't have to. Even if it meant writing my own DISCSUBTITLE tags to sub-divide it into 2 or 3 discs at a time, I'd do that if I knew that Kodi could replicate that division on screen. It's not another level, it's a way of telling Kodi that these x amount of discs go together in the same way that an album title says these tracks go together. The navigation is still album > discs > tracks except that 'discs' has the possibility of containing the contents of more than one actual disc where grouping them together makes sense, either because MB have tagged them like that, or the user has. In all the testing that I have done, this doesn't affect anything else apart from grouping together discs with the same title, which is presumably why they are tagged with the same title in the first place. Which is better to see on screen, Vol 1, Vol 1, Vol 2, Vol 2, Vol 3 etc or Vol 1, Vol 2, Vol 3 ? If you were coming to Kodi for the first time, would you expect to see those duplicates, or expect it to be able to group them together ?

[DaveTBlake]

I guess my reasons for wanting to do it this way is that it's more aesthetically pleasing in the GUI whereas your reasons against doing it are more technical

Yes, that is exactly right. I do understand why as a user you want what I see as a fuzzy split into discs/volumes, but OTOH it is so niche conceptually. Setting out knowing the sometimes "discs" are not actually going to be discs just seems icky. "Disc groups"? No, not seriously. Also what about art for volumes, grouped discs, whatever they are? Having folders per disc, using disc number are nice generic ideas, you can't use title in the same way.

But I don't want to piss you off with a "no, no, no" attitude, so show me. I was half expecting an example of what you want to do to appear as a commit on #2. Perhaps do that.

You also need to be aware that looking at CDirectoryNodes has resulted in https://github.com/DaveTBlake/xbmc/tree/NodeFolderLabelAll and a PR to Kodi which I will merge sooner or later but hope to get a review if I can find an interested team member. It could be worth pulling that in and rebasing on top of it.

[DaveTBlake]

An approach, that I don't think you will like but is more generic, would be to spot where the same non-null DISCSUBTITLE value has been set for more than one disc in the album (boxed set or not), and to append the disc number. So for that dreaded example you would see "Vol1 (Disc 1)", "Vol1 (Disc 2)", "Vol2 (Disc 3)", "Vol2 (Disc 4)" etc. , better than repeated titles on screen.

That would make me happy technically, the node type/level would always be "discs", not sometimes some other parts. Also each volume of "A tree full of secrets" is pretty huge, surely it is more useful split into discs, and when you want to play a volume you add both separately?

[the-black-eagle]

Backwards again !!!

Yeah it is pretty big, but 2 discs of tracks is a useable, manageable chunk. There is a Rachmaninoff set on MB with 32 discs. I'd love to be able to split that into 11 sets rather than 32 individual discs - it has no titles anyway so left like that it would show as 32 discs.

Vol 1(disc 1) etc is better than just Vol 1, Vol 1.

Art. Kodi shows the right cover for each volume because each disc has to have a cover otherwise one disc will show blank art when you play it. So I still need to assign 18 covers although Kodi finds these and does it automatically. If not, when scrolling down the list of tracks, when you get to the second disc, the art would disappear......I just know you're probably not going to like that although it's no different to assigning the art if you split it into individual discs.

Oh, I have a commit ready to go. A couple actually. One that adds the art stuff, one that does this and one that fixes a bug that I found today where the song table was not being updated correctly for an album that already existed in a different path.

Don't worry about pissing me off, it's extremely unlikely to happen no matter which way things eventually go. Discussion is always good, as are other peoples points of view. 'Fuzzy' it might be, but fuzzy logic gives scope for flexibility where hard and fast rules do not. It's probably not going to be seen by that many people, but the ones that do see it will just think Kodi is smart. If it doesn't go in, how long before someone shows up on the forum saying that they have discs with the same titles, why doesn't kodi group them ? In fact, maybe some more user feedback about this would be helpful. What do actual users want ? The ones that want to be able to see proper sets, how do they want them split up ?

[DaveTBlake]

Backwards again !!!

Which bit, all of it?

In fact, maybe some more user feedback about this would be helpful. What do actual users want?

That is always tricky. The most vocal on the forum are not necessarily the most representative in opinion or use, but where else can you go. Then you have us team members, and we are always sure we are right even with features we don't use ;) And of course the answers you get always depend on the way you ask the question.

Can we hammer away on this a bit more before raising users hopes/expectations with a forum posting. I want to look at what you have done, see if I can pick serious holes first.

Art: you may have mixed up what is done by core and what is at the discretion of the skin, yet to check more carefully.

Great you are robust, I may be criticising your design/implementation but I am not attacking you. I am delighted that you have taken up this challenge.

[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.