Readd query syntax validation #7418

Fu188 · 2021-02-03T02:58:23Z

Readd the query syntax validation. Fixes #7411

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Siedlerchr · 2021-02-03T18:01:35Z

I tried your fix locally, it does not work, it still freezes when I paste text. Also you removed the key on enter pressed.

Fu188 · 2021-02-04T02:58:49Z

I tried your fix locally, it does not work, it still freezes when I paste text. Also you removed the key on enter pressed.

I suppose that this issue is just to readd query syntax. In this case, I just rollback the original version and did not pull request my solution. If needed, I will pull request my solution to deal with GUI freezes.

Fu188 · 2021-02-04T04:10:24Z

I tried your fix locally, it does not work, it still freezes when I paste text. Also you removed the key on enter pressed.

I have upload my solution to fix GUI freezes.

Siedlerchr · 2021-02-04T11:20:50Z

src/main/java/org/jabref/gui/importer/fetcher/WebSearchPaneViewModel.java

+    // Use Thread Pool to control the match time
+    private boolean isPatternMatched(Pattern pattern, String queryString) {
+        boolean isMatched;
+        Callable<Boolean> call = () -> {


You better use our BackGroundTask.wrap(...)

Done! Please check it!

Yep, works better now. No more freezing

Siedlerchr · 2021-02-05T18:42:27Z

The freezing is gone, but please readd the searching on enter. I'm just thinking if the code can be optimized a bit more. Maybe @tobiasdiez has an idea

CHANGELOG.md

Fu188 · 2021-02-06T01:57:26Z

The freezing is gone, but please readd the searching on enter. I'm just thinking if the code can be optimized a bit more. Maybe @tobiasdiez has an idea

I have readded the searching on enter. Please check it.

koppor · 2021-02-07T22:30:33Z

Note that we used Apache Lucene's query syntax. I am going to submit a pull request to the documentation to update it.

I think, a regular expression cannot cover Lucence's full power as lucense is a context-senstitive grammar IMHO.

I think, at the appropriate place, a try to parse the syntax by the selected fetcher could be tried. See here for an inspriation:

jabref/src/main/java/org/jabref/logic/importer/SearchBasedFetcher.java

Line 42 in 93ad499

return this.performSearch(parser.parse(searchQuery, NO_EXPLICIT_FIELD));

Thus, I am strongly against adding regex syntax checking here. I would ask for parsing the query for validation with the same method the query is parsed to query the online databases.

koppor

The code should use the "right" instance of AbstractQueryTransformer. It should enahnce these classes to provide proper feedback which part of the query is not valid.

koppor · 2021-02-07T23:01:51Z

Note that this PR in its current state just reverted the quick fix of #7391. Note the following text of that PR:

I am so sorry that I did not highlight this text in #7411. I also added the screenshot there.

koppor · 2021-02-08T06:05:57Z

The new explanation has been created as PR to the documentation: JabRef/user-documentation#348

tobiasdiez

I agree, we should reuse the query parser from lucene and catch the ParseException to construct the error message. Otherwise this looks good to me!

Thanks for reimplementing this. Next time though please ask the developers first if it is reasonable to spent time on this. I was also of the opinion that the PR that removed this feature was merged to quickly, but please respect the decision of the core developer team. We try to strongly listen to users and other contributors, and take their suggestions into account. But in the end we are the ones that have to have the bigger picture in mind and have to maintain everything in the long run. Right now you spent time reimplementing the feature, and we spent time reviewing it - and probably in two weeks @DominikVoigt will create a PR redesigning the whole search interface and making this PR obsolete. That's a bit unfortunate use of time.

Nonetheless, let's get this in and hope it will be useful for the new interface as well.

tobiasdiez · 2021-02-08T09:46:57Z

src/main/java/org/jabref/gui/importer/fetcher/WebSearchPaneViewModel.java

+        boolean isMatched = true;
+
+        try {
+            BackgroundTask.wrap(() -> pattern.matcher(queryString.strip()).matches())


This should only take a few milliseconds, so I wouldn't use a background task here.

In this case, it's actually function matches() takes a lot of time that make GUI freeze. When we input a long title, it will take about 2 mintues, which will make GUI freeze

oooh that's surprising. The query parser is quicker, so this shouldn't be a problem any longer once use it.

If you use the query parser instead of a regex, then it's good to go from my side.

I need to merge a certain number PR due to some personal reasons....

We have some quick win ideas at https://github.com/koppor/jabref/labels/good%20first%20issue. For instance
JabRef#286 should be a quick win.

Actually, I indeed spent a lot of time dealing with this bug.

We spend a lot time implementing the new query syntax and discussing how to unfreeze JabRef in certain cases. When it is possible to use the "query parser" as @tobiasdiez suggested, it should be OK. It is IMHO just about deleting the regex code etc. Then this PR will probably only be some lines of code.

koppor · 2021-02-08T20:19:42Z

Thanks for reimplementing this. Next time though please ask the developers first if it is reasonable to spent time on this. I was also of the opinion that the PR that removed this feature was merged to quickly, but please respect the decision of the core developer team. We try to strongly listen to users and other contributors, and take their suggestions into account.

It should also be noted that we never released a version without the query syntax check. We merged the PR to avoid JabRef hanging when an invalid search query was inserted

in two weeks @DominikVoigt will create a PR redesigning the whole search interface and making this PR obsolete.

Here, it should be noted, that the new search is already implemented. "Only" the feedback regarding the syntax is missing.

Nonetheless, let's get this in and hope it will be useful for the new interface as well.

The syntax parsed here is NOT the syntax, JabRef uses for searching. As far as I see, the regex misses AND and OR. I do not see why we should come up with a self-made regex to parse Apache Lucene.

In other words:

The set of valid expressions detected in this PR is not the same set as the set of expression accepted by JabRef in the search. When this PR is merged as is, the feedback to the user is wrong in many cases. (This is also the reason why we removed the validation. Besides hanging it was not completely right.)

Pleeeeeeeeeeeeeeeeeeeeease re-use the existing query parsing. I already pointed to code where it can be located from. When this is implemented, the set of validated queries is the same as JabRef takes as valid queries for searching.

For users, it would be even more useful if the position of the first error in the query syntax would be shown. This should also possible with the already included libraries.

calixtus · 2021-02-15T20:04:08Z

Devcall-decision: We decided to close this PR since, we haven' heard about any change here.
In fact, @DominikVoigt is already working on a different solution, using the transformators to provide feedback about parsing the query strings. So we would like to ask you to be patient for his work.

readd query syntax validation

fc67487

Siedlerchr added the status: changes required Pull requests that are not yet complete label Feb 3, 2021

readd query syntax validation

f9bb458

readd query syntax validation

cfaf66e

Siedlerchr reviewed Feb 4, 2021

View reviewed changes

Fu188 added 2 commits February 4, 2021 20:27

readd query syntax validation

270cda0

readd query syntax validation

8195b76

Fu188 requested a review from Siedlerchr February 4, 2021 12:29

calixtus reviewed Feb 5, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

readd query syntax validation

13e03da

Fu188 requested a review from calixtus February 6, 2021 01:57

Fu188 added 2 commits February 6, 2021 09:58

Merge branch 'master' into fix-for-issue-7411

a2d16d6

Update WebSearchPane.java

779a772

koppor requested a review from DominikVoigt February 7, 2021 21:48

koppor requested changes Feb 7, 2021

View reviewed changes

tobiasdiez requested changes Feb 8, 2021

View reviewed changes

calixtus closed this Feb 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Readd query syntax validation #7418

Readd query syntax validation #7418

Fu188 commented Feb 3, 2021 •

edited

Loading

Siedlerchr commented Feb 3, 2021

Fu188 commented Feb 4, 2021

Fu188 commented Feb 4, 2021

Siedlerchr Feb 4, 2021

Fu188 Feb 4, 2021

Siedlerchr Feb 5, 2021 •

edited

Loading

Siedlerchr commented Feb 5, 2021

Fu188 commented Feb 6, 2021

koppor commented Feb 7, 2021

koppor left a comment

koppor commented Feb 7, 2021

koppor commented Feb 8, 2021

tobiasdiez left a comment

tobiasdiez Feb 8, 2021

Fu188 Feb 8, 2021

tobiasdiez Feb 8, 2021

This comment was marked as off-topic.

tobiasdiez Feb 8, 2021

koppor Feb 8, 2021

koppor commented Feb 8, 2021

calixtus commented Feb 15, 2021

Readd query syntax validation #7418

Readd query syntax validation #7418

Conversation

Fu188 commented Feb 3, 2021 • edited Loading

Siedlerchr commented Feb 3, 2021

Fu188 commented Feb 4, 2021

Fu188 commented Feb 4, 2021

Siedlerchr Feb 4, 2021

Choose a reason for hiding this comment

Fu188 Feb 4, 2021

Choose a reason for hiding this comment

Siedlerchr Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Siedlerchr commented Feb 5, 2021

Fu188 commented Feb 6, 2021

koppor commented Feb 7, 2021

koppor left a comment

Choose a reason for hiding this comment

koppor commented Feb 7, 2021

koppor commented Feb 8, 2021

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez Feb 8, 2021

Choose a reason for hiding this comment

Fu188 Feb 8, 2021

Choose a reason for hiding this comment

tobiasdiez Feb 8, 2021

Choose a reason for hiding this comment

This comment was marked as off-topic.

tobiasdiez Feb 8, 2021

Choose a reason for hiding this comment

koppor Feb 8, 2021

Choose a reason for hiding this comment

koppor commented Feb 8, 2021

calixtus commented Feb 15, 2021

Fu188 commented Feb 3, 2021 •

edited

Loading

Siedlerchr Feb 5, 2021 •

edited

Loading