Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Album list experiment #1073

Draft
wants to merge 12 commits into
base: public/9.0
Choose a base branch
from

Conversation

darrell-k
Copy link
Contributor

Very much a work in progress:

  1. Replaces the (IMHO) odd behaviour where the user's choice of 1 or 2 artist menu entries changes the category of contributors added to album list entries. The user now defines which roles to link in the album list via the Settings-->Interface preference
  2. Fixes what looks like a bug where for artist album lists, we were running a complicated SQL to flatten multiple artists or multiple roles into a single array only for it to never (as far as I can tell) be used.

There's definitely more work to do (I think I found an inconsistency in albumsQuery which I've highlighted in this code, not sure what the answer is) but am creating a draft PR as an aid to discussion.


<option [% IF NOT prefs.pref_showArtist %]selected [% END %]value="0">[% 'DISABLED' | getstring %]</option>
<option [% IF prefs.pref_showArtist %]selected [% END %]value="1">[% 'ENABLED' | getstring %]</option>

</select>
</select> -->
<input type="text" class="stdedit" name="pref_showArtist" id="showArtist]" value="[% prefs.pref_showArtist | html %]" size="40"><br>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need sensible default, and migration from the current 1/0 options.

@@ -872,6 +828,7 @@ sub albumsQuery {

$request->addResultLoopIfValueDefined($loopname, $chunkCount, 'artist', $c->{'contributors.name'});
}
# !!!!!!!!!!!! So which way to go????
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, but we need to go one way or the other...

@@ -1532,19 +1533,23 @@ sub _albums {
}
else {
$_->{'artists'} = [ $_->{'artist'} ];
$_->{'artist_ids'} = [ $_->{'id'} ];
$_->{'artist_ids'} = [ $_->{'artist_id'} ];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a bug - we were setting the artist_id to the album id

### but maybe you've searched for a track artist & want to see the album artist in the list!
# if (!$wantMeta) {
# delete $_->{'artist'};
# }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained in the comments.

strings.txt Outdated
HE הצגת מבצע עם אלבומים
IT Visualizza artista assieme agli album
NL Artiest bij albums tonen
NL Artiestenrollen bij albums tonen
NO Vis artist ved album
PL Pokazuj wykonawców z albumami
RU Показать исполнителя с альбомами
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete. If we go this way, we'll need to revise the help text as well.

@michaelherger
Copy link
Member

I haven't looked into this in detail yet. But you want to do away with an option which has been around for a while, and which people have become used to. You would need to have great reasons to do so. What are they? What actual user problem are you trying to solve?

The most complicated query you seem to want to get rid of you added only a few days ago, didn't you? I'm confused.

@darrell-k
Copy link
Contributor Author

It is splitting the option, not doing away with it. With this change the user would be able to have their choice of Artist menus (and the role selections in the combined list) separated from their choice of which contributor roles to include in the album contributor links. We could of course make it so that the default would be to replicate the existing behaviour.

Let's look in more detail at what is happening currently (for whole albums, not works):

  1. When not filtering by role:
    (a) 2 artist lists: This will create links for ALBUMARTIST, or ARTISTs if no ALBUMARTIST.
    (b) Single artist list: This will choose the role with the highest numerical value, as long as role exists for the album and the role is in the extra roles from the user Artist menu preference or is ALBUMARTIST or ARTIST and build the list of links for that role only.

  2. When filtering by role:
    (a) 2 artist lists: This uses the roleID(s) active in the query as well as ALBUMARTIST and ARTISTs and builds a flat list of all contributors found, in descending role id sequence, so artists from multiple roles appear in the final list, unlike 1.
    (b) Single artist list: The same as 2(a) with the addition of the user's chosen extra roles from the Artist menu preference.

Then there is the question of what "filtering by role" means: technically it means $roleID is populated. From a user point of view I documented it for a previous PR - the changes I proposed in the second table in this post #953 (comment) were accepted and merged over the Xmas period at the end of last year.

There was also some forum discussion which resulted in me writing this: https://forums.slimdevices.com/forum/developer-forums/developers/1664597-contributor-roles-and-the-album-display-ui#post1664597 - an early take on the issue before hitting on the current idea of offering more flexibility by extending the preferences.

The user problem I am hoping to solve with this approach is that currently the role types used to create the contributor links in album lists change, depending on a seemingly unrelated user setting: the choice of separate or combined artist menus.

I also think that getting away from a dependency on the "role filter" is a great reason: that option should stick to its description, which is to control the selection of results for an artist, and should not affect the metadata to be shown for those results.

Finally, the user will be able to add links for multiple contributor roles of their choice to the album listing.

In my opinion, these changes will make LMS easier to use and understand, for existing and new users. (And as I said above, we can set defaults so that if an existing user does nothing, things carry on as before). The existing logic works well enough if all you've got is ARTIST and ALBUMARTIST tags, but as you know a lot of users have much more complex tagging systems.

Now, onto the technical question:

Yes, I just introduced a new query for Works a couple of days ago, and yes, it is greatly simplified (and the "role filter" query, which was almost as complicated, apart from the Works version having to create a virtual sort column, goes away completely).

The reason is, that having had to get my head around what those queries were doing, in order test the Works changes, I realised we could have a greatly simplified query (well 2 queries, one based on contributor_album, for whole albums, and one based on contributor_track, for works) by retrieving all roles for the album or work into a hash, and then reading out of that hash what we want, in whatever sequence we want, based on the new artist role preference list or a list based on the existing logic, or whatever. I think it's much easier to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants