Fix pressing ESC in text fields (#11190)

* Fix pressing ESC in text fields - Reuse "SearchTextField" - Make ... usage consistent in "Search..." and "Filter groups..." - Let translation key contain the dots * Remove "Close" key binding for Entry Editor. Only "OPEN_CLOSE_ENTRY_EDITOR" (formerly known as "EDIT_ENTRY") is listend to * Add comment * Remove debug logging * Implement unfocus * Fix typo * Support CLEAR_SEARCH shortcut also at GlobalSearchBar * Extract methods * Fix CSS (and simplify GroupTreeView) * Pass "keyBindingRepositor" in constructor
JabRef · Apr 15, 2024 · bda81a4 · bda81a4
1 parent 0d5620e
commit bda81a4
Show file tree

Hide file tree

Showing 13 changed files with 145 additions and 66 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,6 +25,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
 ### Fixed
 
 - We fixed an issue where entry type with duplicate fields prevented opening existing libraries with custom entry types [#11127](https://github.com/JabRef/jabref/issues/11127)
+- We fixed the usage of the key binding for "Clear search" (default: <kbd>Escape</kbd>). [#10764](https://github.com/JabRef/jabref/issues/10764).
 
 ### Removed
 

diff --git a/src/main/java/org/jabref/gui/Base.css b/src/main/java/org/jabref/gui/Base.css
@@ -1048,24 +1048,27 @@ TextFlow > .tooltip-text-monospaced {
     -fx-icon-color: -jr-theme-text;
 }
 
-.mainToolbar .search-field {
+.search-field {
     -fx-background-color: -jr-search-background;
     -fx-border-width: 1;
     -fx-border-color: -jr-separator;
     -fx-border-radius: 2;
     -fx-fill: -jr-search-text;
 }
 
-.mainToolbar .search-field .button .glyph-icon {
+.search-field .button .glyph-icon {
     -fx-fill: -jr-search-text;
     -fx-text-fill: -jr-search-text;
     -fx-icon-color: -jr-search-text;
 }
 
-/* magnifier glass */
-.mainToolbar .search-field .glyph-icon {
-    -fx-fill: -jr-search-text;
-    -fx-text-fill: -jr-search-text;
+/*
+ * The magnifying glass icon left of the search text field.
+ * Currently, hits "Web search" and "Groups" only (not the searchbar on the top).
+*/
+.search-field-icon {
+    -fx-icon-color: -jr-search-text;
+    -fx-font-size: 1.7em;
 }
 
 /* search modifier buttons */

diff --git a/src/main/java/org/jabref/gui/actions/StandardActions.java b/src/main/java/org/jabref/gui/actions/StandardActions.java
@@ -121,7 +121,7 @@ public enum StandardActions implements Action {
     SHOW_PREFS(Localization.lang("Preferences"), IconTheme.JabRefIcons.PREFERENCES, KeyBinding.SHOW_PREFS),
     MANAGE_JOURNALS(Localization.lang("Manage journal abbreviations")),
     CUSTOMIZE_KEYBINDING(Localization.lang("Customize keyboard shortcuts"), IconTheme.JabRefIcons.KEY_BINDINGS),
-    EDIT_ENTRY(Localization.lang("Open entry editor"), IconTheme.JabRefIcons.EDIT_ENTRY, KeyBinding.EDIT_ENTRY),
+    EDIT_ENTRY(Localization.lang("Open entry editor"), IconTheme.JabRefIcons.EDIT_ENTRY, KeyBinding.OPEN_CLOSE_ENTRY_EDITOR),
     SHOW_PDF_VIEWER(Localization.lang("Open document viewer"), IconTheme.JabRefIcons.PDF_FILE),
     NEXT_PREVIEW_STYLE(Localization.lang("Next preview style"), KeyBinding.NEXT_PREVIEW_LAYOUT),
     PREVIOUS_PREVIEW_STYLE(Localization.lang("Previous preview style"), KeyBinding.PREVIOUS_PREVIEW_LAYOUT),
@@ -149,7 +149,7 @@ public enum StandardActions implements Action {
     CLEANUP_ENTRIES(Localization.lang("Cleanup entries"), IconTheme.JabRefIcons.CLEANUP_ENTRIES, KeyBinding.CLEANUP),
     SET_FILE_LINKS(Localization.lang("Automatically set file links"), KeyBinding.AUTOMATICALLY_LINK_FILES),
 
-    EDIT_FILE_LINK(Localization.lang("Edit"), IconTheme.JabRefIcons.EDIT, KeyBinding.EDIT_ENTRY),
+    EDIT_FILE_LINK(Localization.lang("Edit"), IconTheme.JabRefIcons.EDIT, KeyBinding.OPEN_CLOSE_ENTRY_EDITOR),
     DOWNLOAD_FILE(Localization.lang("Download file"), IconTheme.JabRefIcons.DOWNLOAD_FILE),
     REDOWNLOAD_FILE(Localization.lang("Redownload file"), IconTheme.JabRefIcons.DOWNLOAD_FILE),
     RENAME_FILE_TO_PATTERN(Localization.lang("Rename file to defined pattern"), IconTheme.JabRefIcons.AUTO_RENAME),

diff --git a/src/main/java/org/jabref/gui/entryeditor/EntryEditor.java b/src/main/java/org/jabref/gui/entryeditor/EntryEditor.java
@@ -206,7 +206,11 @@ private void setupKeyBindings() {
                         event.consume();
                         break;
                     case CLOSE:
-                    case EDIT_ENTRY:
+                        // We do not want to close the entry editor as such
+                        // We just want to unfocus the field
+                        tabbed.requestFocus();
+                        break;
+                    case OPEN_CLOSE_ENTRY_EDITOR:
                         close();
                         event.consume();
                         break;

diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeView.java b/src/main/java/org/jabref/gui/groups/GroupTreeView.java
@@ -49,6 +49,7 @@
 import org.jabref.gui.actions.ActionFactory;
 import org.jabref.gui.actions.SimpleCommand;
 import org.jabref.gui.actions.StandardActions;
+import org.jabref.gui.search.SearchTextField;
 import org.jabref.gui.util.BindingsHelper;
 import org.jabref.gui.util.ControlHelper;
 import org.jabref.gui.util.CustomLocalDragboard;
@@ -121,14 +122,9 @@ public GroupTreeView(TaskExecutor taskExecutor,
     }
 
     private void createNodes() {
-        searchField = new CustomTextField();
-
-        searchField.setPromptText(Localization.lang("Filter groups"));
-        searchField.setId("searchField");
-        HBox.setHgrow(searchField, Priority.ALWAYS);
-        HBox groupFilterBar = new HBox(searchField);
-        groupFilterBar.setId("groupFilterBar");
-        this.setTop(groupFilterBar);
+        searchField = SearchTextField.create(preferencesService.getKeyBindingRepository());
+        searchField.setPromptText(Localization.lang("Filter groups..."));
+        this.setTop(searchField);
 
         mainColumn = new TreeTableColumn<>();
         mainColumn.setId("mainColumn");

diff --git a/src/main/java/org/jabref/gui/importer/fetcher/WebSearchPaneView.java b/src/main/java/org/jabref/gui/importer/fetcher/WebSearchPaneView.java
@@ -49,26 +49,34 @@ private void initialize() {
         fetchers.itemsProperty().bind(viewModel.fetchersProperty());
         fetchers.valueProperty().bindBidirectional(viewModel.selectedFetcherProperty());
         fetchers.setMaxWidth(Double.POSITIVE_INFINITY);
-
-        // Create help button for currently selected fetcher
-        StackPane helpButtonContainer = new StackPane();
-        ActionFactory factory = new ActionFactory(preferences.getKeyBindingRepository());
-        EasyBind.subscribe(viewModel.selectedFetcherProperty(), fetcher -> {
-            if ((fetcher != null) && fetcher.getHelpPage().isPresent()) {
-                Button helpButton = factory.createIconButton(StandardActions.HELP, new HelpAction(fetcher.getHelpPage().get(), dialogService, preferences.getFilePreferences()));
-                helpButtonContainer.getChildren().setAll(helpButton);
-            } else {
-                helpButtonContainer.getChildren().clear();
-            }
-        });
-        HBox fetcherContainer = new HBox(fetchers, helpButtonContainer);
         HBox.setHgrow(fetchers, Priority.ALWAYS);
 
-        // Create text field for query input
-        TextField query = SearchTextField.create();
-        query.getStyleClass().add("searchBar");
+        StackPane helpButtonContainer = createHelpButtonContainer();
+        HBox fetcherContainer = new HBox(fetchers, helpButtonContainer);
+        TextField query = SearchTextField.create(preferences.getKeyBindingRepository());
+        getChildren().addAll(fetcherContainer, query, createSearchButton());
 
         viewModel.queryProperty().bind(query.textProperty());
+
+        addQueryValidationHints(query);
+
+        enableEnterToTriggerSearch(query);
+
+        ClipBoardManager.addX11Support(query);
+    }
+
+    /**
+     * Allows triggering search on pressing enter
+     */
+    private void enableEnterToTriggerSearch(TextField query) {
+        query.setOnKeyPressed(event -> {
+            if (event.getCode() == KeyCode.ENTER) {
+                viewModel.search();
+            }
+        });
+    }
+
+    private void addQueryValidationHints(TextField query) {
         EasyBind.subscribe(viewModel.queryValidationStatus().validProperty(),
                 valid -> {
                     if (!valid && viewModel.queryValidationStatus().getHighestMessage().isPresent()) {
@@ -79,23 +87,35 @@ private void initialize() {
                         query.pseudoClassStateChanged(QUERY_INVALID, false);
                     }
                 });
+    }
 
-        // Allows triggering search on pressing enter
-        query.setOnKeyPressed(event -> {
-            if (event.getCode() == KeyCode.ENTER) {
-                viewModel.search();
-            }
-        });
-
-        ClipBoardManager.addX11Support(query);
-
-        // Create button that triggers search
+    /**
+     * Create button that triggers search
+     */
+    private Button createSearchButton() {
         BooleanExpression importerEnabled = preferences.getImporterPreferences().importerEnabledProperty();
         Button search = new Button(Localization.lang("Search"));
         search.setDefaultButton(false);
         search.setOnAction(event -> viewModel.search());
         search.setMaxWidth(Double.MAX_VALUE);
         search.disableProperty().bind(importerEnabled.not());
-        getChildren().addAll(fetcherContainer, query, search);
+        return search;
+    }
+
+    /**
+     * Creatse help button for currently selected fetcher
+     */
+    private StackPane createHelpButtonContainer() {
+        StackPane helpButtonContainer = new StackPane();
+        ActionFactory factory = new ActionFactory(preferences.getKeyBindingRepository());
+        EasyBind.subscribe(viewModel.selectedFetcherProperty(), fetcher -> {
+            if ((fetcher != null) && fetcher.getHelpPage().isPresent()) {
+                Button helpButton = factory.createIconButton(StandardActions.HELP, new HelpAction(fetcher.getHelpPage().get(), dialogService, preferences.getFilePreferences()));
+                helpButtonContainer.getChildren().setAll(helpButton);
+            } else {
+                helpButtonContainer.getChildren().clear();
+            }
+        });
+        return helpButtonContainer;
     }
 }
diff --git a/src/main/java/org/jabref/gui/keyboard/KeyBinding.java b/src/main/java/org/jabref/gui/keyboard/KeyBinding.java
@@ -2,6 +2,9 @@
 
 import org.jabref.logic.l10n.Localization;
 
+/**
+ * @implNote Cannot be sorted alphabetically, as {@link KeyBindingRepository#getKeyCombination(KeyBinding)} iterates over the enum in order and returns the first match.
+ */
 public enum KeyBinding {
     EDITOR_DELETE("Delete", Localization.lang("Delete text"), "", KeyBindingCategory.EDITOR),
     // DELETE BACKWARDS = Rubout
@@ -46,7 +49,7 @@ public enum KeyBinding {
     DELETE_ENTRY("Delete entry", Localization.lang("Delete entry"), "DELETE", KeyBindingCategory.BIBTEX),
     DEFAULT_DIALOG_ACTION("Execute default action in dialog", Localization.lang("Execute default action in dialog"), "ctrl+ENTER", KeyBindingCategory.VIEW),
     DOWNLOAD_FULL_TEXT("Download full text documents", Localization.lang("Download full text documents"), "alt+F7", KeyBindingCategory.QUALITY),
-    EDIT_ENTRY("Open / close entry editor", Localization.lang("Open / close entry editor"), "ctrl+E", KeyBindingCategory.VIEW),
+    OPEN_CLOSE_ENTRY_EDITOR("Open / close entry editor", Localization.lang("Open / close entry editor"), "ctrl+E", KeyBindingCategory.VIEW),
     EXPORT("Export", Localization.lang("Export"), "ctrl+alt+e", KeyBindingCategory.FILE),
     EXPORT_SELECTED("Export Selected", Localization.lang("Export selected entries"), "ctrl+shift+e", KeyBindingCategory.FILE),
     EDIT_STRINGS("Edit strings", Localization.lang("Edit strings"), "ctrl+T", KeyBindingCategory.BIBTEX),
@@ -111,7 +114,7 @@ public enum KeyBinding {
     UNDO("Undo", Localization.lang("Undo"), "ctrl+Z", KeyBindingCategory.EDIT),
     WEB_SEARCH("Web search", Localization.lang("Web search"), "alt+4", KeyBindingCategory.SEARCH),
     WRITE_METADATA_TO_PDF("Write metadata to PDF files", Localization.lang("Write metadata to PDF files"), "F6", KeyBindingCategory.TOOLS),
-    CLEAR_SEARCH("Clear search", Localization.lang("Clear search"), "ESCAPE", KeyBindingCategory.SEARCH),
+    CLEAR_SEARCH("Clear search", Localization.lang("Clear search"), "Esc", KeyBindingCategory.SEARCH),
     CLEAR_READ_STATUS("Clear read status", Localization.lang("Clear read status"), "", KeyBindingCategory.EDIT),
     READ("Set read status to read", Localization.lang("Set read status to read"), "", KeyBindingCategory.EDIT),
     SKIMMED("Set read status to skimmed", Localization.lang("Set read status to skimmed"), "", KeyBindingCategory.EDIT);

diff --git a/src/main/java/org/jabref/gui/keyboard/KeyBindingRepository.java b/src/main/java/org/jabref/gui/keyboard/KeyBindingRepository.java
@@ -6,6 +6,7 @@
 import java.util.Comparator;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
 import java.util.stream.Collectors;
@@ -14,6 +15,7 @@
 import javafx.scene.input.KeyCombination;
 import javafx.scene.input.KeyEvent;
 
+import org.jabref.gui.Globals;
 import org.jabref.logic.util.OS;
 
 public class KeyBindingRepository {
@@ -110,6 +112,11 @@ public int size() {
         return this.bindings.size();
     }
 
+    /**
+     * Searches the key bindings for the given KeyEvent. Only the first matching key binding is returned.
+     * <p>
+     * If you need all matching key bindings, use {@link #mapToKeyBindings(KeyEvent)} instead.
+     */
     public Optional<KeyBinding> mapToKeyBinding(KeyEvent keyEvent) {
         for (KeyBinding binding : KeyBinding.values()) {
             if (checkKeyCombinationEquality(binding, keyEvent)) {
@@ -119,6 +126,28 @@ public Optional<KeyBinding> mapToKeyBinding(KeyEvent keyEvent) {
         return Optional.empty();
     }
 
+    /**
+     * Used if the same key could be used by multiple actions
+     */
+    private Set<KeyBinding> mapToKeyBindings(KeyEvent keyEvent) {
+        return Arrays.stream(KeyBinding.values())
+                     .filter(binding -> checkKeyCombinationEquality(binding, keyEvent))
+                     .collect(Collectors.toSet());
+    }
+
+    /**
+     * Checks if the given KeyEvent matches the given KeyBinding.
+     * <p>
+     * Used if a keyboard shortcut leads to multiple actions (e.g., ESC for closing a dialog and clearing the search).
+     */
+    public boolean matches(KeyEvent event, KeyBinding keyBinding) {
+        return Globals.getKeyPrefs().mapToKeyBindings(event)
+                      .stream()
+                      .filter(binding -> binding == keyBinding)
+                      .findFirst()
+                      .isPresent();
+    }
+
     public Optional<KeyCombination> getKeyCombination(KeyBinding bindName) {
         String binding = get(bindName.getConstant());
         if (binding.isEmpty()) {

diff --git a/src/main/java/org/jabref/gui/keyboard/TextInputKeyBindings.java b/src/main/java/org/jabref/gui/keyboard/TextInputKeyBindings.java
@@ -90,10 +90,7 @@ public static void call(Scene scene, KeyEvent event) {
                         focusedTextField.positionCaret(res.caretPosition);
                         event.consume();
                     }
-                    case CLOSE -> {
-                        focusedTextField.clear();
-                        event.consume();
-                    }
+                    // CLEAR_SEARCH is handled at org.jabref.gui.search.SearchTextField
                 }
             });
         }

diff --git a/src/main/java/org/jabref/gui/search/GlobalSearchBar.java b/src/main/java/org/jabref/gui/search/GlobalSearchBar.java
@@ -4,7 +4,6 @@
 import java.time.Duration;
 import java.util.List;
 import java.util.Objects;
-import java.util.Optional;
 import java.util.regex.Pattern;
 import java.util.regex.PatternSyntaxException;
 
@@ -88,7 +87,7 @@ public class GlobalSearchBar extends HBox {
     private static final PseudoClass CLASS_NO_RESULTS = PseudoClass.getPseudoClass("emptyResult");
     private static final PseudoClass CLASS_RESULTS_FOUND = PseudoClass.getPseudoClass("emptyResult");
 
-    private final CustomTextField searchField = SearchTextField.create();
+    private final CustomTextField searchField;
     private final ToggleButton caseSensitiveButton;
     private final ToggleButton regularExpressionButton;
     private final ToggleButton fulltextButton;
@@ -130,6 +129,9 @@ public GlobalSearchBar(LibraryTabContainer tabContainer,
             searchQueryProperty = stateManager.activeGlobalSearchQueryProperty();
         }
 
+        KeyBindingRepository keyBindingRepository = preferencesService.getKeyBindingRepository();
+
+        searchField = SearchTextField.create(keyBindingRepository);
         searchField.disableProperty().bind(needsDatabase(stateManager).not());
 
         // fits the standard "found x entries"-message thus hinders the searchbar to jump around while searching if the tabContainer width is too small
@@ -140,18 +142,14 @@ public GlobalSearchBar(LibraryTabContainer tabContainer,
         searchFieldTooltip.setMaxHeight(10);
         updateHintVisibility();
 
-        KeyBindingRepository keyBindingRepository = preferencesService.getKeyBindingRepository();
         searchField.addEventFilter(KeyEvent.KEY_PRESSED, event -> {
-            Optional<KeyBinding> keyBinding = keyBindingRepository.mapToKeyBinding(event);
-            if (keyBinding.isPresent()) {
-                if (keyBinding.get() == KeyBinding.CLOSE) {
-                    // Clear search and select first entry, if available
-                    searchField.setText("");
-                    if (searchType == SearchType.NORMAL_SEARCH) {
-                        tabContainer.getCurrentLibraryTab().getMainTable().getSelectionModel().selectFirst();
-                    }
-                    event.consume();
+            if (keyBindingRepository.matches(event, KeyBinding.CLEAR_SEARCH)) {
+                // Clear search and select first entry, if available
+                searchField.clear();
+                if (searchType == SearchType.NORMAL_SEARCH) {
+                    tabContainer.getCurrentLibraryTab().getMainTable().getSelectionModel().selectFirst();
                 }
+                event.consume();
             }
         });
 

diff --git a/src/main/java/org/jabref/gui/search/SearchTextField.java b/src/main/java/org/jabref/gui/search/SearchTextField.java
@@ -1,18 +1,39 @@
 package org.jabref.gui.search;
 
+import javafx.scene.Node;
+import javafx.scene.input.KeyEvent;
+
 import org.jabref.gui.icon.IconTheme;
+import org.jabref.gui.keyboard.KeyBinding;
+import org.jabref.gui.keyboard.KeyBindingRepository;
 import org.jabref.logic.l10n.Localization;
 
 import org.controlsfx.control.textfield.CustomTextField;
 import org.controlsfx.control.textfield.TextFields;
 
 public class SearchTextField {
 
-    public static CustomTextField create() {
+    public static CustomTextField create(KeyBindingRepository keyBindingRepository) {
         CustomTextField textField = (CustomTextField) TextFields.createClearableTextField();
-        textField.setPromptText(Localization.lang("Search") + "...");
-        textField.setLeft(IconTheme.JabRefIcons.SEARCH.getGraphicNode());
+        textField.setPromptText(Localization.lang("Search..."));
         textField.setId("searchField");
+        textField.getStyleClass().add("search-field");
+
+        Node graphicNode = IconTheme.JabRefIcons.SEARCH.getGraphicNode();
+        graphicNode.getStyleClass().add("search-field-icon");
+        textField.setLeft(graphicNode);
+
+        textField.addEventFilter(KeyEvent.KEY_PRESSED, event -> {
+            // Other key bindings are handled at org.jabref.gui.keyboard.TextInputKeyBindings
+            // We need to handle clear search here to have the code "more clean"
+            // Otherwise, we would have to add a new class for this and handle the case hitting that class in TextInputKeyBindings
+
+            if (keyBindingRepository.matches(event, KeyBinding.CLEAR_SEARCH)) {
+                       textField.clear();
+                       event.consume();
+            }
+        });
+
         return textField;
     }
 }