Skip to content

Sorting by role #1011

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.

Sign up for GitHub

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

Open
darrell-k wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Sorting by role #1011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
52d1891
sorting by role: WIP
darrell-k Dec 18, 2024
45cf021
Include roles in VALID_ALBUM_SORTS, allow sort for works, exclude art…
darrell-k Dec 19, 2024
d415061
retrieve roles for all displayed albums when viewing by artist
darrell-k Dec 19, 2024
879a2af
Merge branch 'master' into sorting-by-role
darrell-k Dec 20, 2024
1d57ab3
Merge branch 'fix-descending-sort-toggle' into sorting-by-role
darrell-k Dec 20, 2024
8605973
various refinements
darrell-k Dec 21, 2024
6570844
Cacheing,control rolesort when viewed albums change
darrell-k Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 61 additions & 2 deletions MaterialSkin/HTML/material/html/js/browse-functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1876,12 +1876,70 @@ function browseHeaderAction(view, act, event, ignoreOpenMenus) {
} else if (USE_GRID_ACTION==act) {
view.changeLayout(true);
} else if (ALBUM_SORTS_ACTION==act || TRACK_SORTS_ACTION==act) {
var sort=ALBUM_SORTS_ACTION==act ? getAlbumSort(view.command, view.inGenre) : getTrackSort(item.stdItem);
console.log("DK item.id="+item.id);
var sort=ALBUM_SORTS_ACTION==act ? getAlbumSort(view.command, view.inGenre, view.current.roleArray) : getTrackSort(item.stdItem);
var menuItems=[];
var sorts=ALBUM_SORTS_ACTION==act ? B_ALBUM_SORTS : B_TRACK_SORTS;
// var gotSelection;
for (var i=0,len=sorts.length; i<len; ++i) {
// gotSelection ||= sort.by==sorts[i].key;
menuItems.push({key:sorts[i].key, label:sorts[i].label, selected:sort.by==sorts[i].key});
}

if (LMS_VERSION>=90100 && ALBUM_SORTS_ACTION==act) {
Copy link
Owner

@CDrummond CDrummond Dec 21, 2024

Choose a reason for hiding this comment

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

Calling this every time the menu shown is inefficient. The result of this call should be stored with the current item, and reused if possible.

Copy link
Contributor Author

@darrell-k darrell-k Dec 21, 2024

Choose a reason for hiding this comment

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

I was trying doing this, but as soon as I started pushing to an array other than menuItems, the application would no longer wait for the server command results. The problem is I don't understand how the async stuff works.

Copy link
Owner

@CDrummond CDrummond Dec 21, 2024

Choose a reason for hiding this comment

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

It does not 'wait', the menuItems is 'reactive' - so vue.js will update UI when anything changes. What you need to do is copy the (parsed) details of this response somewhere, and also update menuIitems. If this cached version exists, then use that for menuItems.

Copy link
Contributor Author

@darrell-k darrell-k Dec 21, 2024

Choose a reason for hiding this comment

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

Ah, I understand, it's in the framework. No wonder I couldn't find it!

Copy link
Contributor Author

@darrell-k darrell-k Dec 23, 2024

Choose a reason for hiding this comment

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

OK, I'm about to push some changes which hopefully cache the roles command results correctly.

These changes require the LMS from my updated branch https://github.com/darrell-k/slimserver/tree/sorting-by-role

I've also decided to clear any role sort when changing the view, writing the default to local storage for new views (except if we're viewing all albums, because by definition all roles sorts would be available).

This avoids having a stored sort (in local storage) which might not apply to a new view, because the set of albums do not contain the previously-selected sort role.

There is commented out code for a different approach, which "pretends" the sort is the default 'album' one if the actually used sort is on a role which isn't used in a new album list. This works because the LMS query will return the albums_loop in album name sequence for a role sort where the role doesn't exist for any albums. This means if the user subsequently displays an album list where the previously used sort role exists, it would still be used. But this is a bit messy and possibly fragile.

I suppose the ideal solution would be to have getAlbumSort always validate a role sort against the current albums list, but I think that would mean really having to wait for the LMS rolesQuery to return.

Don't know what you think on this?

Anyway, lots more testing to do, but I thought I'd push this so you can see where I am right now. (And I know I have a few debug messages to remove!)

if (view.current.roleSortItems!=undefined && this.lmsLastScan==view.current.lmsLastScan) {
console.log("DK using stored role list");
for (var i=0,len=view.current.roleSortItems.length; i<len; ++i) {
// gotSelection ||= sort.by==view.current.roleSortItems[i].key;
menuItems.push({key:view.current.roleSortItems[i].key, label:view.current.roleSortItems[i].label, selected:sort.by==view.current.roleSortItems[i].key});
}
// if (!gotSelection) {
// // this means the stored sort is not available for the current list, so set the selected sort to 'album'.
// menuItems[0].selected = 1;
// }
} else {
console.log("DK refreshing role list");
view.current.lmsLastScan = this.lmsLastScan;
view.current.roleSortItems=[];
let id = originalId(view.current.id);
if (id.startsWith('artist_id')) {
let albums = [];
for (var i=0,len=view.items.length; i<len; ++i) {
if (view.items[i]['id'].startsWith('album_id:')) {
albums.push(view.items[i]['id'].split(':')[1]);
}
}
id = 'album_id:'+albums.join(',');
}
let command = {command:['roles'], params:[id]};
browseAddLibId(view, command.params);
lmsList('', command.command, command.params, 0, LMS_BATCH_SIZE, true, view.nextReqId()).then(({data}) => {
logJsonMessage("RESP", data);
if (data.result && undefined!=data.result.roles_loop) {
let excludeRole = [1,5,6]; // don't want artist, albumartist, trackartist
if (id.startsWith('work_id:')) {
excludeRole.push(2); // don't want composer if we're already viewing a work
}
view.current.roleArray = data.result.roles_loop.map(e => e.role_id).filter(e => !excludeRole.includes(e));
for (let r=0, loop=data.result.roles_loop, len=loop.length; r<len; ++r) {
let rid = parseInt(loop[r].role_id);
if (!excludeRole.includes(rid)) {
// gotSelection ||= sort.by==loop[r].role_id;
view.current.roleSortItems.push({key:loop[r].role_id, label:roleDisplayName(loop[r].role_id,1)+', '+sorts[sorts.map(e => e.key).indexOf('album')].label});
menuItems.push({key:loop[r].role_id, label:roleDisplayName(loop[r].role_id,1)+', '+sorts[sorts.map(e => e.key).indexOf('album')].label, selected:sort.by==loop[r].role_id});
}
}
// if (!gotSelection) {
// // this means the stored sort is not available for the current list, so set the selected sort to 'album'.
// menuItems[0].selected = 1;
// }
}
}).catch(err => {
//????
});
}
}

showMenu(view, {show:true, x:event ? event.clientX : window.innerWidth, y:event ? event.clientY : 52, sortItems:menuItems, reverseSort:sort.rev,
isAlbums:ALBUM_SORTS_ACTION==act, name:'sort'});
} else if (VLIB_ACTION==act) {
Expand Down Expand Up @@ -2540,7 +2598,8 @@ function browseReplaceCommandTerms(view, cmd, item) {
}
} else if (cmd.params[i].startsWith(SORT_KEY+ALBUM_SORT_PLACEHOLDER) ||
cmd.params[i].startsWith(SORT_KEY+ARTIST_ALBUM_SORT_PLACEHOLDER)) {
var sort=getAlbumSort(cmd, view.inGenre);
console.log("DK item.id="+item.id);
var sort=getAlbumSort(cmd, view.inGenre, undefined, item.id=='mm:/myMusicAlbums');
Copy link
Owner

@CDrummond CDrummond Dec 27, 2024

Choose a reason for hiding this comment

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

Why is "myMusicAlbums' getting sorts for all roles? I'm not sure it makes sense to sort the LMS albums lit by (e.g.) 'Guitarist' - that info is never shown.

Copy link
Contributor Author

@darrell-k darrell-k Dec 27, 2024

Choose a reason for hiding this comment

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

If you choose a role sort, LMS will return the sorted role person as the first artist in artists and artist_ids, causing the name to show first in the album list artist links.

Copy link
Owner

@CDrummond CDrummond Dec 27, 2024

Choose a reason for hiding this comment

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

Still, sounds like a change just because it can be done - not one, IMVHO, that make sense. I see a list of albums - ordering by _Album_Artist, Release date, or title make sense, anything else not so much. Why, with this particular list, would I want to sort by (e.g.) Guitarist? Makes no sense.

Copy link
Contributor Author

@darrell-k darrell-k Dec 27, 2024

Choose a reason for hiding this comment

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

Yeah, I did mention earlier in this thread that I wasn't sure about role sorts at the top level albums list. But once I removed artist/albumartist/trackartist from the generated sorts, it looked OK to me, so I thought "why not?". It's not inconceivable that someone might want to see a list of their albums by guitarist, producer or whatever.

Copy link
Owner

@CDrummond CDrummond Dec 27, 2024

Choose a reason for hiding this comment

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

Sorry, not convinced. Just because you can do something does not mean you should.

Copy link
Contributor Author

@darrell-k darrell-k Dec 29, 2024

Choose a reason for hiding this comment

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

Looking back at this, All that code is doing is ensuring that a previously-selected album sort is held when displaying all albums, on the basis that it has to be valid - see comment above:

I've also decided to clear any role sort when changing the view, writing the default to local storage for new views (except if we're viewing all albums, because by definition all roles sorts would be available)...

I suppose the wider point you are making is that you don't think the role sort options should be available in the all albums list at all. But why not there if we offer them (for example) when browsing albums for a particular genre? In the end, it's just some extra options added to the drop-down sort menu.

Copy link
Owner

@CDrummond CDrummond Dec 29, 2024

Choose a reason for hiding this comment

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

Because this very much sounds like adding something just because you can. In reality when would you ever want to order all your albums by the Guitarist? Why would you do that as opposed to entering the "Guitarist" browse mode?

...and the same goes for "Albums" under "Genres".

Do any other music servers add such a feature? As its not something I can personally see the point to. And I don't want to overcrowd a menu just because we can.

Copy link
Contributor Author

@darrell-k darrell-k Dec 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why would you do that as opposed to entering the "Guitarist" browse mode? ...and the same goes for "Albums" under "Genres".

Because it would avoid having to set up Additional Browse Menus for each role by Album (in addition to Role by Artist), and allow the user to quickly find different roles in whatever set of albums they are currently viewing rather than having to go back to the main menu and select a new browse mode each time. And even after doing that, the album list would not be sorted by the browsed role, only restricted to it.

Copy link
Owner

@CDrummond CDrummond Dec 29, 2024

Choose a reason for hiding this comment

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

I'm probably being slow here, so sorry for being a pain, but... and? Why would you want to sort albums by Guitarist? Artist, yes, but Guitarist? Makes no sense. Why would you want role-by-album??? If you care about Guitarist then you'd want Guitarist -> ABC -> albums. But sorting ALL albums by Guitarist? No, don't see the use-case. this very much sounds like "we have all these new role options, lets put these in all the possible places they could (not should) go"

(I'm using Guitarist as an example, I realise there are many more roles)

Again, does any other media server do such a thing (by default)?

Copy link
Contributor Author

@darrell-k darrell-k Dec 29, 2024

Choose a reason for hiding this comment

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

On other media servers I don't know, I don't use anything but LMS. A user did mention one which was more flexible than LMS, in that it allowed browsing by Album then Artist Role (or any arbitrary hierarchy - I think it might have been Minim), as well as Artist Role then Album, which is sort of in the same area.

Now, LMS does not offer this directly, but through the sort mechanism, we can offer the route.

For example, if I wanted to see all my jazz clarinet stuff, I could go to Genres -> Jazz -> Releases and then sort by Clarinet Player. Of course there are other routes, but I think this is a valid and useful one.

Bear in mind that only roles used on the albums being displayed will be added to the sort options. So if the user hasn't tagged composer/conductor/band or created user-defined roles for anything in the list, they won't see anything other than the existing sort options. And if they have tagged those roles it likely they are important to them. So it's not going to bombard the user with sort options irrelevant to them.

The origin of this PR was a request by users with a lot of instances of a work, and after displaying the works-album list they want to be able to sort the performances by orchestra, conductor, soloist, whatever. We could restrict the additional sort options to work-albums only, but as you will have worked out by now, I think that would be a shame, and once it's there for work-albums someone is bound to ask why it's not there for albums in general.

Another way of looking at this:

LMS long ago went beyond just Album/Artist/Year, with the introduction of Composer, Conductor and Band, and now we have user-defined roles. For many users, the days when Artist is the only (or even most important) role in a library are long gone.

But LMS sort options didn't keep up: the list of available sorts was hardcoded in the albums query, and it would throw an error if it got a sort parameter that it didn't recognise. On that basis, Material has the same hardcoded list. But now, if LMS is to allow sorting by these additional roles, why would Material not?

// Remove "sort:album" from "playlistcontrol" - LMS fails on this.
if (LMS_VERSION<80500 && 'album'==sort.by && isPlayListControl) {
cmd.params.splice(i, 1);
Expand Down
2 changes: 1 addition & 1 deletion MaterialSkin/HTML/material/html/js/browse-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1091,7 +1091,7 @@ var lmsBrowse = Vue.component("lms-browse", {
(INSERT_ACTION==loop[i].action && this.wide>=WIDE_MIX_BTN && this.showDetailedSubtoolbar && !lmsOptions.playShuffle) ||
(this.tbarActions.length<2 && (i<(this.tbarActions.length<2 ? 2 : 1))) ||
((ALBUM_SORTS_ACTION==loop[i].action || TRACK_SORTS_ACTION==loop[i].action) && this.items.length<2) ||
((ALBUM_SORTS_ACTION==loop[i].action || ADD_RANDOM_ALBUM_ACTION==loop[i].action) && this.current && this.current.stdItem==STD_ITEM_WORK) ||
// ((ALBUM_SORTS_ACTION==loop[i].action || ADD_RANDOM_ALBUM_ACTION==loop[i].action) && this.current && this.current.stdItem==STD_ITEM_WORK) ||
(SCROLL_TO_ACTION==loop[i].action &&
(!this.items[0].id.startsWith(FILTER_PREFIX) ||
(this.items.length < (this.grid.use ? (this.grid.numColumns*10) : 50) ) ) ) ||
Expand Down
10 changes: 9 additions & 1 deletion MaterialSkin/HTML/material/html/js/browse-resp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1011,7 +1011,15 @@ function parseBrowseResp(data, parent, options, cacheKey, parentCommand, parentG
}
}
if (reverse && !isNewMusic) {
data.result.albums_loop = data.result.albums_loop.reverse();
// sort field for role sorting might be null, if so make sure nulls stay below non-nulls even when reversed.
let loopNotNulls = [];
let loopNulls = [];
const firstNullPos = data.result.albums_loop.map(e => e.rolesort_name).indexOf('');
if (firstNullPos>0) {
loopNotNulls = data.result.albums_loop.slice(0,firstNullPos);
loopNulls = data.result.albums_loop.slice(firstNullPos);
}
data.result.albums_loop = loopNulls.length ? loopNotNulls.reverse().concat(loopNulls) : data.result.albums_loop.reverse();
}
}
var albumGroups = !isWorksAlbums && (groupReleases && haveReleaseType && lmsOptions.supportReleaseTypes && lmsOptions.groupByReleaseType>0) ? {} : undefined;
Expand Down
2 changes: 1 addition & 1 deletion MaterialSkin/HTML/material/html/js/standarditems.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ const STD_ITEMS=[
},
{
command: ["albums"],
params: [ALBUM_TAGS_PLACEHOLDER/*, SORT_KEY+ALBUM_SORT_PLACEHOLDER*/],
params: [ALBUM_TAGS_PLACEHOLDER, SORT_KEY+ALBUM_SORT_PLACEHOLDER],
menu: [PLAY_ACTION, INSERT_ACTION, PLAY_SHUFFLE_ACTION, ADD_ACTION, ADD_RANDOM_ALBUM_ACTION, DIVIDER, ADD_TO_FAV_ACTION, SELECT_ACTION, COPY_DETAILS_ACTION/*, CUSTOM_ACTIONS, MORE_LIB_ACTION*/],
actionMenu: [DIVIDER, INSERT_ACTION, PLAY_SHUFFLE_ACTION, ADD_RANDOM_ALBUM_ACTION, DIVIDER, ADD_TO_FAV_ACTION/*, CUSTOM_ACTIONS, MORE_LIB_ACTION*/]
},
Expand Down
24 changes: 19 additions & 5 deletions MaterialSkin/HTML/material/html/js/utils-deferred.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,13 +499,18 @@ function commandAlbumSortKey(command, genre) {
const VALID_ALBUM_SORTS = new Set(["album", "artistalbum", "artflow", "yearalbum", "yearartistalbum"]);
const VALID_TRACK_SORTS = new Set(["title", "tracknum", "albumtrack", "yearalbumtrack", "artisttitle", "yeartitle"]);

function getAlbumSort(command, genre) {
function getAlbumSort(command, genre, roleArray, allroles) {
let key = commandAlbumSortKey(command, genre);
let def = ALBUM_SORT_KEY==key || (ALBUM_SORT_KEY+"C")==key ? "album" : "yearalbum";
let parts = getLocalStorageVal(key, def).split(".");
let val = {by:parts[0], rev:parts.length>1};
if (!VALID_ALBUM_SORTS.has(val.by)) {
// let rolesort = LMS_VERSION>=90100 && (BASE_ARTIST_TYPE_IDS.map(String).includes(val.by) || lmsOptions.userDefinedRoles[val.by]!=undefined) ? 1 : 0;
let rolesort = allroles
? LMS_VERSION>=90100 && (BASE_ARTIST_TYPE_IDS.map(String).includes(val.by) || lmsOptions.userDefinedRoles[val.by]!=undefined)
: roleArray && roleArray.map(String).includes(val.by);
if (!VALID_ALBUM_SORTS.has(val.by) && !rolesort) {
val.by = def;
setLocalStorageVal(key, def);
}
return val;
}
Expand Down Expand Up @@ -810,17 +815,26 @@ function isSameArtistsL(item, rolea, roleList) {
return false;
}

function roleDisplayName(role) {
function roleDisplayName(role, includeArtists) {
if (undefined==role) {
return undefined;
}
let val = isNaN(role) ? roleIntValue(role) : parseInt(role);
if (val<=0) {
return undefined;
}
if (val==ARTIST_ROLE || val==ALBUM_ARTIST_ROLE || val==TRACK_ARTIST_ROLE) {
if (!includeArtists && (val==ARTIST_ROLE || val==ALBUM_ARTIST_ROLE || val==TRACK_ARTIST_ROLE)) {
return undefined;
}
if (val==ARTIST_ROLE) {
return i18n("Artist");
}
if (val==ALBUM_ARTIST_ROLE) {
return i18n("Album artist");
}
if (val==TRACK_ARTIST_ROLE) {
return i18n("Track artist");
}
if (val==BAND_ARTIST_ROLE) {
return i18n("Band/orchestra");
}
Expand Down Expand Up @@ -868,4 +882,4 @@ function arraysEqual(a, b) {
}
}
return true;
}
}