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

Fix pressing ESC in text fields #11190

Merged
merged 10 commits into from
Apr 15, 2024
Merged

Fix pressing ESC in text fields #11190

merged 10 commits into from
Apr 15, 2024

Conversation

koppor
Copy link
Member

@koppor koppor commented Apr 13, 2024

Fixes #10764

I somehow hang. Seems that the CLEAR_SEARCH is not found - even if configured.

Did not reset preferences again... Is it because Esc != Escape?

WARN: event: KeyEvent [source = CustomTextField[id=searchField, styleClass=text-input text-field custom-text-field clearable-field search-field], target = CustomTextField[id=searchField, styleClass=text-input text-field custom-text-field clearable-field search-field], eventType = KEY_PRESSED, consumed = false, character =  , text = , code = ESCAPE]
2024-04-13 14:54:09 [JavaFX Application Thread] org.jabref.gui.search.SearchTextField.lambda$create$2()
WARN: bindings: [CLOSE]

Just removing the case statement for the text field would be enough to fix the issue.

Background:

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

- Reuse "SearchTextField"
- Make ... usage consistent in "Search..." and "Filter groups..."
- Let translation key contain the dots
@koppor koppor added this to the 5.14 milestone Apr 13, 2024
@Siedlerchr
Copy link
Member

The key problem is the clash of Keybinding

@koppor
Copy link
Member Author

koppor commented Apr 13, 2024

The key problem is the clash of Keybinding

I added a method to handle that.

Did not want to re-use "close" (as it was done in 2022)

As user, I want Esc for both, clear search and close dialog.

OK, maybe, clear search is the same as close dialog, because the search is closed (somehow)? If yes, then I will also reuse the CLOSE keybinding

@Siedlerchr
Copy link
Member

Tested it without resetting my preferences on macOS:
Esc now closes the entry editor dialog.
When I use it in search field, it clears the text correctly

@Siedlerchr
Copy link
Member

with resetting the prefs same behavior. Esc closes the entry editor now

@calixtus
Copy link
Member

> Task :guiTest
org.jabref.gui.search.GlobalSearchBarTest

  Test recordingSearchQueriesOnFocusLostOnly(FxRobot) FAILED

  java.lang.NullPointerException: Cannot invoke "org.jabref.preferences.PreferencesService.getKeyBindingRepository()" because "org.jabref.gui.Globals.prefs" is null


GlobalSearchBarTest > recordingSearchQueriesOnFocusLostOnly(FxRobot) FAILED
    java.lang.NullPointerException: Cannot invoke "org.jabref.preferences.PreferencesService.getKeyBindingRepository()" because "org.jabref.gui.Globals.prefs" is null
        at org.testfx.util.WaitForAsyncUtils.---- Delayed Exception: (See Trace Below) ----(WaitForAsyncUtils.java:0)

@koppor
Copy link
Member Author

koppor commented Apr 14, 2024

with resetting the prefs same behavior. Esc closes the entry editor now

This is what should be expected, because Esc is for closing 😅.

I personally found it very annoying and would remove the functionality to close the entry editor via keyboard shortcut. Nobody missed it the last two years 😅

@Siedlerchr
Copy link
Member

Siedlerchr commented Apr 14, 2024

Closing the entry editor with ESC is worse than now. When I am typing into a field I just want to get ridd of the focus of the field, not
closing the whole entry editor

CTRL + E is both for opening and closing the entry editor. but only opening works..

Only "OPEN_CLOSE_ENTRY_EDITOR" (formerly known as "EDIT_ENTRY") is listend to
@koppor
Copy link
Member Author

koppor commented Apr 15, 2024

org.jabref.gui.search.GlobalSearchBarTest

Caused by Debugging Code.

@koppor
Copy link
Member Author

koppor commented Apr 15, 2024

Current state of the search icons:

I added the search icon also to the groups bar, because it felt consistent (place 3). The color of the icon is consistent to the web search (place 2). I could not style the icon for the top search bar (place 1), but this OK for me :)

image

Copy link
Contributor

github-actions bot commented Apr 15, 2024

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

@koppor koppor marked this pull request as ready for review April 15, 2024 11:04
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

search and entry editor work now as expected

@Siedlerchr Siedlerchr added this pull request to the merge queue Apr 15, 2024
Merged via the queue into main with commit bda81a4 Apr 15, 2024
20 checks passed
@Siedlerchr Siedlerchr deleted the fix-10764 branch April 15, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Workaround in comments] ESC removes information from entry editor
3 participants