Skip to content

Fix pressing ESC in text fields#11190

Merged
Siedlerchr merged 10 commits into
mainfrom
fix-10764
Apr 15, 2024
Merged

Fix pressing ESC in text fields#11190
Siedlerchr merged 10 commits into
mainfrom
fix-10764

Conversation

@koppor

@koppor koppor commented Apr 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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.

@koppor
Fix pressing ESC in text fields
88cb574 
- 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

Siedlerchr commented Apr 13, 2024

Copy link
Copy Markdown
Member

The key problem is the clash of Keybinding

@koppor

koppor commented Apr 13, 2024

Copy link
Copy Markdown
Member Author

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

Siedlerchr commented Apr 13, 2024

Copy link
Copy Markdown
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

Siedlerchr commented Apr 13, 2024

Copy link
Copy Markdown
Member

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

@calixtus

calixtus commented Apr 13, 2024

Copy link
Copy Markdown
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

koppor commented Apr 14, 2024

Copy link
Copy Markdown
Member Author

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

Siedlerchr commented Apr 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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..

koppor added 2 commits April 15, 2024 10:43
@koppor
Remove "Close" key binding for Entry Editor.
04676c6 
Only "OPEN_CLOSE_ENTRY_EDITOR" (formerly known as "EDIT_ENTRY") is listend to
@koppor
Add comment
82405d6
@koppor

koppor commented Apr 15, 2024

Copy link
Copy Markdown
Member Author

org.jabref.gui.search.GlobalSearchBarTest

Caused by Debugging Code.

@koppor

koppor commented Apr 15, 2024

Copy link
Copy Markdown
Member Author

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

@github-actions

github-actions Bot commented Apr 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
Siedlerchr
Siedlerchr approved these changes Apr 15, 2024
View reviewed changes

@Siedlerchr Siedlerchr left a comment

Copy link
Copy Markdown
Member

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
@Siedlerchr Siedlerchr deleted the fix-10764 branch April 15, 2024 20:32
This was referenced Apr 16, 2024
Closed
Closed
@Siedlerchr Siedlerchr mentioned this pull request Apr 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.14

Development

Successfully merging this pull request may close these issues.

[Workaround in comments] ESC removes information from entry editor

3 participants

@koppor @Siedlerchr @calixtus