RFR: 8301121: RichTextArea Control (Incubator) [v65]

[Andy Goryachev]

On Wed, 8 Jan 2025 16:46:11 GMT, Pascal <duke at 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