Skip to content

Fix keybindings in entry editor#2971

Merged
tobiasdiez merged 3 commits into
masterfrom
keyevent
Jul 6, 2017
Merged

Fix keybindings in entry editor#2971
tobiasdiez merged 3 commits into
masterfrom
keyevent

Conversation

@Siedlerchr

@Siedlerchr Siedlerchr commented Jul 5, 2017

Copy link
Copy Markdown
Member

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

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

I have a few remarks.

//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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.


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

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.

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.


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

Siedlerchr commented Jul 7, 2017 via email

Copy link
Copy Markdown
Member Author

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