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

Unclear how to use addKeyDown/Up/PressListener #4046

Closed
Artur- opened this issue May 2, 2018 · 5 comments
Closed

Unclear how to use addKeyDown/Up/PressListener #4046

Artur- opened this issue May 2, 2018 · 5 comments

Comments

@Artur-
Copy link
Member

Artur- commented May 2, 2018

KeyNotifier.addKeyDownListener(String key, ComponentEventListener<KeyDownEvent> listener, KeyModifier... modifiers) javadoc says

Adds a {@code keydown} listener to this component, conditional on key and modifiers.
key: the key to match

What remains unclear is:

  • Why is the key a String? What happens with addKeyDownListener("abc",...)?
  • How do I listen to enter? Debugging shows that I should use "Enter" as they key to match, which was quite unexpected. Especially as "enter" does not work. There is seemingly no way to know upfront if I should use esc, Esc, escape or Escape to match the escape key

I would have expected the key parameter to be a a character and 13 would listen to enter

@heruan
Copy link
Member

heruan commented May 2, 2018

As the author of the PR which introduced these notifiers, I can say that: key is a string since it reflects the event.key property.

keyCode is deprecated since e.g. 13 is not ENTER is all keyboard layouts; neither code suits well, since it is tied to the hardware keyboard, e.g. with a Russian layout Э will trigger event.code = 'Quote' but event.key = 'Э'.

Thus the choice to use key which represent the exact key the user intended; common constants to avoid typing keys manually are defined in Key.

I totally agree the JavaDocs should be more clear about this.

@knoobie
Copy link
Contributor

knoobie commented May 3, 2018

@heruan - what do you think about the following to allow easier discovery of "Key".

  • change Key to an enum
  • create FunctionalInterface KeyCodeSupplier
  • Key impoements KeyCodeSupplier
  • change all methods to use KeyCodeSupplier instead of String so that 80-90% of the use case is done with your common impl and if somebody has a special case he can easily do () -> "ö".

Dunno if this could be a way the Flow Team likes to handle stuff.

@heruan
Copy link
Member

heruan commented May 3, 2018

Wouldn’t this be just overhead? Instead of this:

  1. addKeyPressListener(Key.ENTER, ...)
  2. addKeyPressListener("ö", ...)

You’d have this:

  1. addKeyPressListener(Key.ENTER, ...)
  2. addKeyPressListener(() -> "ö", ...)

I don’t see any added value, am I missing something?

I really think it’s just a matter of JavaDocs here.

@Legioth
Copy link
Member

Legioth commented May 3, 2018

The benefit would be discoverability, i.e. that your IDE will offer better auto complete suggestions.

As an alternative to an enum, I would suggest an interface with constants and a static of(String) factory method for other cases. The code would thus look like this instead:

  1. addKeyPressListener(Key.ENTER, ...)
  2. addKeyPressListener(Key.of("ö"), ...)

Another alternative would be to have two separate overloads of addKeyPressListener (and all other similar methods), one accepting Key enum values and one accepting strings. This would again help the IDE guide users to the predefined Key.XYZ values.

@heruan
Copy link
Member

heruan commented May 3, 2018

@knoobie and @Legioth suggestions in #4061, plus improved JavaDocs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants