Navigation Menu

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

All: Fix filters when language is in Korean #3947

Merged
merged 2 commits into from Oct 22, 2020
Merged

All: Fix filters when language is in Korean #3947

merged 2 commits into from Oct 22, 2020

Conversation

naviji
Copy link
Contributor

@naviji naviji commented Oct 18, 2020

We shouldn't use basic search when the filters like "tag" have a value in languages like Korean.

Otherwise, queries like these won't work.
tag:여보세요

Bug reported here: https://discourse.joplinapp.org/t/search-tag-in-asian-laguage-doesnt-work-properly/11285

@laurent22
Copy link
Owner

Do the filters support Asian languages? After this fix, what happens if someone searches for "tag:여보세요" ?

@naviji
Copy link
Contributor Author

naviji commented Oct 18, 2020

Do the filters support Asian languages?

Yes, because we are using the SQL LIKE operator to match the tag names. We can also put a wildcard anywhere in the string.

Before this fix, if we search for "tag:여보세요", we'll see the Korean text, change the search type to "basic" and then do a basic search for the empty string. This means all the notes will be returned.

(Empty string since after parsing the query, we'll get no actual "text" to search. But maybe this is also a bug, since if we're doing a basic search we should parse "tag:여보세요" as just "text". No reason to interpret it as a filter then.)

After this fix, we'll be checking for the presence of Asian language characters in text, title and body terms only. So the search becomes an FTS search and we get back all the notes with tag "여보세요" (as expected).

@JackGruber
Copy link
Contributor

@naviji
Would this also fix The search for tags with umlauts (like ä ö ü) does not work #3857 ?

@naviji
Copy link
Contributor Author

naviji commented Oct 18, 2020

Bug #3857 is caused because of a different reason. We were removing the diacritics from all the filter values before doing the search. This should be done only for the text, title, and body values.

I added a fix for that too in this PR. Queries like tag:zähler and tag:tag:сделать should all work now.

@tessus tessus added the search label Oct 18, 2020
try {
allTerms = filterParser(query);
} catch (error) {
console.warn(error);
Copy link
Owner

Choose a reason for hiding this comment

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

Should we proceed with the rest of the code if there was an error here? What kind of error be thrown in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, we could throw an error here and exit from this function. But there needs to be a catch statement higher up.

Errors would be invalid filters like -notebook:abc, type:blah , foo:bar etc.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes it seems that getting the error displayed somewhere in the UI wouldn't be that easy, so let's keep it as it is now. A warning is a good start and there's the doc to explain how the query should be formatted.

@laurent22 laurent22 merged commit b737ca7 into laurent22:dev Oct 22, 2020
@laurent22
Copy link
Owner

Thanks for the fix @naviji!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants