From ebcb7dc7adb59cd7357c73be05974745d7b0eca9 Mon Sep 17 00:00:00 2001 From: Dominik Voigt Date: Tue, 1 Sep 2020 12:39:40 +0200 Subject: [PATCH] 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 --- ...tract-query-syntax-for-query-conversion.md | 13 +++++ .../gui/importer/fetcher/WebSearchPane.css | 8 +++ .../gui/importer/fetcher/WebSearchPane.java | 5 +- .../fetcher/WebSearchPaneViewModel.java | 57 ++++++++++++++++++- .../jabref/logic/importer/QueryParser.java | 7 +-- .../logic/importer/SearchBasedFetcher.java | 5 +- .../importer/fetcher/ComplexSearchQuery.java | 13 +++++ src/main/resources/l10n/JabRef_en.properties | 6 +- 8 files changed, 101 insertions(+), 13 deletions(-) create mode 100644 src/main/java/org/jabref/gui/importer/fetcher/WebSearchPane.css 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 index cb6a8f214fa..f25f161c2ff 100644 --- 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 new file mode 100644 index 00000000000..2b255447024 --- /dev/null +++ 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 index 64b005c2f1b..2b9b493a84d 100644 --- 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 index 342d7e61fc3..a93c3da7ea9 100644 --- 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,6 +13,9 @@ 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; @@ -17,8 +23,10 @@ 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 getFetchers() { @@ -91,13 +105,50 @@ public void search() { SearchBasedFetcher activeFetcher = getSelectedFetcher(); - BackgroundTask task = BackgroundTask.wrap(() -> new ParserResult(activeFetcher.performSearch(getQuery().trim()))) - .withInitialMessage(Localization.lang("Processing %0", getQuery())); - + BackgroundTask task; + QueryParser queryParser = new QueryParser(); + Optional 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 index 6ec1be9d9f7..6fe611f442f 100644 --- 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 parseQueryStringIntoComplexQuery(String quer luceneQuery.visit(visitor); List 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 index c16c9001a4b..8aa8dcf04f1 100644 --- 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 performComplexSearch(ComplexSearchQuery complexSearchQuery) throws FetcherException { - // Default implementation behaves as performSearch using the default field phrases as query - List 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 index ed6a1c0a776..b7c5c3cbd88 100644 --- 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 defaultFieldPhrases = new ArrayList<>(); private List authors = new ArrayList<>(); diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index 15954e36a76..45c83fd6b13 100644 --- 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. +