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

Increasing limit of returned results from searches #4213

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

bradbeattie
Copy link
Contributor

@bradbeattie bradbeattie commented Dec 16, 2022

Changes
In the context of jellyfin/jellyfin#8842, many more than 24 items might be relevant. This PR increases the number of returned results per category to accommodate this (e.g. searching for movies with the tag christmas).

Issues
While this isn't a complete solution to #1752 (which would likely require infinite horizontal scroll), it's a sufficient short-term measure to relieve the pressure some users might feel.

@bradbeattie bradbeattie force-pushed the patch-1 branch 4 times, most recently from fecdec0 to 8763d11 Compare December 18, 2022 22:03
@bradbeattie bradbeattie changed the title Upping search limit Increasing limit of returned results from searches Dec 18, 2022
@lecelmakerman
Copy link

lecelmakerman commented Dec 22, 2022

For non infinite scroll horizontal, I work whit this:
General - Custom CSS

#searchPage .itemsContainer, .vertical-list { display: flex; flex-wrap: wrap; }

@bradbeattie
Copy link
Contributor Author

Working around the PR approval workflow, I can get the effect of this with the following addition to my Dockerfile:

RUN sed -i 's/,Limit:24,/,Limit:240,/g' /jellyfin/jellyfin-web/search.*.chunk.js

@thornbill
Copy link
Member

This is not really a viable solution to the problem imho.

There can be 16 different search queries made when searching to search for different item types which at a worst case would mean that 16 x 240 (3,840) results would need to be rendered on the screen. That is just not feasible. Also I doubt anyone wants to horizontal scroll through 240 results.

What we really need is a "view more" option for each section that goes to a paginated results screen for only that item type.

@bradbeattie
Copy link
Contributor Author

Agreed that the worst case is near 4000 entries. Realistically though, I doubt anywhere around this number would ever be displayed. That said, I'm wholeheartedly on board with a paginated or infinite scroll approach. But given the lack of progress on #1752, I figured a quick workaround would be worthwhile.

240 might be overkill, but ~50-100 might still prove useful especially in the context of tag searching.

@bradbeattie bradbeattie force-pushed the patch-1 branch 2 times, most recently from 6a50c0a to e42ec61 Compare December 29, 2022 23:33
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@thornbill thornbill added the enhancement Improve existing functionality or small fixes label Jan 4, 2023
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Apr 27, 2023
@jellyfin-bot
Copy link
Collaborator

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@fhumayun
Copy link

Looking for heroes to resolve this merge conflict 😌

@bradbeattie bradbeattie requested a review from a team as a code owner October 2, 2023 15:54
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Oct 2, 2023
@bradbeattie
Copy link
Contributor Author

Merge conflict resolved. Adjusted the limit from 24 to 240 to 100 as an attempt to compromise on the blocking disagreement.

Copy link
Member

@thornbill thornbill left a comment

Choose a reason for hiding this comment

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

Really I think the search page should be rewritten... firing off so many requests is clearly not working well, so we should make a single request and handle separating them by type client side.

This seems like a fine compromise in the meantime though. 🙂

@bradbeattie
Copy link
Contributor Author

bradbeattie commented Oct 4, 2023

I'd argue that search should be pushed off to something like https://github.com/meilisearch/meilisearch to help address issues of typo correction, stemming, filtering, etc. But that's a much bigger lift than this change.

Additionally, it looks like the PR approval needs a push to move it through.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.3% 2.3% Duplication

@thornbill thornbill merged commit c8bed05 into jellyfin:master Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality or small fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants