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 delete entry keybindings bug #1815

Merged
merged 5 commits into from
Sep 21, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I encountered an unnecessary space so i deleted it

- Fixed entry table traversal with Tab (no column traversal thus no double jump)
- Fixed [#1757](https://github.com/JabRef/jabref/issues/1757): Crash after saving illegal argument in entry editor
- Fixed [#1663](https://github.com/JabRef/jabref/issues/1663): Better multi-monitor support
- Fixed [#1882](https://github.com/JabRef/jabref/issues/1882): Crash after saving illegal bibtexkey in entry editor
- Fixed field `location` containing only city is not exported correctly to MS-Office 2007 xml format
- Fixed [#1235](https://github.com/JabRef/jabref/issues/1235): Modified Key bindings do not work correctly
- Fixed field `key` field is not exported to MS-Office 2008 xml format
- Fixed [#1181](https://github.com/JabRef/jabref/issues/1181) and [#1504](https://github.com/JabRef/jabref/issues/1504): Improved "Normalize to BibTeX name format": Support separated names with commas and colons. Considered name affixes such as "Jr".
- Fixed download files failed silently when an invalid directory is selected
Expand Down
89 changes: 67 additions & 22 deletions src/main/java/net/sf/jabref/gui/keyboard/KeyBindingsListener.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

import java.awt.event.KeyAdapter;
import java.awt.event.KeyEvent;
import java.util.ArrayList;
import java.util.Locale;
import java.util.stream.Collectors;

/**
* respond to grabKey and display the key binding
Expand All @@ -16,51 +19,93 @@ public KeyBindingsListener(KeyBindingTable table) {

@Override
public void keyPressed(KeyEvent evt) {
// first check if anything is selected if not the return

boolean isFunctionKey = false;
boolean isEscapeKey = false;
boolean isDeleteKey = false;

// first check if anything is selected if not then return
final int selRow = table.getSelectedRow();
boolean isAnyRowSelected = selRow >= 0;
if (!isAnyRowSelected) {
return;
}

final String modifier = KeyEvent.getKeyModifiersText(evt.getModifiers());
final String modifier = getModifierText(evt);

// VALIDATE code and modifier
// all key bindings must have a modifier: ctrl alt etc
if ("".equals(modifier)) {
int kc = evt.getKeyCode();
boolean isFunctionKey = (kc >= KeyEvent.VK_F1) && (kc <= KeyEvent.VK_F12);
boolean isEscapeKey = kc == KeyEvent.VK_ESCAPE;
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
// need a modifier except for function, escape and delete keys
return;
}
}

final String code = KeyEvent.getKeyText(evt.getKeyCode());
// second key cannot be a modifiers
if ("Tab".equals(code)
|| "Backspace".equals(code)
|| "Enter".equals(code)
|| "Space".equals(code)
|| "Ctrl".equals(code)
|| "Shift".equals(code)
|| "Alt".equals(code)) {
int code = evt.getKeyCode();
String newKey;
//skip the event triggered only by a modifier, tab, backspace or enter because these normally have preset
// functionality if they alone are pressed
if (code == KeyEvent.VK_ALT ||
code == KeyEvent.VK_TAB ||
code == KeyEvent.VK_BACK_SPACE ||
code == KeyEvent.VK_ENTER ||
Copy link
Member

Choose a reason for hiding this comment

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

Could you comment why ENTER is treated as modifier? Also applies to "TAB" and "BACK_SPACE". Maybe the reason is that these keys should not be used at shortcuts at all? Maybe a new if statement should be introduced with an appropriate comment.

Copy link
Member

Choose a reason for hiding this comment

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

BUMP

code == KeyEvent.VK_SPACE ||
code == KeyEvent.VK_CONTROL ||
code == KeyEvent.VK_SHIFT ||
code == KeyEvent.VK_META) {
return;
}

// COMPUTE new key binding
String newKey;
if ("".equals(modifier)) {
newKey = code;
if (isFunctionKey) {
newKey = KeyEvent.getKeyText(code);
} else if (isEscapeKey) {
newKey = "ESCAPE";
} else if (isDeleteKey) {
newKey = "DELETE";
} else {
return;
}
} else {
newKey = modifier.toLowerCase().replace("+", " ") + " " + code;
newKey = modifier.toLowerCase(Locale.ENGLISH) + " " + KeyEvent.getKeyText(code);
}

// SHOW new key binding
//SHOW new key binding
//find which key is selected and set its value
table.setValueAt(newKey, selRow, 1);
table.revalidate();
table.repaint();
}

/**
* Collects th English translations of all modifiers and returns them separated by a space
*
* @param evt the KeyEvent that was triggered to set the KeyBindings
* @return a String containing the modifier keys text
*/
private String getModifierText(KeyEvent evt) {
ArrayList<String> modifiersList = new ArrayList<>();

if (evt.isControlDown()) {
modifiersList.add("ctrl");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing the meta key, which is especially for mac users an important modifier key. The default shortcuts of JabRef also include this modifier, so please add it.

if (evt.isAltDown()) {
modifiersList.add("alt");
}
if (evt.isShiftDown()) {
modifiersList.add("shift");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be replaced by one line by using modifiersList.stream.collect

if (evt.isAltGraphDown()) {
modifiersList.add("alt gr");
Copy link
Member

Choose a reason for hiding this comment

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

Is that true? altgr without space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On linux 'alt gr' works just fine. On my windows machine at home, 'ctrl alt' is set instead of 'alt gr' but no matter if it's written with or without space in between. I tested it on the master and 'alt gr' is always parsed as 'ctrl alt' so it works as intended I guess.

}
if (evt.isMetaDown()) {
modifiersList.add("meta");
}
//Now build the String with all the modifier texts
String modifiersAsString = modifiersList.stream().collect(Collectors.joining(" "));
return modifiersAsString;
}
}