More releases rework #1303

darrell-k · 2025-01-16T18:26:09Z

Looking into https://forums.lyrion.org/forum/user-forums/logitech-media-server/1746593-9-0-2-albums-search-query-fails I found that at some point in the many versions of Releases.pm the album_id array (introduced by me!) became redundant.

At least, I can't find any differences in the results from albumsQuery.

This also fixes the bug where the search parameter was being ignored. And another one I found when testing: the /R/ tag wasn't working properly when no artist_id was passed, it was using only albums.contributor to find the required roles, which meant that other roles for the album were not being passed back.

I also simplified the main $request which drives _releases: $menuRoles is already added to @searchTags and the alternative of adding all roles to the request is unnecessary (and might be causing an explosion in the number of contributors added to artists and artist_ids returned by albumsQuery).

Once this gets past pre-merge testing, we should ask users of 9.0.2 to give release types in Default Skin a good going over.

Signed-off-by: darrell-k <[email protected]>

darrell-k · 2025-01-16T18:26:46Z

...and the DCO worked :)

michaelherger · 2025-01-16T21:52:15Z

Slim/Control/Queries.pm

+				if ( $contributorID ) {
+					$contributorRoleSql .= " AND contributor = ?" if $contributorID;


The check for $contributorID is redundant here.

Signed-off-by: darrell-k <[email protected]>

darrell-k · 2025-01-17T10:25:02Z

While we're here, maybe we should increase this (Material Skin would use 25000)?

slimserver/Slim/Menu/BrowseLibrary/Releases.pm

Line 10 in 6dc2b5e

use constant MAX_ALBUMS => 1500;

michaelherger · 2025-01-17T12:12:05Z

While we're here, maybe we should increase this (Material Skin would use 25000)?

slimserver/Slim/Menu/BrowseLibrary/Releases.pm

Line 10 in 6dc2b5e

use constant MAX_ALBUMS => 1500;

Has it ever caused a problem? Do you remember why we set this relatively low? In the case of genres for an album this certainly isn't a problem anyway.

But just skimming that page a few things sprang to my mind:

https://github.com/LMS-Community/slimserver/blob/6dc2b5e8c9401545728f1a9bfbfb97228890607e/Slim/Menu/BrowseLibrary/Releases.pm#L209C6-L209C13

You're redefining multiple variables of the same name $request. That's somewhat risky, as you might at some point want to use an earlier instance with the same name. Either re-use the same variable if you don't need the old request, or create new variables.

On the same line: we don't need a $client there, do we?

The outer, greater loop doesn't name $_. Again, somewhat risky, as you might have inner loops, trying to use the same. In such a large loop I'd name variables for clarity. But this has nothing to do with the change 😁. Just while you're at it.

darrell-k · 2025-01-17T12:23:15Z

Has it ever caused a problem? Do you remember why we set this relatively low? In the case of genres for an album this certainly isn't a problem anyway.

With these changes it would only cause a problem if not all release types/contributor roles were identified by looking at the first 1500 albums. But I don't think that increasing it substantially would cause problems, it's not like we're pumping the results out over the network to clients. (Material gets away with its much larger limit, even though that is going out to the client.)

I'll work on the other points you make.

darrell-k · 2025-01-20T16:49:00Z

@michaelherger Further testing of this has brought to light a situation where the album_id array is still needed (This might be the main reason it was introduced, I wish I could remember exactly):

Consider 2 albums where on one, an artist is both ARTIST and COMPOSER, and another where they are only COMPOSER (eg Bob Dylan is often tagged like this, because of the number of covers by other artists).

For the album where Bob is both ARTIST and COMPOSER, the main loop will add the album id only to $albumList{ALBUMS}, not the COMPOSER one. And the one where he is only COMPOSER will be added to $albumList{COMPOSER} array.

All is well with the current code when creating the menu entries with album_id:<array values> except when the 999 SQL parameter limit is busted, but without it, the COMPOSITIONS/COMPOSER menu entry can only use role_id:COMPOSER which will of course also return tracks/albums where Bob is both ARTIST and COMPOSER.

So either:

cross our fingers and continue to always use the album_id arrays. (the limit will be busted less often with the fix to take account of the "search" parameter).
a version of 1. that uses role_id instead of album_id in the menu entries when there is no artist_id passed in, on the basis that (probably) every album will have been tagged with an ARTIST, so nothing would go into the COMPOSER category in this case. This should reduce the SQL parameter limit problem to only lists for artists where there are over 995 or so albums in a single release type category.
don't pass the album_id array, but add a not_role_id parameter to albumsQuery and titlesQuery, so that we could pass for example role_id:COMPOSER and not_role_id:ARTIST,ALBUMARTIST to those queries for the COMPOSITIONS/COMPOSER menu entries.

I think 1 is not good enough, and that 2 would increase the complexity of _releases again, so I think I'd go for 3. But what do you think?

And, coincidently, a user has asked on the forums if they could also see compilations in their user-defined release type categories (that is, not one of @PRIMARY_RELEASE_TYPES as defined in Slim::Schema::Albums. This seems reasonable to me, but what do you think?

michaelherger · 2025-01-20T17:10:30Z

Thanks for the update. For 9.0 I'd actually opt for 1.) - with an additional cap added to the array. Just trim it if it's too long. It's unlikely to happen, as you say. And would require the fewest changes, wouldn't it?

For 9.1 you could still try to do the real fix. Just make sure you document the decision and the array in a comment so next time we better understand the decision.

Signed-off-by: darrell-k <[email protected]>

darrell-k · 2025-01-21T17:41:53Z

Latest version pushed: not fully tested yet, but making it available for comments.

michaelherger · 2025-01-22T06:11:06Z

Slim/Control/Queries.pm

+				my $rolesRef;
+				my $contributorRoleSql = "SELECT role FROM contributor_album WHERE album = ?";
+				if ( $contributorID ) {
+					$contributorRoleSql .= " AND contributor = ?";


Only this line is conditional, isn't it? Keep the rest outside, as it's identical. That would be less code to maintain.

Slim/Menu/BrowseLibrary/Releases.pm

michaelherger · 2025-01-22T07:10:42Z

Slim/Menu/BrowseLibrary/Releases.pm

+	my $limitList = sub {
+		my $list = shift;
+		my $name = shift;
+		my $albumCount = scalar @$list;
+		if ( $albumCount > LIST_LIMIT ) {
+			splice @$list, LIST_LIMIT;
+			$name .= ' (' . cstring($client, 'FIRST') . ' ' . LIST_LIMIT . ' ' . cstring($client, 'ALBUMS') .')';
+		}
+		return $list, $name;
+	};


I usually prefer not to create closures unless it really simplifies things by re-using a lot of the variables. The only one missing here would be $client. Would you mind changing this to a regular sub?

Adding new strings sucks. But concatenating strings often doesn't work with language other than "yours". Of the three languages I know one would fail (French). I know this will hardly ever be used, but could you still create a new string? Thanks!

michaelherger · 2025-01-22T07:14:54Z

Slim/Menu/BrowseLibrary/Releases.pm

+			splice @$list, LIST_LIMIT;
+			$name .= ' (' . cstring($client, 'FIRST') . ' ' . LIST_LIMIT . ' ' . cstring($client, 'ALBUMS') .')';
+		}
+		return $list, $name;


As $list is a list reference, you actually don't have to return it. It's modified directly. Therefore ($listRef, $name) = $limitList($listRef, $name); is overwriting $listRef with itself, like $a = $a. You don't have to return $list.

Unless I'm completely wrong, of course.

Signed-off-by: darrell-k <[email protected]>

darrell-k · 2025-01-22T12:30:01Z

Changes pushed. Seems to work well. I used deepl.com for translations.

michaelherger · 2025-01-22T15:45:40Z

Slim/Control/Queries.pm

 				my $contributorRoleSql = "SELECT role FROM contributor_album WHERE album = ?";
+				$contributorRoleSql .= " AND contributor = ?" if $contributorID;
+				$contributorRoleSth ||= $dbh->prepare_cached($contributorRoleSql);
 				if ( $contributorID ) {
-					$contributorRoleSql .= " AND contributor = ?";
-					$contributorRoleSth ||= $dbh->prepare_cached($contributorRoleSql);
 					$rolesRef = $dbh->selectall_arrayref($contributorRoleSth, , undef, $c->{'albums.id'}, $contributorID);
 				} else {
-					$contributorRoleSth ||= $dbh->prepare_cached($contributorRoleSql);
 					$rolesRef = $dbh->selectall_arrayref($contributorRoleSth, , undef, $c->{'albums.id'});
 				}


I'm really sorry about the previous comment on this and my request to change it... the $rolesRef line was wrapped on my screen and I thought it was identical in both lines... I thought only one line was conditional, when the SQL statement, and calling it, are, of course. Considering this, I'd prefer the previous version. I'm really sorry!

But while we're at it: is there one comma too many in selectall_arrayref($contributorRoleSth, , undef, $c->{'albums.id'})? Before the undef?

I wondered about that. According to the definition:

$ary_ref = $dbh->selectall_arrayref($statement, \%attr, @bind_values);

I suppose what must be happening with the existing code is that Perl is constructing @bind_values from undef, $c->{'albums.id'} and $contributorID and then ignoring the undef element when deciding if we have the right number of bind variables.

I've changed the statements.

Actually, with a bit more playing about, it's not doing that, it seems to just ignore the repeated comma. Anyway...

And with all this parameter confusion you didn't see my being sorry? 😁 Never mind: I'm going to merge and revert what I made you do myself. I'm still sorry about that confusion.

Saw it, but thought it didn't matter one way or the other. One less line of code but an extra if ...

michaelherger · 2025-01-22T15:47:41Z

Changes pushed. Seems to work well. I used deepl.com for translations.

If you're using Visual Studio Code you can enable Copilot, which has been super helpful with this kind of work: after writing the EN version, it would often be enough to type the new language code, and Copilot would automatically complete the full line.

Signed-off-by: darrell-k <[email protected]>

michaelherger

Thanks!

More releases rework

d181fae

Signed-off-by: darrell-k <[email protected]>

michaelherger reviewed Jan 16, 2025

View reviewed changes

remove inline if properly

f14f44b

Signed-off-by: darrell-k <[email protected]>

9.0 version: limit album results per category; code tidying

80dec4f

Signed-off-by: darrell-k <[email protected]>

michaelherger requested changes Jan 22, 2025

View reviewed changes

some rework

69e9948

Signed-off-by: darrell-k <[email protected]>

michaelherger requested changes Jan 22, 2025

View reviewed changes

tidy up selectall_arrayref statements

73dff39

Signed-off-by: darrell-k <[email protected]>

michaelherger approved these changes Jan 22, 2025

View reviewed changes

michaelherger merged commit 75dcb91 into LMS-Community:public/9.0 Jan 22, 2025
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More releases rework #1303

More releases rework #1303

darrell-k commented Jan 16, 2025

darrell-k commented Jan 16, 2025

michaelherger Jan 16, 2025

darrell-k commented Jan 17, 2025

michaelherger commented Jan 17, 2025

darrell-k commented Jan 17, 2025

darrell-k commented Jan 20, 2025

michaelherger commented Jan 20, 2025

darrell-k commented Jan 21, 2025

michaelherger Jan 22, 2025

michaelherger Jan 22, 2025

michaelherger Jan 22, 2025

darrell-k commented Jan 22, 2025

michaelherger Jan 22, 2025

darrell-k Jan 22, 2025

darrell-k Jan 22, 2025

michaelherger Jan 22, 2025

darrell-k Jan 22, 2025

michaelherger commented Jan 22, 2025

michaelherger left a comment

		if ( $contributorID ) {
		$contributorRoleSql .= " AND contributor = ?" if $contributorID;

More releases rework #1303

More releases rework #1303

Conversation

darrell-k commented Jan 16, 2025

darrell-k commented Jan 16, 2025

Choose a reason for hiding this comment

darrell-k commented Jan 17, 2025

michaelherger commented Jan 17, 2025

darrell-k commented Jan 17, 2025

darrell-k commented Jan 20, 2025

michaelherger commented Jan 20, 2025

darrell-k commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrell-k commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelherger commented Jan 22, 2025

michaelherger left a comment

Choose a reason for hiding this comment