On Wed, 8 Jan 2025 16:46:11 GMT, Pascal <d...@openjdk.org> wrote:
>> Good question!
>>
>> I've decided to create a new class for the incubator for two main reasons:
>> 1. the only way to construct `KeyCombination` is from a `String`, which
>> incurs the penalty of parsing
>> 2. `KeyCombination` does not differentiate key pressed / key released / key
>> typed events
>>
>> While 1) can be easily addressed by adding a Builder, introducing 2) may
>> cause issues with the existing code, since `KeyCombination` is an existing
>> public API.
>>
>> Any suggestions?
>
> `KeyCombination` is just the abstract base class. Subclasses like
> `javafx.scene.input.KeyCodeCombination` provide normal constructors without
> any parsing:
>
>
> public KeyCodeCombination(final @NamedArg("code") KeyCode code,
> final @NamedArg("modifiers") Modifier...
> modifiers) { /* ... */ }
>
>
> The abstract class provides a _match_ method which subclasses can (and do)
> override, so it could be possible to create a subclass that implements the
> method based on pressed/typed:
>
>
> @Override
> public boolean match(final KeyEvent event) {
> // KeyEvent has e.g. KEY_PRESSED and KEY_RELEASED, so should be
> possible to check here...
> return checkPressedReleasedTyped(event) && super.match(event);
> }
You are right, in theory it is possible to coerce `KeyCombination` to do what's
needed.
In practice, it's going to be rather clunky. The main issue is that we can't
use `KeyCombination` in the API calls because the base class has no concept or
key pressed/released/typed. Basically, we would need to do one of the two
things:
1. create a child class(es) of `KeyCombination` that contain the event type and
accept only those, or
2. accept `KeyCombination` but do a check at runtime of whether the right class
is passed, throwing an exception or ignoring the whole thing which is not
optimal.
Another alternative is to add three more modifiers (something like
`KeyPressed`, `KeyReleased`, `KeyTyped`), maybe defaulting to `KeyPressed` when
none is specified.
Doing this, however, brings up another issue of possible backward compatibility
when the new `KeyCombination` is used elsewhere - for example, what happens
when the `MenuItem::setAccelerator` is called with a `KeyTyped` or
`KeyReleased` modifier? The fact that the modified `KeyCombination` now
encodes the type of the event may cause the existing code to misbehave, and we
don't want to introduce regression.
All that, and the fact that we already have a non-public `KeyBinding` class for
the purposes of the existing non-public InputMap, is basically the rationale
behind the proposed design.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1907667261
