Add query syntax validity feedback to UI

Make QueryParser handle query parsing issues more robust Make default implementation of performComplexSearch include all fields Signed-off-by: Dominik Voigt <[email protected]>
JabRef · Sep 1, 2020 · ebcb7dc · ebcb7dc
1 parent bf60475
commit ebcb7dc
Show file tree

Hide file tree

Showing 8 changed files with 101 additions and 13 deletions.
diff --git a/docs/adr/0015-support-an-abstract-query-syntax-for-query-conversion.md b/docs/adr/0015-support-an-abstract-query-syntax-for-query-conversion.md
@@ -27,10 +27,23 @@ For simplicitly, and lack of universal capabilities across fetchers, only basic
   * `year` (for single year)
   * `year-range` (for range e.g. `year-range:2012-2015`)
 * The `journal`, `year`, and `year-range` fields should only be populated once in each query
+* The `year` and `year-range` fields are mutually exclusive
 * Example:
   * `author:"Igor Steinmacher" author:"Christoph Treude" year:2017` will be converted to
   * `author:"Igor Steinmacher" AND author:"Christoph Treude" AND year:2017`
 
+The supported syntax can be expressed in EBNF as follows:
+
+Query := {Clause} \
+Clause:= \[Field\] Term \
+Field := author: | title: | journal: | year: | year-range: | default:\
+Term  := Word | Phrase \
+
+Word can be derived to any series of non-whitespace characters.
+Phrases are multiple words wrapped in quotes and may contain white-space characters within the quotes.\
+Note: Even though this EBNF syntactically allows the creation of queries with year and year-range fields,
+such a query does not make sense semantically and therefore will not be executed.
+
 ### Positive Consequences
 
 * Already tested

diff --git a/src/main/java/org/jabref/gui/importer/fetcher/WebSearchPane.css b/src/main/java/org/jabref/gui/importer/fetcher/WebSearchPane.css
@@ -0,0 +1,8 @@
+.searchBar:valid {
+    -fx-background-color: rgba(50, 205, 50, 0.5)
+}
+
+.searchBar:invalid {
+    -fx-background-color: rgba(240, 128, 128, 0.5);
+}
+
diff --git a/src/main/java/org/jabref/gui/importer/fetcher/WebSearchPane.java b/src/main/java/org/jabref/gui/importer/fetcher/WebSearchPane.java
@@ -70,7 +70,9 @@ protected Node createContentPane() {
 
         // Create text field for query input
         TextField query = SearchTextField.create();
-        query.setOnAction(event -> viewModel.search());
+        query.getStyleClass().add("searchBar");
+        query.textProperty().addListener((observable, oldValue, newValue) -> viewModel.validateQueryStringAndGiveColorFeedback(query, newValue));
+
         viewModel.queryProperty().bind(query.textProperty());
 
         // Create button that triggers search
@@ -80,6 +82,7 @@ protected Node createContentPane() {
 
         // Put everything together
         VBox container = new VBox();
+        container.getStylesheets().add(WebSearchPane.class.getResource("WebSearchPane.css").toExternalForm());
         container.setAlignment(Pos.CENTER);
         container.getChildren().addAll(fetcherContainer, query, search);
         return container;

diff --git a/src/main/java/org/jabref/gui/importer/fetcher/WebSearchPaneViewModel.java b/src/main/java/org/jabref/gui/importer/fetcher/WebSearchPaneViewModel.java
@@ -1,6 +1,9 @@
 package org.jabref.gui.importer.fetcher;
 
+import java.util.Optional;
 import java.util.SortedSet;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import javafx.beans.property.ListProperty;
 import javafx.beans.property.ObjectProperty;
@@ -10,15 +13,20 @@
 import javafx.beans.property.StringProperty;
 import javafx.collections.FXCollections;
 import javafx.collections.ObservableList;
+import javafx.css.PseudoClass;
+import javafx.scene.control.TextField;
+import javafx.scene.control.Tooltip;
 
 import org.jabref.gui.DialogService;
 import org.jabref.gui.JabRefFrame;
 import org.jabref.gui.importer.ImportEntriesDialog;
 import org.jabref.gui.util.BackgroundTask;
 import org.jabref.logic.importer.ImportFormatPreferences;
 import org.jabref.logic.importer.ParserResult;
+import org.jabref.logic.importer.QueryParser;
 import org.jabref.logic.importer.SearchBasedFetcher;
 import org.jabref.logic.importer.WebFetchers;
+import org.jabref.logic.importer.fetcher.ComplexSearchQuery;
 import org.jabref.logic.l10n.Localization;
 import org.jabref.model.strings.StringUtil;
 import org.jabref.preferences.JabRefPreferences;
@@ -32,6 +40,7 @@ public class WebSearchPaneViewModel {
     private final StringProperty query = new SimpleStringProperty();
     private final JabRefFrame frame;
     private final DialogService dialogService;
+    private final Pattern queryPattern;
 
     public WebSearchPaneViewModel(ImportFormatPreferences importPreferences, JabRefFrame frame, JabRefPreferences preferences, DialogService dialogService) {
         // TODO: Rework so that we don't rely on JabRefFrame and not the complete preferences
@@ -52,6 +61,11 @@ public WebSearchPaneViewModel(ImportFormatPreferences importPreferences, JabRefF
             int newIndex = fetchers.indexOf(newFetcher);
             preferences.putInt(JabRefPreferences.SELECTED_FETCHER_INDEX, newIndex);
         });
+
+        String allowedFields = "((author|journal|title|year|year-range):\\s?)?";
+        // Either a single word, or a phrase with quotes, or a year-range
+        String allowedTermText = "(((\\d{4}-\\d{4})|(\\w+)|(\"\\w+[^\"]*\"))\\s?)+";
+        queryPattern = Pattern.compile("^(" + allowedFields + allowedTermText + ")+$");
     }
 
     public ObservableList<SearchBasedFetcher> getFetchers() {
@@ -91,13 +105,50 @@ public void search() {
 
         SearchBasedFetcher activeFetcher = getSelectedFetcher();
 
-        BackgroundTask<ParserResult> task = BackgroundTask.wrap(() -> new ParserResult(activeFetcher.performSearch(getQuery().trim())))
-                                                          .withInitialMessage(Localization.lang("Processing %0", getQuery()));
-
+        BackgroundTask<ParserResult> task;
+        QueryParser queryParser = new QueryParser();
+        Optional<ComplexSearchQuery> generatedQuery = queryParser.parseQueryStringIntoComplexQuery(getQuery());
+        if (generatedQuery.isPresent()) {
+            task = BackgroundTask.wrap(() -> new ParserResult(activeFetcher.performComplexSearch(generatedQuery.get())))
+                                 .withInitialMessage(Localization.lang("Processing %0", getQuery()));
+        } else {
+            task = BackgroundTask.wrap(() -> new ParserResult(activeFetcher.performSearch(getQuery().trim())))
+                                 .withInitialMessage(Localization.lang("Processing %0", getQuery()));
+        }
         task.onFailure(dialogService::showErrorDialogAndWait);
 
         ImportEntriesDialog dialog = new ImportEntriesDialog(frame.getCurrentBasePanel().getBibDatabaseContext(), task);
         dialog.setTitle(activeFetcher.getName());
         dialog.showAndWait();
     }
+
+    public void validateQueryStringAndGiveColorFeedback(TextField querySource, String queryString) {
+        Matcher queryValidation = queryPattern.matcher(queryString.strip());
+        if (queryValidation.matches()) {
+            if (containsYearAndYearRange(queryString)) {
+                setPseudoClassToInvalid(querySource);
+                querySource.setTooltip(new Tooltip(Localization.lang("The query cannot contain a year and year-range field.")));
+            } else {
+                setPseudoClassToValid(querySource);
+                querySource.setTooltip(new Tooltip(Localization.lang("This search contains entries that match all specified terms.")));
+            }
+        } else {
+            setPseudoClassToInvalid(querySource);
+            querySource.setTooltip(new Tooltip(Localization.lang("This query uses an unsupported syntax.")));
+        }
+    }
+
+    private void setPseudoClassToValid(TextField querySource) {
+        querySource.pseudoClassStateChanged(PseudoClass.getPseudoClass("invalid"), false);
+        querySource.pseudoClassStateChanged(PseudoClass.getPseudoClass("valid"), true);
+    }
+
+    private void setPseudoClassToInvalid(TextField querySource) {
+        querySource.pseudoClassStateChanged(PseudoClass.getPseudoClass("invalid"), true);
+        querySource.pseudoClassStateChanged(PseudoClass.getPseudoClass("valid"), false);
+    }
+
+    private boolean containsYearAndYearRange(String queryString) {
+        return queryString.toLowerCase().contains("year:") && queryString.toLowerCase().contains("year-range:");
+    }
 }
diff --git a/src/main/java/org/jabref/logic/importer/QueryParser.java b/src/main/java/org/jabref/logic/importer/QueryParser.java
@@ -43,10 +43,9 @@ public Optional<ComplexSearchQuery> parseQueryStringIntoComplexQuery(String quer
             luceneQuery.visit(visitor);
 
             List<Term> sortedTerms = new ArrayList<>(terms);
-            sortedTerms.sort(Comparator.comparing(Term::text));
-            builder.terms(sortedTerms);
-            return Optional.of(builder.build());
-        } catch (QueryNodeException | IllegalStateException ex) {
+            sortedTerms.sort(Comparator.comparing(Term::text).reversed());
+            return Optional.of(ComplexSearchQuery.fromTerms(terms));
+        } catch (QueryNodeException | IllegalStateException | IllegalArgumentException ex) {
             return Optional.empty();
         }
     }

diff --git a/src/main/java/org/jabref/logic/importer/SearchBasedFetcher.java b/src/main/java/org/jabref/logic/importer/SearchBasedFetcher.java
@@ -26,8 +26,7 @@ public interface SearchBasedFetcher extends WebFetcher {
      * @return a list of {@link BibEntry}, which are matched by the query (may be empty)
      */
     default List<BibEntry> performComplexSearch(ComplexSearchQuery complexSearchQuery) throws FetcherException {
-        // Default implementation behaves as performSearch using the default field phrases as query
-        List<String> defaultPhrases = complexSearchQuery.getDefaultFieldPhrases();
-        return performSearch(String.join(" ", defaultPhrases));
+        // Default implementation behaves as perform search on all fields concatenated as query
+        return performSearch(complexSearchQuery.toString());
     }
 }
diff --git a/src/main/java/org/jabref/logic/importer/fetcher/ComplexSearchQuery.java b/src/main/java/org/jabref/logic/importer/fetcher/ComplexSearchQuery.java
@@ -124,6 +124,19 @@ public int hashCode() {
         return result;
     }
 
+    @Override
+    public String toString() {
+        StringBuilder stringRepresentation = new StringBuilder();
+        getSingleYear().ifPresent(singleYear -> stringRepresentation.append(singleYear).append(" "));
+        getFromYear().ifPresent(fromYear -> stringRepresentation.append(fromYear).append(" "));
+        getToYear().ifPresent(toYear -> stringRepresentation.append(toYear).append(" "));
+        getJournal().ifPresent(journal -> stringRepresentation.append(journal).append(" "));
+        stringRepresentation.append(String.join(" ", getTitlePhrases()))
+                            .append(String.join(" ", getDefaultFieldPhrases()))
+                            .append(String.join(" ", getAuthors()));
+        return stringRepresentation.toString();
+    }
+
     public static class ComplexSearchQueryBuilder {
         private List<String> defaultFieldPhrases = new ArrayList<>();
         private List<String> authors = new ArrayList<>();

diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties
@@ -2258,14 +2258,16 @@ Close\ others=Close others
 Reveal\ in\ file\ explorer=Reveal in file explorer
 
 (\ Note\:\ Press\ return\ to\ commit\ changes\ in\ the\ table\!\ )=( Note\: Press return to commit changes in the table\! )
-
 Reset=Reset
 Reset\ entry\ types\ and\ fields\ to\ defaults=Reset entry types and fields to defaults
 This\ will\ reset\ all\ entry\ types\ to\ their\ default\ values\ and\ remove\ all\ custom\ entry\ types=This will reset all entry types to their default values and remove all custom entry types
-
 Replace\ tabs\ with\ space=Replace tabs with space
 Replace\ tabs\ with\ space\ in\ the\ field\ content.=Replace tabs with space in the field content.
 Remove\ redundant\ spaces=Remove redundant spaces
 Replaces\ consecutive\ spaces\ with\ a\ single\ space\ in\ the\ field\ content.=Replaces consecutive spaces with a single space in the field content.
 Remove\ digits=Remove digits
 Removes\ digits.=Removes digits.
+The\ query\ cannot\ contain\ a\ year\ and\ year-range\ field.=The query cannot contain a year and year-range field.
+This\ query\ uses\ an\ unsupported\ syntax.=This query uses an unsupported syntax.
+This\ search\ contains\ entries\ that\ match\ all\ specified\ terms.=This search contains entries that match all specified terms.
+