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 sorting by role #1323

Open
wants to merge 1 commit into
base: public/9.1
Choose a base branch
from

Conversation

darrell-k
Copy link
Contributor

Replacement for #1322

Allow sorting by any role which exists for the set of albums being displayed.

This PR implements the changes necessary for Default Skin, and additional changes to allow this functionality also for Material Skin (PR in progress).

Signed-off-by: darrell-k <[email protected]>
@darrell-k darrell-k mentioned this pull request Feb 6, 2025
@michaelherger
Copy link
Member

I thought I'd give this a try before trying to understand the working... The outcome has been irritating. Eg. in Default if I sort by Conductor, and I only have few of them, the page size would be massive. It's set to 50 items per page. But depending on the page, there can be thousands of them. Most likely a "no role defined" page, where there wouldn't be any conductor for all my pop/rock music. So paging is completely broken.

And the usefulness of this is questionable, when there's no indication of the grouping. I seem to have conductors starting with A-J, but nothing there after? And who are they anyway? There's no indication in the Default skin, is there?

@michaelherger
Copy link
Member

So I'm testing this with your test library on a MacBook Pro, M2 Pro, 16GB RAM, SSD. And it takes something like 30s or more to render the "-" page with 15k items. How would that perform on the typical pCP installation? I haven't profiled the process, don't know whether it's the admittedly slow rendering of the html, or the rendering on the client side, or the additional DB work. But do you believe this is usable?

I hope Material would behave better, as it's not rendering thousands of items on the server side. And it would hopefully render like group headings. Otherwise this is IMHO useless. And therefore I wonder whether we should even try to implement this in the Default skin.

@CDrummond
Copy link
Contributor

I must admit that I'm not 100% sold on its usefulness - not sure I really see the use case for it (as mentioned on the Material pull request).

@darrell-k
Copy link
Contributor Author

So paging is completely broken.

Yes, sorry I didn't notice this (I did performance testing with curl JSON calls). It's because of the default skin paging which won't split a textkey over multiple pages, and everything without the chosen role is lumped together under the same empty key at the bottom of the list. Not trivial to fix, but I have a couple of ideas. Stay tuned.

(BTW, it's not great with that big library even under the default album sort, there are some massive pages, 4000 albums or so sometimes)

And the usefulness of this is questionable, when there's no indication of the grouping. I seem to have conductors starting with A-J, but nothing there after? And who are they anyway? There's no indication in the Default skin, is there?

No different to Artist sorts, the indication is the first name in line 2 of the album text.

don't know whether it's the admittedly slow rendering of the html, or the rendering on the client side, or the additional DB work. But do you believe this is usable?

Database performance is fine: a little extra when selecting a role sort, but it can still complete the queries in < 2 seconds on my old chromebook 4GB RAM, 25000 albums. The problem is the rendering of the '-' textkey section in Default Skin, as I mentioned above.

I must admit that I'm not 100% sold on its usefulness - not sure I really see the use case for it

We already allow sorting by Artist, and for non-Rock/Pop releases, other roles are equally, sometimes more, important/relevant to the user. So I think its worth me persevering.

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.

3 participants