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 keybindings in entry editor #2971

Merged
merged 3 commits into from
Jul 6, 2017
Merged

Fix keybindings in entry editor #2971

merged 3 commits into from
Jul 6, 2017

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Jul 5, 2017

Awt key events are now converted to javafx key event and some events are filtered out, like copy, paste, cut, close editor, the rest is now propagated back to the jframe

Fix for #2949

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

Try to match awt key events with keycombinations
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jul 5, 2017
Remove + signs to make swing work again
Add + sign only in binding checker
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

I have a few remarks.

@@ -179,7 +181,29 @@ public void keyReleased(java.awt.event.KeyEvent e) {
public void keyPressed(java.awt.event.KeyEvent e) {
//We need to consume this event here to prevent the propgation of keybinding events back to the JFrame

e.consume();
Optional<KeyCode> keyCode = Arrays.stream(KeyCode.values()).filter(k -> k.getName().equals(e.getKeyText(e.getKeyCode()))).findFirst();
Copy link
Member

Choose a reason for hiding this comment

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

Can you please encapsulate this code in a Optional<KeyBinding> KeyPrefs#mapToKeyBinding method that accepts a awt.KeyEvent.

case CLOSE_ENTRY_EDITOR:
case DELETE_ENTRY:
case SELECT_ALL:
e.consume();
Copy link
Member

Choose a reason for hiding this comment

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

Do we really have to handle these events on the swing side? Does it not suffice to consume them at

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, unfortunately. Although the event is consumed it is still catched by the swing side and executed.
I have found no way to stop this.

Copy link
Member

Choose a reason for hiding this comment

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

Good to know. In my opinion this is a bug in the FXPanel.

@@ -142,6 +142,7 @@ public KeyStroke getKey(KeyBinding bindName) {

private KeyCombination getKeyCombination(KeyBinding bindName) {
String binding = get(bindName.getConstant());
binding = binding.replaceAll(" ", "+"); //javafdx expects plus signs between modifiers, swing not
Copy link
Member

Choose a reason for hiding this comment

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

I would actually prefer it the other way around: store it in JavaFX format and replace "+" -> " " for swing since we want to completely switch to FX at some point.

@@ -168,6 +174,16 @@ public void testSetSingleKeyBindingToDefault() {
assertFalse(keyBindingRepository.checkKeyCombinationEquality(KeyBinding.ABBREVIATE, shortcutKeyEvent));
}

@Test
public void testConversionAwtKeyEventJavafxKeyEvent() {
java.awt.event.KeyEvent evt = new java.awt.event.KeyEvent(mock(JFrame.class), 0, 0, InputEvent.CTRL_MASK, java.awt.event.KeyEvent.VK_S, java.awt.event.KeyEvent.CHAR_UNDEFINED);
Copy link
Member

Choose a reason for hiding this comment

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

Reuse the method I suggested to introduce above.

Adapt test
Add + between keybindings
@Siedlerchr
Copy link
Member Author

I readded the plus signs between. To make it work in entry editor and swing the keybindings have to be reset once/or just set new

@Siedlerchr Siedlerchr closed this Jul 6, 2017
@Siedlerchr Siedlerchr reopened this Jul 6, 2017
@tobiasdiez tobiasdiez merged commit edd3f7c into master Jul 6, 2017
@tobiasdiez tobiasdiez deleted the keyevent branch July 6, 2017 14:23
@matthiasgeiger
Copy link
Member

To make it work in entry editor and swing the keybindings have to be reset once/or just set new

Would it be possible to automate this as a preferences migration?

@Siedlerchr
Copy link
Member Author

Siedlerchr commented Jul 7, 2017 via email

Siedlerchr added a commit that referenced this pull request Jul 10, 2017
* upstream/master:
  Fix Brazilian Portugese language loading (#2981)
  Use sftp's symlink command to provide symlink to latest version
  Update gradle from 4.0 to 4.0.1
  Fix group storage (#2978)
  Fix keybindings in entry editor (#2971)
Siedlerchr added a commit that referenced this pull request Jul 11, 2017
* upstream/master:
  Eclipse J
  Add switch indentation for Eclipse and add some new missing formatting options
  Check for different editions in the duplicate check (#2991)
  Add CheckStyle Check for Constants (final static) (#2992)
  Add Remove link context menu entry in file editor (#2972)
  Fix DiVA tests
  Remove <pre> tag from entries fetched using MathSciNet (#2990)
  Fix Brazilian Portugese language loading (#2981)
  Use sftp's symlink command to provide symlink to latest version
  Update gradle from 4.0 to 4.0.1
  Fix group storage (#2978)
  Fix keybindings in entry editor (#2971)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants