-
-
Notifications
You must be signed in to change notification settings - Fork 736
add fuzzy trigram searching to add-on store #19309
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds trigram-based fuzzy search functionality to the NVDA add-on store, improving search usability by allowing typos and partial matches. The search results are now filtered and ordered by relevance, with the ability to restore the previous sort order when the search is cleared.
Key changes:
- Implemented trigram-based similarity scoring for add-on search with a threshold of 0.3
- Added "Search relevance" as a sortable field that becomes the default when searching
- Automatically switches sort field to "Search relevance" when search is active and reverts to previous sort when cleared
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| source/gui/addonStoreGui/viewModels/addonList.py | Adds trigram search implementation, searchRank field, searchableText property, and logic to handle sort field transitions |
| source/gui/addonStoreGui/controls/storeDialog.py | Updates UI to clear selection on search, switch to search relevance sorting, and synchronize column filter display |
| source/gui/addonStoreGui/controls/addonList.py | Updates column click handling to use sortableFields instead of presentedFields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| :return: A frozenset of trigrams. | ||
| :raises ValueError: If the text is too short to generate trigrams. | ||
| """ | ||
| normalized = strxfrm(text.strip()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning: strxfrm is not a normalization function.
It just provides a string that can be used as a key for locale-aware comparisons. Did you really intend to use this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I believe it works as expected when performing trigram comparisons. e.g. ensuring an accented character matches the character itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> strxfrm("Sean")
'\x0e\x91\x0e!\x0e\x02\x0ep\x01\x01\x12\x01\x01'
>>> strxfrm("Séan")
'\x0e\x91\x0e!\x0e\x02\x0ep\x01\x02\x02\x0e\x01\x12\x01\x01'
Will trigram search work on these strings?
Also calling the result of strxfrm normalized is misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it will match by each char, in theory it should provide much better matching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's misleading. The conversion and stripping normalizes the text for the purpose of trigram searches
|
Here are points of feedback after my first tests:
|
Link to issue number:
None
Summary of the issue:
Add-on store list searching uses an exact search, i.e. it doesn't allow for typos or similar text to be found.
This also means that we cannot sort by search results relevance.
Instead a trigram search can be used to improve the search usability of the add-on store.
Description of user facing changes:
When searching add-ons, add-ons are filtered and ordered by search results relevance. The order can be updated after searching, and the previous order will restore if the search is cleared.
Description of developer facing changes:
None
Description of development approach:
searchRankfield toAddonListFieldand made it the default sorting method when a search is active. [1] [2] [3]sortableFieldsproperty, which includes the new "Search relevance" option, and ensured that sorting and column selection are synchronized with the current search/filter state. [1] [2] [3] [4] [5]Testing strategy:
Tested manually by filtering the add-on store
Known issues with pull request:
None
Code Review Checklist: