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

Improve global search #2482

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

JaiZed
Copy link
Collaborator

@JaiZed JaiZed commented May 1, 2024

Used Levenshtein distance to sort by closest match.
Increased total limit to 30.
Increased dropdown height to show more results initially (and which can also be scrolled into view).
After upgrade to next Mantine major version, scrollbars will appear automatically (when that code is uncommented).

JaiZed added 2 commits May 1, 2024 16:55
Used Levenshtein distance to sort by closest match
Increased total limit to 30.
Increased dropdown height to show more results initially (and which can also be scrolled into view).
After upgrade to next Mantine major version, scrollbars will appear automatically (when that code is uncommented).
Co-authored-by: Anderson Shindy Oki <anderson.vs.oki@gmail.com>
@JaiZed
Copy link
Collaborator Author

JaiZed commented May 2, 2024

I was just going to wait and remove the comment next week when the Mantine upgrade was done.

@morpheus65535
Copy link
Owner

Levenshtein module isn't part of Bazarr and isn't part of base Python install. This wont work unless you add it to vendored libs. You want me to do it? I haven't created a script or a wiki on how to do this yet.

@JaiZed
Copy link
Collaborator Author

JaiZed commented May 2, 2024

Yes, please add it for me.

I just imported it without downloading anything and it worked, so I assumed it was built in.

@morpheus65535
Copy link
Owner

Levenshtein rely on cmake that need to be compiled on destination computer. We can't vendor this module. If it's explicitly required, we'll need to add it to requirements.txt and make sure it will be available or can be compiled on all major platforms.

Does it worth the effort?

@JaiZed
Copy link
Collaborator Author

JaiZed commented May 2, 2024

I think it's worth the effort because the sorted results are so much better.
However, I will look to see if there are any other options that don't require this module.

I'm wondering how it got into my Python environment. Perhaps a side effect on installing whisper or something else?

@JaiZed
Copy link
Collaborator Author

JaiZed commented May 2, 2024

I have found a Python library called textdistance that has the same functionality.
pip install textdistance

It seems to work the same and I think it is pure Python.
Is that acceptable?

@morpheus65535
Copy link
Owner

Is that acceptable?

Yes absolutely. I've added it to your PR.

JaiZed added 3 commits May 2, 2024 14:51
Also switch to Hamming algorithm as it appears to give similar or even arguably better results but executes more quickly than Levenshtein.
Comment on lines +88 to +91
limit={30}
// TODO: uncomment following line after upgrade to Mantine 7.x or higher
// scrollAreaProps = {{type: auto}}
maxDropdownHeight={400}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@JaiZed

I dare to say you don't need this after Mantine is updated to 7. I will leave you to check again without this once the mantine 7 is merged.

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.

None yet

3 participants