Fix delete entry keybindings bug #1815

Braunch · 2016-08-22T13:48:53Z

When using a different localization then English, setting the keybindings does not work correctly (see here #1235 ). I figured out that the problem is that the KeyEvent returns localized key texts so you can not use this to set the KeyBindings(the JabRef KeyBindings use English key texts). At the moment I only see two possibilities that do not end up in comparing tons of Strings to manually get the english localization.

~~First would be to change the locale to English, so that the KeyEvent returns english key texts, but I don't know what I might break with that and if the rest of the localization will still work.~~

Second solution would be to change the KeyBindings so that they use KeyCodes instead of Strings. This would IMHO improve the KeyBindings system but it would mean a lot of changes in all the KeyBinding related code.
See the comment below for my approach #1815 (comment).

Manually tested changed features in running JabRef

koppor · 2016-08-23T16:04:16Z

src/test/resources/testbib/testjabref_292.bib

@@ -299,3 +299,4 @@ @siumed.edu
  url = {http://dx.doi.org/10.1016/j.jneumeth.2008.01.009}
 }

+@Comment{jabref-meta: databaseType:bibtex;}


This is JabRef 3.x - do not change bib files created with old versions of JabRef

Braunch · 2016-08-25T10:44:37Z

I now found a way to fix it. I am building a String with the English modifier key text using the triggered KeyEvents is-ModifierKey-Down methods

boceckts · 2016-08-26T12:22:17Z

CHANGELOG.md

@@ -77,6 +77,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
 - Fixed a number of issues related to accessing the GUI from outside the EDT
 - Added a few missing translation strings
 - Fixed NullPointerException when opening Customize entry type dialog without an open database
+- Fixed localization error when changing key bindings while not using the english localization


Instead of "Fixed localization error when changing key bindings ..." I would rather write "Fixed modified keybindings not working ..."

boceckts · 2016-08-26T13:13:59Z

Please resolve conflicts and minor comments, then it's good to me 👍

Braunch · 2016-08-29T15:29:54Z

@boceckts can you test it on win and mac for me please? The meta key does not work on my linux system since there are OS functions on it.

boceckts · 2016-08-30T15:02:41Z

@Braunch Windows uses the windows key for its own shortcuts, same as with some linux distributions. On mac it now does work to make shortcuts directly with the command(meta) key....but I just realized that if you create a shortcut with the control key, e.g. "ctrl + S" it will automatically be replaced by "command(meta) + S".
So I guess the meta key doesn't have to be listened to..sry for that.

Braunch · 2016-08-31T10:06:08Z

Is this intended? Why should the grabbed keys differ from the keys actually used by the user? I changed the behavior so that you can set the 'meta + key' shortcut with the meta.

Braunch · 2016-09-03T14:59:57Z

Any opinion on my solution with the meta key from the @JabRef/developers ?

koppor · 2016-09-06T20:03:00Z

src/main/java/net/sf/jabref/gui/keyboard/KeyBindingsListener.java


 /**
 * respond to grabKey and display the key binding
 */
 public class KeyBindingsListener extends KeyAdapter {

    private final KeyBindingTable table;
+    private boolean isDeleteKey;


You don't need to make them class variables, do you?

Still unanswered. 😇 - maybe these variables can now just be deleted?

koppor · 2016-09-11T10:27:47Z

src/main/java/net/sf/jabref/gui/keyboard/KeyBindingsListener.java

-            boolean isDeleteKey = kc == KeyEvent.VK_DELETE;
+            isFunctionKey = (kc >= KeyEvent.VK_F1) && (kc <= KeyEvent.VK_F12);
+            isEscapeKey = kc == KeyEvent.VK_ESCAPE;
+            isDeleteKey = kc == KeyEvent.VK_DELETE;
            if (!(isFunctionKey || isEscapeKey || isDeleteKey)) {
                return; // need a modifier except for function, escape and delete keys


Move comment form after the return to the line above

koppor · 2016-09-11T10:39:55Z

Please merge upstream/mater and fix minor comments. Then, it should be good to go.

Braunch · 2016-09-12T10:09:57Z

I addressed the small comments and resolved the conflicts.

koppor · 2016-09-12T21:27:47Z

You commited with both your st university address and your normal development address. Please double check your git config.

Please merge upstream/master again. Can you make one commit as described at https://github.com/JabRef/jabref/wiki/Tools#rebase-everything-as-one-commit-on-master? Then, I can resolve the conflicts by myself.

Braunch · 2016-09-13T09:48:17Z

I resolved the conflicts and squashed my commits. Should be good to go.

koppor · 2016-09-14T11:41:33Z

Please do not squash, this makes reviewing very hard as we cannot see, what has been adressed. I added a BUMP on all my unaswered comments. Please fix/answer.

You can see all comments at https://github.com/JabRef/jabref/pull/1815/files

…bindingsBug

Braunch · 2016-09-15T09:50:06Z

I addressed your comments and merged the upstream/master. I squashed following your comment #1815 (comment) Did I get that wrong?

Braunch · 2016-09-15T09:57:04Z

CHANGELOG.md

@@ -45,12 +45,13 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
 - Selecting an entry in the search result Window will now select the correct entry in the bib file
 - Entries in the SearchResultDialog are now converted to Unicode
 - Suggestions in the autocomplete (search) are now in Unicode
- Fixed NullPointerException when opening search result window for an untitled database 
+- Fixed NullPointerException when opening search result window for an untitled database


I encountered an unnecessary space so i deleted it

…bindingsBug

* Fix delete entry keybindings bug * Fix comments and unnecessary class variables

Braunch added bug Confirmed bugs or reports that are very likely to be bugs stupro labels Aug 22, 2016

koppor reviewed Aug 23, 2016
View reviewed changes

koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 24, 2016

koppor added this to the v3.6 milestone Aug 24, 2016

Braunch added stupro-ready-for-internal-review and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Aug 25, 2016

Braunch force-pushed the FixDeleteEntryKeybindingsBug branch from 17cc8aa to 86dbd96 Compare August 25, 2016 10:42

koppor modified the milestones: v3.7, v3.6 Aug 25, 2016

boceckts reviewed Aug 26, 2016
View reviewed changes

boceckts removed the stupro-ready-for-internal-review label Aug 26, 2016

Braunch force-pushed the FixDeleteEntryKeybindingsBug branch from 13545a8 to dfc898c Compare August 29, 2016 10:45

Braunch added the stupro-ready-for-internal-review label Aug 29, 2016

boceckts removed the stupro-ready-for-internal-review label Aug 30, 2016

Braunch force-pushed the FixDeleteEntryKeybindingsBug branch from dfc898c to 40bc30a Compare August 31, 2016 09:56

boceckts added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 31, 2016

Braunch force-pushed the FixDeleteEntryKeybindingsBug branch 2 times, most recently from 5717b1e to 285f1ba Compare September 1, 2016 11:15

boceckts changed the title ~~[WIP] Fix delete entry keybindings bug~~ Fix delete entry keybindings bug Sep 1, 2016

Braunch force-pushed the FixDeleteEntryKeybindingsBug branch 2 times, most recently from 5c01a6d to fc2b753 Compare September 6, 2016 10:12

koppor reviewed Sep 6, 2016
View reviewed changes

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 7, 2016

koppor reviewed Sep 11, 2016
View reviewed changes

Braunch removed stupro status: help wanted labels Sep 11, 2016

Braunch force-pushed the FixDeleteEntryKeybindingsBug branch from 39cbeb4 to 005d18c Compare September 12, 2016 08:15

Braunch added the stupro label Sep 12, 2016

Fix delete entry keybindings bug

b646a53

Braunch force-pushed the FixDeleteEntryKeybindingsBug branch from 005d18c to b646a53 Compare September 13, 2016 09:46

Braunch added 2 commits September 15, 2016 11:45

Fix comments and unnecessary class variables

0de49e2

Merge remote-tracking branch 'upstream/master' into FixDeleteEntryKey…

1299685

…bindingsBug

Braunch commented Sep 15, 2016

View reviewed changes

Merge remote-tracking branch 'upstream/master' into FixDeleteEntryKey…

54e709a

…bindingsBug

boceckts added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 19, 2016

Merge remote-tracking branch 'upstream/master' into FixDeleteEntryKey…

0e1a1f1

…bindingsBug

Siedlerchr merged commit 6c50dcf into JabRef:master Sep 21, 2016

Braunch mentioned this pull request Sep 21, 2016

Modified Key bindings do not work correctly #1235

Closed

zesaro pushed a commit to zesaro/jabref that referenced this pull request Nov 22, 2016

Fix delete entry keybindings bug (JabRef#1815)

3867d4f

* Fix delete entry keybindings bug * Fix comments and unnecessary class variables

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix delete entry keybindings bug #1815

Fix delete entry keybindings bug #1815

Braunch commented Aug 22, 2016 •

edited

Loading

koppor Aug 23, 2016

Braunch commented Aug 25, 2016

boceckts Aug 26, 2016

boceckts commented Aug 26, 2016

Braunch commented Aug 29, 2016

boceckts commented Aug 30, 2016

Braunch commented Aug 31, 2016 •

edited

Loading

Braunch commented Sep 3, 2016 •

edited

Loading

koppor Sep 6, 2016

koppor Sep 11, 2016 •

edited

Loading

koppor Sep 14, 2016

koppor Sep 11, 2016

koppor Sep 14, 2016

koppor commented Sep 11, 2016

Braunch commented Sep 12, 2016

koppor commented Sep 12, 2016

Braunch commented Sep 13, 2016

koppor commented Sep 14, 2016 •

edited

Loading

Braunch commented Sep 15, 2016

Braunch Sep 15, 2016

Fix delete entry keybindings bug #1815

Fix delete entry keybindings bug #1815

Conversation

Braunch commented Aug 22, 2016 • edited Loading

koppor Aug 23, 2016

Choose a reason for hiding this comment

Braunch commented Aug 25, 2016

boceckts Aug 26, 2016

Choose a reason for hiding this comment

boceckts commented Aug 26, 2016

Braunch commented Aug 29, 2016

boceckts commented Aug 30, 2016

Braunch commented Aug 31, 2016 • edited Loading

Braunch commented Sep 3, 2016 • edited Loading

koppor Sep 6, 2016

Choose a reason for hiding this comment

koppor Sep 11, 2016 • edited Loading

Choose a reason for hiding this comment

koppor Sep 14, 2016

Choose a reason for hiding this comment

koppor Sep 11, 2016

Choose a reason for hiding this comment

koppor Sep 14, 2016

Choose a reason for hiding this comment

koppor commented Sep 11, 2016

Braunch commented Sep 12, 2016

koppor commented Sep 12, 2016

Braunch commented Sep 13, 2016

koppor commented Sep 14, 2016 • edited Loading

Braunch commented Sep 15, 2016

Braunch Sep 15, 2016

Choose a reason for hiding this comment

Braunch commented Aug 22, 2016 •

edited

Loading

Braunch commented Aug 31, 2016 •

edited

Loading

Braunch commented Sep 3, 2016 •

edited

Loading

koppor Sep 11, 2016 •

edited

Loading

koppor commented Sep 14, 2016 •

edited

Loading