-
-
Notifications
You must be signed in to change notification settings - Fork 455
Add Acronym Support for Fuzzy Search #185
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
Conversation
|
I originally want to do it as the way I have done before, but then I change my mind and decide to write a straight-forward search for the acronyms and query, which will make retrieve metadata easier and support the case capital letter and space both exist. Because it is a straightforward loop, the performance should be good. |
|
I think the test case need update |
|
If this pull request has been approved, #183 should remove the part that create acronym before the pinyin. However, in order to achieve a correct matchdata highlight, IAlphabet may need to return an extra bool value suggesting whether the string is translated, since one Chinese character will project to multiple letter in pinyin form. |
|
I am not sure how we should implement the data match for translated data, since the length change in translation...... should we simply don't highlight data when the string has been translated? Or return an extra string for matching data, which make it the same length as the original string. |
…performance, but seems satisfying. It requires at most n times loop (n: number of translated charater) mapping once.
|
@jjw24 please take a try on the new mapping class, and it allow us to even highlight text when using full pinyin. However, it will take another loop to do the mapping every time, not sure if it will be possible to optimize it more. |
|
just need some clarification:
|
It matches the first letter of a word seperated from whitespace or upper letter. So vsc will match both "visual studio code" or "Visual Studio Code" or "visualStudioCode" or "VisualStudioCode". |
|
Just reworking scoring to be based on percentage of total acronyms so code doesn't bring up long strings. Will push up the branch for you to take a look |
|
Thank you! |
| if (char.IsUpper(stringToCompare[compareStringIndex]) || | ||
| char.IsNumber(stringToCompare[compareStringIndex]) || | ||
| char.IsWhiteSpace(stringToCompare[compareStringIndex]) || | ||
| spaceMet) |
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.
@taooceros what's this 'spaceMet' variable trying to solve and is this needed for translation?
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 think it is used to mark character after space to be acronym even when it is lower case.
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.
Ok cool, I thought so as well. I am thinking of using substring index = 0 to replace this, since the start of substring char is always acronym
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.
Have we split the string to compare? I think we only split the query string.
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.
Oh we didn't, lol ok nvm then
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.
Alternatively then, we can go back an index to check if it is space
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.
Yeah, that's feasible.
|
please look at the updates and let me know if good to merge in here: |
Great! I will take a look soon. |
Acronym Scoring Change
|
@jjw24 any further change we need to make for this PR? |
|
Let me take a final look in 2hrs |
jjw24
left a comment
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.
Great work @taooceros 👍
Uh oh!
There was an error while loading. Please reload this page.