Convert Key to interface, add static instances and improve JavaDocs #4061

heruan · 2018-05-03T09:28:26Z

Improves keyboard event listeners:

com.vaadin.flow.component.Key is now a functional interface;
the same interface provides several predefined instances (from MDN/Key_Values)
JavaDocs on notifier mixins are more exhaustive

Fixes #4046

This change is

Legioth · 2018-05-03T09:39:01Z

Apparently, <kbd> is not valid in JavaDocs.

heruan · 2018-05-03T09:52:46Z

Replaced <kbd> with <code>.

heruan · 2018-05-08T13:38:40Z

Not sure why TC build fails 😕

Legioth · 2018-05-08T14:03:12Z

The build log says flow-server/src/main/java/com/vaadin/flow/component/Key.java:446: error: unknown attribute: rel and similarly for rows 456, 463, 470, 1444 (unknown attribute: title) and 1445

heruan · 2018-05-08T14:08:33Z

Thank you @Legioth, I'll have a look at that!

Strange enough this is what I see (as guest) on the TC build log:

[main] ERROR com.vaadin.flow.router.InternalServerError - There was an exception while trying to navigate to 'fail/params'
java.lang.IllegalArgumentException: Given route parameter 'class java.lang.Long' is of the wrong type. Required 'class java.lang.String'.
	at com.vaadin.flow.router.BeforeEvent.checkUrlParameterType(BeforeEvent.java:241)
	at com.vaadin.flow.router.BeforeEvent.rerouteTo(BeforeEvent.java:218)
	at com.vaadin.flow.router.RouterTest$FailRerouteWithParams.beforeEnter(RouterTest.java:477)
	at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.executeBeforeEnterNavigation(AbstractNavigationStateRenderer.java:328)
	at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.handle(AbstractNavigationStateRenderer.java:194)
	at com.vaadin.flow.router.Router.handleNavigation(Router.java:201)
	at com.vaadin.flow.router.Router.navigate(Router.java:172)
	at com.vaadin.flow.router.RouterTest.reroute_fails_with_faulty_url_parameters(RouterTest.java:1490)

vaadin-bot · 2018-05-08T14:30:54Z

flow-server/src/main/java/com/vaadin/flow/component/Key.java

 */
-public class Key {
+@FunctionalInterface
+public interface Key extends Serializable {


Move constants to a class or enum.

vaadin-bot · 2018-05-08T14:49:43Z

SonarQube analysis reported 1 issue

1 major

Watch the comments in this conversation to review them.

Legioth · 2018-05-09T10:58:55Z

There was an exception while trying to navigate to 'fail/params' comes from a test that is supposed to throw an exception, so that one is fine.

The trick is that when the the build log terminates after a bunch of JavaDoc warnings, then there's also at least one JavaDoc error earlier during the same run. Those errors can be found by searching for "error:" in the log.

pleku · 2018-05-14T08:55:23Z

Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion.

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 279 at r4 (raw file):

     * called an "escape sequence."
     */
    Key ESCAPE = Key.of("Escape");

This doesn't work in MS Edge, please use the key code instead (I think it is 27)
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/5290772/#comment-3

Comments from Reviewable

pleku · 2018-05-14T08:57:47Z

Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 210 at r4 (raw file):

     * The Delete key, <code>Del</code>.
     */
    Key DELETE = Key.of("Delete");

Are you sure this works on MS Edge ? Reported here that it uses Del instead https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/5290772/#comment-2
Should change to the key code instead ?

Comments from Reviewable

heruan · 2018-05-14T09:12:04Z

@pleku What do you mean "change to the key code"? Just for DELETE? This PR is all about not using key codes, which bring other compatibility issues (with e.g. Asian keyboards).

Seems like Edge does report the wrong value and it's being tackled by the Edge team as a bug. Maybe we should just note this on the JavaDoc of the DELETE constant, linking at the bug report?

pleku · 2018-05-14T10:59:40Z

@heruan Edge reports Del instead of Delete (and Esc instead of Escape (?)) and it is a bug in Edge, true. Thus the API we have won't work on Edge. But if you use the key code http://keycode.info/ it works on both systems. And that key code is consistent for "functional" and number keys and even with other keyboards.

This PR is all about not using key codes
Sorry, I didn't look through the whole PR, so I didn't realise that (we run into this issue elsewhere and I recalled seeing a PR related to it).

I'm not sure how I want to workaround this issue in this PR, maybe a comment yes... but basically people should be using a key code in stead of the key for cross browser supported Escape and Delete. We should maybe add eg. specific EscPressedEvent and DeletePressedEvent that use the key code instead of key. And also EnterPressedEvent could mayhaps be great.

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

Legioth · 2018-05-15T09:16:06Z

I agree with @heruan that it's in general better to use key values instead of keyCode. Two solutions to consider include:

Make Key slightly more abstract so that it can either define a key or a keyCode.
Make Key support multiple codes, so that e.g. both Del and Delete could be considered for Key.DELETE.

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

heruan · 2018-05-22T10:37:16Z

I committed solution 2, to stay consistent with the initial attempt to let the user specify the semantic of the key rather than its physical position on the keyboard.

Another matter is whether or not implement an API to specify also the code, which correspond to a physical location of the key, e.g.

pressing Q in a US keyboard: event.key = 'q' and event.code = 'KeyQ';
pressing A in a French keyboard: event.key = 'a' but still event.code = 'KeyQ'.

Be aware that the value reported by http://keycode.info (i.e. keyCode) is deprecated.

caalador · 2018-05-23T11:07:59Z

Hi. Could you get the latest master into this branch? It should fix the build.

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

pekam · 2018-05-23T11:18:08Z

Reviewed 2 of 5 files at r1, 1 of 1 files at r5, 1 of 2 files at r6, 1 of 1 files at r7.
Review status: 4 of 5 files reviewed at latest revision, 3 unresolved discussions.

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 368 at r7 (raw file):

     * The <code>Power</code> button or key, to toggle power on and off.
     * <p>
     * Note: Not all systems pass this key through to to the user agent.

Duplicate "to"

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 832 at r7 (raw file):

    /**
     * Adjusts audio balance twoard the right.

Typo "twoard"

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 1096 at r7 (raw file):

    /**
     * General-purpose media funciton key, color-coded green; this has index 1

typo: "funciton"
same for the 4 below

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 1147 at r7 (raw file):

    /**
     * The Exit button, which exits the curreent application or menu.

typo: "curreent"

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 1354 at r7 (raw file):

     * A button to move the picture-in-picture view downward.
     */
    Key PIP_DOWN = Key.of("PinPDown");

There's "PIP" and "PinP" with an 'n'
Is this intentional or a typo?
Same for the three below.

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 1717 at r7 (raw file):

    /**
     * The decimal point key (typically <code>.</code> or <code>,</code>
     * depending on the region. In newer browsers, this value to simply be the

Missing ")"
Missing verb in the next sentence

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 1765 at r7 (raw file):

     * @return the {@link Key} instance
     */
    static Key of(String... keys) {

Now it allows to pass zero arguments and also seems a bit more confusing than before.

Maybe it could be something like:
static Key of(String key, String... duplicateKeys)

and explain in the javadocs that the duplicateKeys (or whatever would be a good name for this varargs) should be used only in rare cases where multiple codes map to the same key.

flow-server/src/main/java/com/vaadin/flow/component/KeyNotifier.java, line 95 at r7 (raw file):

    /**
     * Adds a {@code keydown} listener to this component, which will trigger
     * only if the keys involved in the event matche the {@code key} and

typo: "matche" (same for the two methods below)

flow-server/src/main/java/com/vaadin/flow/component/KeyNotifier.java, line 98 at r7 (raw file):

Key#of(String)

I guess this link doesn't work after changing the parameters to varargs. (same for the two methods below)

Comments from Reviewable

heruan · 2018-05-23T12:47:17Z

I apologize for the many typos, descriptions come from MDN and I trusted them too much 😅

Review status: 3 of 5 files reviewed at latest revision, 12 unresolved discussions.

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 368 at r7 (raw file):

Previously, pekam (Pekka Maanpää) wrote…

Duplicate "to"

Done.

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 832 at r7 (raw file):

Previously, pekam (Pekka Maanpää) wrote…

Typo "twoard"

Done.

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 1096 at r7 (raw file):

Previously, pekam (Pekka Maanpää) wrote…

typo: "funciton"
same for the 4 below

Done.

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 1147 at r7 (raw file):

Previously, pekam (Pekka Maanpää) wrote…

typo: "curreent"

Done.

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 1354 at r7 (raw file):

Previously, pekam (Pekka Maanpää) wrote…

There's "PIP" and "PinP" with an 'n'
Is this intentional or a typo?
Same for the three below.

Done.

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 1717 at r7 (raw file):

Previously, pekam (Pekka Maanpää) wrote…

Missing ")"
Missing verb in the next sentence

Done.

flow-server/src/main/java/com/vaadin/flow/component/Key.java, line 1765 at r7 (raw file):

Previously, pekam (Pekka Maanpää) wrote…

Now it allows to pass zero arguments and also seems a bit more confusing than before.

Maybe it could be something like:
static Key of(String key, String... duplicateKeys)

and explain in the javadocs that the duplicateKeys (or whatever would be a good name for this varargs) should be used only in rare cases where multiple codes map to the same key.

Done.

Comments from Reviewable

pekam · 2018-05-23T13:27:22Z

I apologize for the many typos, descriptions come from MDN and I trusted them too much 😅

Oh, right. Well now we have better docs than MDN. :D

Reviewed 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

pekam · 2018-05-23T13:36:56Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

a discussion (no related file):
One more thing: Now that Key can have additional values, we should have a test verifying that if you add a listener for a key with multiple values, the listener gets called for each of those key events.

Just say if you are busy or otherwise need help with this since we'd like to get this in before releasing the RC tomorrow. :)

Comments from Reviewable

heruan · 2018-05-23T14:17:59Z

Review status: 4 of 6 files reviewed at latest revision, 5 unresolved discussions.

a discussion (no related file):
Sure! The test indeed failed, but now should be okay 🤓

we'd like to get this in before releasing the RC tomorrow

get this in before releasing the RC tomorrow

before releasing the RC tomorrow

RC tomorrow

😍

Comments from Reviewable

pekam · 2018-05-23T14:25:57Z

Reviewed 2 of 2 files at r9.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

a discussion (no related file):

Previously, heruan (Giovanni Lovato) wrote…

Sure! The test indeed failed, but now should be okay 🤓

we'd like to get this in before releasing the RC tomorrow

get this in before releasing the RC tomorrow

before releasing the RC tomorrow

RC tomorrow

😍

Well that's the plan at least. :D

Thanks for the test! Good thing that the bug was found at this point.

There's still one un-resolved comment about typos in javadocs.

Comments from Reviewable

pekam · 2018-05-23T14:50:56Z

Reviewed 1 of 1 files at r10.
Review status: all files reviewed at latest revision, all discussions resolved.

flow-server/src/main/java/com/vaadin/flow/component/KeyNotifier.java, line 95 at r7 (raw file):

Previously, pekam (Pekka Maanpää) wrote…

typo: "matche" (same for the two methods below)

I fixed it myself, hope you don't mind.

Comments from Reviewable

pekam · 2018-05-24T08:08:56Z

RC tomorrow

😍

Sorry @heruan, we'll be releasing another beta after all.

Anyway, thanks again for the contribution and quick responses! :)

heruan mentioned this pull request May 3, 2018

Unclear how to use addKeyDown/Up/PressListener #4046

Closed

heruan force-pushed the key-notifier-interface branch from 5b4cd10 to ccf49e5 Compare May 3, 2018 09:33

heruan force-pushed the key-notifier-interface branch from a7ab7fa to 1dd9919 Compare May 3, 2018 09:53

caalador assigned heruan May 3, 2018

heruan force-pushed the key-notifier-interface branch 2 times, most recently from 52ea4b2 to 5bde3c2 Compare May 8, 2018 10:41

vaadin-bot reviewed May 8, 2018

View reviewed changes

heruan force-pushed the key-notifier-interface branch from 9156ebd to e993269 Compare May 22, 2018 10:24

heruan force-pushed the key-notifier-interface branch from 808d698 to 162e271 Compare May 23, 2018 10:37

Giovanni Lovato added 6 commits May 23, 2018 14:15

Convert Key to interface, add static instances and improve JavaDocs

b742e42

Use code instead of kbd in JavaDocs

4506e1c

Fix invalid JavaDoc attributes

f5afb12

Remove redundant static keyword

b328274

Make Key support multiple codes

74fca45

Fix JavaDocs

4409419

heruan force-pushed the key-notifier-interface branch from 162e271 to 1b63846 Compare May 23, 2018 12:45

Force at least one argument to Key.of and fix JavaDocs

130f685

heruan force-pushed the key-notifier-interface branch from 1b63846 to 130f685 Compare May 23, 2018 12:56

Fix bug and add test

3228017

Fix javadoc typos

f005508

pekam merged commit 3c0716f into vaadin:master May 24, 2018

pekam added this to the 1.0.0.beta12 milestone May 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Key to interface, add static instances and improve JavaDocs #4061

Convert Key to interface, add static instances and improve JavaDocs #4061

heruan commented May 3, 2018 •

edited by SomeoneToIgnore

Loading

Legioth commented May 3, 2018

heruan commented May 3, 2018

heruan commented May 8, 2018

Legioth commented May 8, 2018

heruan commented May 8, 2018

vaadin-bot May 8, 2018

vaadin-bot commented May 8, 2018

Legioth commented May 9, 2018

pleku commented May 14, 2018

pleku commented May 14, 2018

heruan commented May 14, 2018

pleku commented May 14, 2018

Legioth commented May 15, 2018

heruan commented May 22, 2018

caalador commented May 23, 2018

pekam commented May 23, 2018

heruan commented May 23, 2018

pekam commented May 23, 2018

pekam commented May 23, 2018

heruan commented May 23, 2018

pekam commented May 23, 2018

pekam commented May 23, 2018

pekam commented May 24, 2018

Convert Key to interface, add static instances and improve JavaDocs #4061

Convert Key to interface, add static instances and improve JavaDocs #4061

Conversation

heruan commented May 3, 2018 • edited by SomeoneToIgnore Loading

Legioth commented May 3, 2018

heruan commented May 3, 2018

heruan commented May 8, 2018

Legioth commented May 8, 2018

heruan commented May 8, 2018

vaadin-bot May 8, 2018

Choose a reason for hiding this comment

vaadin-bot commented May 8, 2018

Legioth commented May 9, 2018

pleku commented May 14, 2018

pleku commented May 14, 2018

heruan commented May 14, 2018

pleku commented May 14, 2018

Legioth commented May 15, 2018

heruan commented May 22, 2018

caalador commented May 23, 2018

pekam commented May 23, 2018

heruan commented May 23, 2018

pekam commented May 23, 2018

pekam commented May 23, 2018

heruan commented May 23, 2018

pekam commented May 23, 2018

pekam commented May 23, 2018

pekam commented May 24, 2018

heruan commented May 3, 2018 •

edited by SomeoneToIgnore

Loading