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

Tue Dec 3 21:32:56 UTC 2024

On Tue, 3 Dec 2024 19:08:44 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Incubating a new feature - rich text control, **RichTextArea**, intended to bridge the functional gap with Swing and its StyledEditorKit/JEditorPane. The main design goal is to provide a control that is complete enough to be useful out-of-the box, as well as open to extension by the application developers.
>> 
>> This is a complex feature with a large API surface that would be nearly impossible to get right the first time, even after an extensive review. We are, therefore, introducing this in an incubating module, **jfx.incubator.richtext**. This will allow us to evolve the API in future releases without the strict compatibility constraints that other JavaFX modules have.
>> 
>> Please check out two manual test applications - one for RichTextArea (**RichTextAreaDemoApp**) and one for the CodeArea (**CodeAreaDemoApp**). Also, a small example provides a standalone rich text editor, see **RichEditorDemoApp**.
>> 
>> Because it's an incubating module, please focus on the public APIs rather than implementation.  There **will be** changes to the implementation once/if the module is promoted to the core by popular demand.  The goal of the incubator is to let the app developers try the new feature out. 
>> 
>> **References**
>> 
>> - Proposal: https://github.com/andy-goryachev-oracle/Test/blob/main/doc/RichTextArea/RichTextArea.md
>> - Discussion points: https://github.com/andy-goryachev-oracle/Test/blob/main/doc/RichTextArea/RichTextAreaDiscussion.md
>> - API specification (javadoc): https://cr.openjdk.org/~angorya/RichTextArea/javadoc
>> - RichTextArea RFE: https://bugs.openjdk.org/browse/JDK-8301121
>> - Behavior doc: https://github.com/andy-goryachev-oracle/jfx/blob/8301121.RichTextArea/doc-files/behavior/RichTextAreaBehavior.md
>> - CSS Reference: https://cr.openjdk.org/~angorya/RichTextArea/javadoc/javafx.graphics/javafx/scene/doc-files/cssref.html
>> - InputMap (v3): https://github.com/andy-goryachev-oracle/Test/blob/main/doc/InputMap/InputMapV3.md
>> - Previous Draft PR: https://github.com/openjdk/jfx/pull/1374
>
> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review comments

Here are my comments on the updated KeyBinding / KeyBinding.Builder API. Mostly it's just documentation suggestions with a couple method renaming suggestions.

modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 214:

> 212:      * and the {@code shift} + shortcut key modifier ({@code ⌘ command} on macOS, {@code ctrl} elsewhere).
> 213:      * <p>
> 214:      * This method returns {@code null} on non-macOS platforms.

This comment is wrong, since this method is valid on all platforms.

modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 459:

> 457: //    }
> 458: 
> 459:     /** Key bindings builder */

Can you expand this a bit? Also, end with a period. At a minimum, something like this:

 * A builder for {@code KeyBinding} objects.

but maybe also adding an additional sentence or two along with a pointer to the `KeyBinding.builder` methods and the `build` method of this class?

modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 473:

> 471:          * @return the Builder instance
> 472:          */
> 473:         public Builder onKeyReleased() {

We use methods named `onXxxx` for handers, so the `on` is misleading here. I recommend one of the following alternatives:

* `keyReleased` / `keyTyped` (for symmetry, we might consider adding `keyPressed` even though it is default)
* `eventType(EventType<KeyEvent>)`

The first alternative seems more convenient. If you stick with that (basically a renamed version of what you have), I recommend adding a sentence that it will clear `KEY_PRESSED` and `KEY_TYPED`.

modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 501:

> 499: 
> 500:         /**
> 501:          * Sets the {@code alt} key down condition (the {@code Option} key on macOS).

The docs for this method could be clarified, especially since there is also a no-arg `alt()` method with exactly the same description.

I presume that if `condition` is true then it sets the alt key down condition and if `condition` is false then it clears the alt key down condition? If so, say that (or something similar).

This applies to all of the modifer methods that take a boolean.

modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 513:

> 511: 
> 512:         /**
> 513:          * Sets {@code command} key down condition.

You need to document what this method does on non-macOS platforms. Maybe something like

 * Sets {@code command} key down condition on macOS.
 *
 * <p>
 * Setting this condition on non-macOS platforms will result in the
 * {@code build} method returning {@code null}.

This comment also applies to the other macOS-specific key modifier methods.

modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 537:

> 535:          * @return this Builder
> 536:          */
> 537:         public Builder ctrl() {

s/ctrl/control/

modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 653:

> 651: 
> 652:         /**
> 653:          * Creates a new {@link KeyBinding} instance.

Suggestion: add "from the current settings" after "instance"

-------------

PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2476565912
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868288678
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868367573
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868276670
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868282333
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868355962
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868345590
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868358133