[black_eagle]

Thanks for the positive feedback !!

As regards MP3tag, I have no idea as I don't use Windows.  Were you tagging an actual mp3 file or something different ?  Although the tag name in Picard is 'discsubtitle' the actual frame it sets in the id3v2 tag is 'TSST'.  This is an id3v2.4 tag so any tagging software needs to be using that version for it to work.
Anyway, glad you sorted it

[Efjeejee]

Thanks for the positive feedback !!

As regards MP3tag, I have no idea as I don't use Windows.  Were you tagging an actual mp3 file or something different ?  Although the tag name in Picard is 'discsubtitle' the actual frame it sets in the id3v2 tag is 'TSST'.  This is an id3v2.4 tag so any tagging software needs to be using that version for it to work.
Anyway, glad you sorted it

Only mp3-files here. Tags in format id3v2.4 . Later today I finally learned that MP3tag uses the tagname : 'SETSUBTITLE'. So from now on I can hopefully edit 'discsubtitle'/'TSST' in my favourite MP3tag.
BTW I used the KODI Estuary skin, but it worked as well in Aeon Tajo. Not so good in the Titan-Vanilla skin, I suppose due to skin-problems with v.19
Keep on going the good work!

[scott967]

OK, didn't have any boxsets to test so I created one by adding DISCSUBTITLE tag on flac files organized by album\disc\song files.  My test set has 11 discs, each is named.

scanned into the library. 

1.  bBoxedSet is 0 while iDiscTotal is 11
I thought this was supposed to be forced to bBoxedSet 1 when there are 3 or more DISCSUBTITLEs?

2.  For discs in boxed set, ListItem.DBtype is Album (setting split albums into disks is ON).  Not sure this is a good idea?

3.  When displaying discs for the album, ListItem.DiscTitle is null, so the disctitle currently doesn't show up at all on my musicinformation dialog.  Actually, the musicinformation dialog on a disc is the parent album.   It seems to me maybe useful to have a DBType of disc and a discinformation dialog.  The musicinformation dialog for the disc tracklist container(50) shows all tracks of the album,. not just the tracks for the disc.

4.  When viewing music window songs for a disc, TotalDisks is 0, but when you open songinformation dialog TotalDisks is set correctly.  In the musicplaylist window, TotalDisks is also set correctly.
5.  For non-boxset single-disc albums music window song view also has TotalDisks 0 while songinformation has TotalDisks 1.  I haven't tried non-boxset multi disc albums.

I went back and added RELEASETYPE boxset tags on all the files and that did result in the bBoxedSet getting set to 1 which then allowed the Boxset node to appear.  Other than that I don't think bBoxedSet=1 does anything?

scott s.
.

[black_eagle]

1.  bBoxedSet is 0 while iDiscTotal is 11
I thought this was supposed to be forced to bBoxedSet 1 when there are 3 or more DISCSUBTITLEs?

Yes, it is supposed to be.  As my test data also includes FLAC files in an artist/album/disc/song format and mine scan in perfectly well, get marked as a box set and have perfectly readable disc titles, I would have to ask how you added that tag and whether your FLAC tags are in Ogg format or not ?  What did you tag them with ?

2.  For discs in boxed set, ListItem.DBtype is Album (setting split albums into disks is ON).  Not sure this is a good idea?

Why not ?  It's not like discs need any special handling by a skin as it's all done in core.

3.  When displaying discs for the album, ListItem.DiscTitle is null, so the disctitle currently doesn't show up at all on my musicinformation dialog.  Actually, the musicinformation dialog on a disc is the parent album.   It seems to me maybe useful to have a DBType of disc and a discinformation dialog.  The musicinformation dialog for the disc tracklist container(50) shows all tracks of the album,. not just the tracks for the disc.

Even if there are no disc titles, if you have navigate by disc turned ON, core generates a fake disc title for each disc.  Although a disc information dialog might be useful, I'm not totally convinced.  You can see the tracks on a particular disc already and currently there is no additional information to display in a disc information dialog.  I'm not sure what other info could be displayed there to make it useful ?

4.  When viewing music window songs for a disc, TotalDisks is 0, but when you open songinformation dialog TotalDisks is set correctly.  In the musicplaylist window, TotalDisks is also set correctly.
5.  For non-boxset single-disc albums music window song view also has TotalDisks 0 while songinformation has TotalDisks 1.  I haven't tried non-boxset multi disc albums.

Yeah, this is a problem in core which @DaveBlake is already aware of.  However, I believe that it is non-trivial to fix.  The root cause is (I think) that some properties do not get set correctly, depending on the path through the musicdatabase.  So in some cases they are set, but in others they aren't.  When I noticed this myself, I added a workaround to my skin by setting a property that gets used later when the listitem is not available.

[scott967]

1.  bBoxedSet is 0 while iDiscTotal is 11
I thought this was supposed to be forced to bBoxedSet 1 when there are 3 or more DISCSUBTITLEs?

Yes, it is supposed to be.  As my test data also includes FLAC files in an artist/album/disc/song format and mine scan in perfectly well, get marked as a box set and have perfectly readable disc titles, I would have to ask how you added that tag and whether your FLAC tags are in Ogg format or not ?  What did you tag them with ?

They are Xiph/Vorbis Comment tags from mp3tag. Looking at the database, each song has correct strDiscSubtitle assigned, so the tag is being scanned properly.

2.  For discs in boxed set, ListItem.DBtype is Album (setting split albums into disks is ON).  Not sure this is a good idea?

Why not ?  It's not like discs need any special handling by a skin as it's all done in core, .

It's so a skinner can more easily create a custom view for the discs. It would be easier if I could test String.IsEqual(Container.Content,discs) or String.IsEqual(ListItem.DBType(disc). Currently I think I can do it with [String.IsEqual(Container.Content,albums) + !String.IsEqual(Container.FolderName(albums)] but I still have to test if focus is on the .. or ALL DISCS listitems in the container.

3.  When displaying discs for the album, ListItem.DiscTitle is null, so the disctitle currently doesn't show up at all on my musicinformation dialog.  Actually, the musicinformation dialog on a disc is the parent album.   It seems to me maybe useful to have a DBType of disc and a discinformation dialog.  The musicinformation dialog for the disc tracklist container(50) shows all tracks of the album,. not just the tracks for the disc.

Even if there are no disc titles, if you have navigate by disc turned ON, core generates a fake disc title for each disc.  Although a disc information dialog might be useful, I'm not totally convinced.  You can see the tracks on a particular disc already and currently there is no additional information to display in a disc information dialog.  I'm not sure what other info could be displayed there to make it useful ?

Getting ListItem.DiscTitle for me is better in the info dialog rather than relying on ListItem.Label. And for Container(50), it seems more reasonable to display the tracks for the disc, not the tracks for all discs.

4.  When viewing music window songs for a disc, TotalDisks is 0, but when you open songinformation dialog TotalDisks is set correctly.  In the musicplaylist window, TotalDisks is also set correctly.
5.  For non-boxset single-disc albums music window song view also has TotalDisks 0 while songinformation has TotalDisks 1.  I haven't tried non-boxset multi disc albums.

Yeah, this is a problem in core which @DaveBlake is already aware of.  However, I believe that it is non-trivial to fix.  The root cause is (I think) that some properties do not get set correctly, depending on the path through the musicdatabase.  So in some cases they are set, but in others they aren't.  When I noticed this myself, I added a workaround to my skin by setting a property that gets used later when the listitem is not available. 

OK.

By adding the release type so I can test on IsBoxset it already is very nice feature.

scott s.
.

[DaveBlake]

@scott967 I think @black_eagle would like to look closer at your tagged music files of 11 discs with disc subtotiles that didn't create a boxed set. I'm not suggesting you send/link him all the music files (11 discs gish  no!), but wondering why it didn't automatically interpret it was a set. I leave you to figure out a good way to share this

The question about having a content = "discs" for skinning is something I wondered about. We can get away with treating it as an album rather than add complexity, all the album data is there, or willl skinners want to know they are looking at a disc. If they do then how best to handle it so other skins can continue to treat disc just like album?

Album info dialog..... I think I would like to see the take-up of this featire before we spread into container 50.

Getting all the album props visble on every song listitem... It is a general issue to be dealt with not just a boxed set/multi-disc one, so I pushed @black_eagle away from a hack just for disc total. I hope to get to it, I have some ideas on the scope of the issue just not enough hours in the day!

[black_eagle]

They are Xiph/Vorbis Comment tags from mp3tag. Looking at the database, each song has correct strDiscSubtitle assigned, so the tag is being scanned properly.

After some investigation and looking at my tags, it turns out I had added the boxset tag myself at some point to the sets that are in individual directories (CD1, CD2 etc) and then forgotten that I had done that !!  So my bad there.

Anyway, the reason for that was that the scanner can't count the disc titles across multiple directories. I have a fix for that, assuming it's OK with @DaveBlake so thanks @scott967 for making me look at that bit again. 

For ref, I have added this after line 921 in CMusicDatabase

cpp:
      strSQL = PrepareSQL(

          "SELECT COUNT(DISTINCT strDiscSubtitle) FROM song WHERE song.idAlbum = %i", idAlbum);

      int numTitles = strtol(GetSingleValue(strSQL).c_str(), nullptr, 10);

      if (!bBoxedSet) // not already a boxset via tag or CMusicInfoScanner

        if (!bCompilation) // and not a compilation album

          if (numTitles >= 2) // two distinct titles already so this must be 3 or more

          {

            strSQL = PrepareSQL("UPDATE album SET bBoxedSet=1 WHERE album.idAlbum=%i", idAlbum);

            m_pDS->exec(strSQL);

          }

That correctly adds boxsets that are split over multiple directories without affecting anything else.

I haven't pushed this to github yet though. Good job too, as I've found a little flaw!

[scott967]

Here is a link to a zip of the problem boxset.  It is a single boxset album with snips of the first 5 tracks of the first 3 discs:

https://www.dropbox.com/s/2l4d5rrb1o05ty...t.zip?dl=0

Haven't used dropbox in a while, it looks like you have to select "open" and it complains but gives an option for "download".

scott s.
.

[scott967]

It's correct that creating a new content type of disc would break existing skins that expect artists albums or songs.  Here's where I think I would change:



This is looking at the discs of boxset (album) title Der Ring des Nibelungen.  In this view there's no indication of the disc titles.  You can only tell by the cover art.  Also the title bar says "sort by name" but it's actually sorted by discnumber.

If I open the music info dialog for any of the discs I get this:



Again, no indication of the disc title (happens to be Götterdämmerung Act 1 in this example) and the tracks shown are from the first disk (you have to scroll right on the container to actually see the tracks on this disc).  I think the cover art shown isn't the best either (would prefer thumb<<discnumber>>).  If you click "play" you get play from disc 1 track 1.

scott s.
.

[black_eagle]

Yeah, Estuary sadly isn't the best skin to use (for music anyway) I have found.  I mean, I'm certainly no skinner, but I think it's because in two of the four views it shows ListItem.Title rather than ListItem.Label.  It's not possible in core (AFAIK) to alter the title, but it is the label (and label2) so that's what the code does.

Confluence does show the disc titles in all of it's views (without modding anything) and in the album info dialog (still shows all the tracks though and I don't know enough about skinning to know if that's fixable in the skin dialog or it needs to come from core )







Track info Dialog



Album info Dialog



The only mods I have made to that from stock are to add the overlay on the top right if an album is a boxset, add Disc X of Y and the disc title (if its not empty else disc number) in the track view and add Total Discs and Boxset to the info dialogs.

To be honest, the album dialog info is correct because it shows info from the whole album. Even if there was a disc info dialog, apart from displaying the tracks on that particular disc and the art, there is no other relevant information to display other than repeating what's already in the album dialog and you can see the tracks by hitting enter instead of i.

The view in Estuary that shows 'Sort by name' when viewing the discs is because the previous view was sorted like that.  Confluence only shows that info in the sideblade and there is no method of sorting the discs.  They are always in disc number order.  I did consider that but then decided I couldn't see a need to sort them into a different order.


The fix for multi-folder albums not being correctly detected appears to be working well.  The logic was correct, but I initially added it after an album was updated but before the songs were.  This meant that if you scanned an album with two titled discs twice (same tags but different file locations) it would get added as a boxset.  The check is now done after any songs have been updated so that doesn't occur.

Got some non-kodi stuff to attend to but I'll get the fix pushed later today, if @DaveBlake could get another testbuild for @scott967 to test and see if he can confirm it fixes the issue. 

Cheers

[DaveBlake]

1) Identifying boxed sets that are split over multiple directories.

Good that this flaw came out of Scott's testing. Ironically I had managed to do the same manual edits as @black_eagle to my boxset example in separate folders. Always more testing by mnay people the better!

@black_eagle in principle determining that an album is a boxed set in `CMusicDatabase.AddAlbum` is fine, I always saw it as processing that could belong to either the db or CAlbum rather than CMusicSanner. However designwise it would be nice to have that dertermination done in one place, otherwise some later dev will be left digging to find out why x is happening if the rules are applied part sanning part db save. We can discuss further on git (rather than scare normal users talking about code here).

2) Flaws in Estuary handling of Music
Yea, tell me about it, and my feedback on it when the skin was in alpha was ignored. I did try, but I am not a skinner and no one else at the time had any care for music. Hence I use Confluence, or recommend others explore the many other skins (some with some rich graphics).

3) Boxed sets and skins...
In principle I think it is OK for <i> on a disc to show the Album info dialog, rather than some "disc info" variant. The only disc specific data is number and subtitle and art - all shown on the list/wall screen of discs.

But showing the normal Album info dialog for info on a 13 disc boxed set is not the best user experience. The weakest point is the very long and useless track list. Then again the track list has always been dubious to me. If you want to see songs navigate to that node album>(discs)>songs, use the info dialog for extra info about the artist/album/song not as a substitute for the next level of node.

Having said that, if skinners felt that it would be nice to have variable content on container 50 (discs instead of songs maybe?) then always open to a later addition for that.

[docwra]

Anything needed on the metadata or artwork side for this feature?

I think its essential to work with Estuary as that's the default skin that 99% of the userbase will use.

[black_eagle]

However designwise it would be nice to have that dertermination done in one place

It is now.  And as it turns out, less code and probably faster.  I've run a couple of quick tests so far and everything looks OK, so I'm testing against 1000+ albums currently.  If that checks out I'll push it to github for your perusal.  Do you want it in a separate branch for now so as not to update the PR against master, or should I just push it to the current branch ?

[DaveBlake]

Sounds good @black_eagle . A separate commit initially would make life easiest for me, but would need squashing before merge. So I really don't mind if you use a new branch on local repo as a stepping stone, or work in the branch you raised against master. Know how you love git

[black_eagle]

@DaveBlake Raised it in a fresh PR(4) against what I've done so far in PR3 so easy for you to see the changes.  Also means once you're happy with it I can just merge it.