Browse albums by record label: initial version #1382
Conversation
Signed-off-by: darrell-k <[email protected]>
There was a problem hiding this comment.
So I started this review, expecting "basic functionality to allow browsing albums by record label". But after leaving some of my relatively unimportant nitpick comments I came to realise that code to actually browse albums by label is missing, right? You'll have to add label filtering on the album level. And implement the missing sub _labels etc. I therefore stop here and leave the reviews as they are. I know that this is not done yet. So here we go anyway...
Smart move to have added a label column more than a year ago 😉.
Is a single label good enough (I certainly hope so - but you never know)?
There was a problem hiding this comment.
serverstatusQuery && infoTotalQuery might need an update? They're missing eg. works, too
Slim/Control/Queries.pm
Outdated
| @@ -296,6 +296,7 @@ sub albumsQuery { | |||
| my $work = $request->getParam('work_id'); | |||
There was a problem hiding this comment.
Please remove the tab after $work to fix the alignment. Now that's really important.
Slim/Control/Queries.pm
Outdated
|
|
||
| if (defined $contributorID) { | ||
|
|
||
| $sql .= 'JOIN contributor_album ON albums.id = contributor_album.contributor '; |
There was a problem hiding this comment.
Please indent the first line by one more tab
| my $libraryID = Slim::Music::VirtualLibraries->getRealId($request->getParam('library_id')); | ||
| my $tags = $request->getParam('tags') || ''; | ||
|
|
||
| my $sql = 'SELECT distinct label FROM albums '; |
There was a problem hiding this comment.
We don't have an index on label yet. SQLite is fast, in particular with the usually not too big albums table. Therefore I'd say let's cheat that index in without bumping the database version. That way we don't require a full wipe & rescan, but those doing so anyway will still take advantage of the new index.
There was a problem hiding this comment.
What is the preferred method?
There was a problem hiding this comment.
Just add it to schema_23_up.sql, where you've added the column.
There was a problem hiding this comment.
OK, I didn't know 'old' schema files would be run again.
There was a problem hiding this comment.
Oh, you're actually right 😞: we only wipe the tables during a wipe & rescan. I thought we'd drop the database and build from scratch. So that won't work - unless someone went to manually delete the file. I fear we'll need another migration file. Shouldn't hurt those coming from 9.0, but only those already on 9.1 (@frank1969b).
There was a problem hiding this comment.
Oh, you're actually right 😞: we only wipe the tables during a wipe & rescan. I thought we'd drop the database and build from scratch. So that won't work - unless someone went to manually delete the file. I fear we'll need another migration file. Shouldn't hurt those coming from 9.0, but only those already on 9.1 (@frank1969b).
We could do something like a "create index if doesn't exist" in server startup. Maybe a bit hacky.
There was a problem hiding this comment.
I just noticed that we don't have indexes extid for albums or contributors either. Not sure whether we use them. But eg. counting all albums where extid is not null takes a fraction of a second on a Pi4 with about 200 out of 1700 albums having an extid. Nothing compared to Frank's collection, of course. Would you know off the top of your head whether we ever query for albums or artists with/without extid? You might add them to the .sql files while we're at it...
Anyway: we could put such a hack in place for a few weeks. Should just not forget to remove it after a while Slim::Schema::init() might be a candidate. Maybe log a warning to remind us of the hack?
Slim/Control/Queries.pm
Outdated
|
|
||
| if ($valid && $tags ne 'CC') { | ||
|
|
||
| $sql .= 'ORDER BY 1 '; |
There was a problem hiding this comment.
Wow... hadn't known this shortcut. But... please use the column name for easier reading.
|
I think you can already browse albums by label - that is pick the label and browse its albums? |
Signed-off-by: darrell-k <[email protected]>
Signed-off-by: darrell-k <[email protected]>
Signed-off-by: darrell-k <[email protected]>
|
Don't know whether you have access to CoPilot on Github. Here's what it thinks about this PR: Review and ObservationsStrengthsNew Feature Introduction:The addition of Code Consistency:The changes integrate well with the existing query framework (e.g., consistent use of Caching:Caching in Backward Compatibility:Changes like adding label to the schema and extending queries appear to be non-breaking for existing functionality. Potential ConcernsDatabase Schema Changes:The changes introduce a new column label in the albums table. This requires a migration step to ensure the database schema is updated. It's unclear if this migration is handled separately. Performance Impact:The new Testing:No information is provided about tests for the new functionality. It's important to ensure that unit tests and integration tests cover edge cases and regressions. New File Omission:The contents of Slim/Menu/BrowseLibrary/Labels.pm are not included in the diff, making it difficult to review its implementation. Error Handling:In SuggestionsDatabase Migration:Make sure migration scripts for adding the label column to the database are included and well-documented. Indexing:Ensure the new label field is indexed to optimize query performance. Testing:Add comprehensive tests for:
Documentation:Update any user-facing documentation to reflect the new functionality (e.g., how to use the record_label filter). Code Comments:Add comments to the new labelsQuery function for better maintainability. |
|
Statements that are (a) obvious, (b) miss the point, or (c) wrong. I have absolutely no time for AI. |
|
|
||
| foreach (@$items) { | ||
| $_->{'name'} = $_->{'record_label'}; | ||
| $_->{'image'} = 'html/images/albums.png'; |
There was a problem hiding this comment.
I was surprised by the number of labels found in my (test) collection. Must be because many of those files come from users with extensive metadata 😁.
But looking at that list I was wondering... albums.png all over the place doesn't make sense IMHO. I'd prefer a text list only. Or did you consider adding support for label artwork?... Would require another schema change. Or some MAI work? I bet people caring about labels would recognise those logos.
There was a problem hiding this comment.
I thought the same thing: a job for MAI if it's feasible. I just used album as a placeholder. For the top level menu icon, we could do with a label.png but I am certainly not the right person to be designing one. Maybe you could ask AI to come up with a record label icon in the style of LMS. If it does a good job, I might change my opinion of its usefulness 😂.
There was a problem hiding this comment.
AI has failed me (in the short time I tried).
Just in case you were considering a dedicated labels table, reserve a column for a logo ID...
You for sure already know there is always this person with the most complicated setup. So here I'm with albums tagged with multiple labels :-) |
How so? A release by definition is by a single record label, I would have thought. I know it's sometimes complicated where there may be different imprints or even different labels in different territories, but surely they would count as different albums? If you can provide details of your use case, we can consider it. |
You better try to talk @pascalberger out of it 😂. I think this will increase complexity a lot for what seems like an extremely esoteric use case... |
I think it depends on what you consider a label. Following the definition used by e.g. Discogs, releases with multiple labels is very common. E.g.
Same for MusicBrainz (e.g. https://musicbrainz.org/release/cb230074-a468-4012-9900-59268364f649) |
|
Thanks for tagging me. I will keep an eye sehen there is a build that smells Like full rescan ;-) Personally - as someone from the "music industry" - I don't Tag Labels for nowadays it's the rightowner that counts and many well known artists keep them for theirselves and licence them for a Limited period only to a label. Further more in the past the UK and/or US Label were different from the German one |
|
I suppose it comes down to the difference between an album and a release, although LMS often uses the terms interchangeably. Yes an album can have many releases, sometimes on different labels, but I people would generally tag those releases as different albums, as the reason for possessing multiple versions (in a digital or digitized library) would be remastering/differences in the track list, etc. This is different to the world of physical media where collectors might be interested in different album covers, physical format, vinyl colour/weight etc. At the risk of further opening the can of worms, what does Musicbrainz think? If catalogue number is an attribute of album, then that would support the album=release option. |
|
In each of the multiple label examples, they appear to be parent and subsidiary label or two subsidiaries from the same parent label. I guess the question here for @pascalberger and others is what is your use case for browsing by label and would you really want to browse by Virgin or Hut to find that Smashing Pumpkins album? How do you see yourself using Browse by Label in LMS? How will the Label tag be populated? Does Picard import that tag? I don't think I have seen a Label tag imported by mp3tag even with the extended MB webscript? Is limiting to one label per Album a problem for your use cases? **EDIT: Correction to the above My Calissical collection is small, but I understand how Browse by Label would be useful for specific labels that have prolific Classical collections. Given I have an extensive Jazz collection, the thought of browsing by Blue Note for example is interesting and there are a few others that could be interesting. In other genres of music I could see browsing specific indie labels that have a thematic focus to create a playlist. But I personally would not use browse by label for a mega label like Virgin. So the question here is keeping this simple and only supporting browse by the first label sufficient for most use cases. I think so, but it will mean that some will have to go through and clean up some Label tags if they have already been populated with multiple entries. Since Browse by Label should be put in the category of "Power Tagger" user is this really issue worth over complicating this for? And before you answer "yes, this must support multiple labels," consider that this requirement may delay or completely prevent Browse Label being implemented. FYI - I am ok either way as I am not sure if and when I would take the time to go add Label tags to my music, but figured I would just share a perspective of realistic usage that sometimes gets lost in these conversations. **EDIT: I guess I do have some labels tagged as Publisher so I would certainly check out the results to see if it is useful for me, but still not sure how much effort I would make to clean it up or how soon I would do that. |
|
As one that partakes in "extreme tagging" I've just run a query on my tagging database and there are 5662 distinct labels in it, of which 345 contain a delimited string in the record, meaning there is more than one label tagged to the album. They've all come via Picard (overriding majority I imagine) or were already pre-populated when buying digital content. labelLichtdicht Records\\[PIAS] Germany If I ran the 5662 through string-grouper I'm pretty sure I could whittle the 5662 list down significantly, and it is something I would do, because having 8 different instances of [PIAS] or 3 different instances of ECM is not useful to me, whereas one would be would be because, like Blue Note, Frontier Records etc. they are a stable for similar genres and/or musicians. As to whether multiple labels matter, probably not, but if it's simple to create a 1:many relationship without taxing performance then having something pop up under more than one label shouldn't be a big deal. As with my comments on forum regarding Genres, being able to select numerous labels for browsing content e.g. Frontiers Music and Nuclear Blast is likely to reveal a richer pool of choice than either in isolation, much as being able to do the same for genres would. |
Music metadata has always been a mess, which is why it’s important for software like Lyrion to have an opinion (i.e. decide on how it will handle something that relies on metadata, implement it and publish it for users to opt in, or not, as the case may be). Absent that, progress is impossible. |
|
Currently when browsing "Record Label" an album list is shown, I'd like to change that for Material. However, it does not (currently) seem possible to use the "record_label:XXXX" as a filter. e.g. I'd like to be able to call ["artists", 0, 1000, "record_label:XXX"] to get an artist list associated with the record label - likewise "genres" and "years". Ideally I'd like to be able to navigate "My Music / Record Label" and then show within this list the list of other browse modes (so excluding "Record Label"). Then you could browse "My Music / Record Label / EMI / Years / 1990 / Genres / Hard Rock / Artists / XXX" To do this the various commands need to be able to filter on "record_label:" Is it planned to add this? Or will it only be possible to filter albums? Is/will "record_label" be returned in the player status query? That way I can show this in the MAI track view. Also, the browse mode label should be pluralised to match others - "Record Labels" not "Record Label". |
|
You forgot to filter the artists by their role on the given label and release type 🤯. Poor @darrell-k who thought he was providing a nice little addition. It could all be done. But this will become a nightmare (not to mention a major effort). Are people really that heavily into labels? Wow... |
|
I don't mind all the suggestions, after all, I did call this the "initial version" and was fully expecting requests for extra functionality. Also it seems worth implementing the many-to-many relationship with albums. But those impatient for this need to pray for rain in the south of England, it's not looking good for you at the moment ;-) |
It’s England, give it an hour. |
Basic functionality to allow browsing albums by record label.