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

Tue Dec 17 19:03:03 UTC 2024

On Fri, 13 Dec 2024 00:08:09 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

Most of the changes look good. I left a few comments.

modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 287:

> 285:      * {@link #register(KeyBinding, Runnable)},
> 286:      * {@link #registerKey(KeyBinding, FunctionTag)},
> 287:      * or registered by the skin.

The "or registered by the skin" part seems doesn't seem right to me. Is that really what happens? And if so, why is it the right thing to do?

modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 292:

> 290:      */
> 291:     public void unregister(KeyBinding k) {
> 292:         map.put(k, NULL);

Why does this add a `NULL` rather than remove the mapping? I'm trying to understand the difference between this method and `resetKeyBindings` (maybe this is related to my earlier question above?). One thing to consider is that allowing `NULL` values means that there is no way to tell the difference between a key with a `NULL` value and a key that isn't in the map without calling `map.contains`, which you don't expose.

modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 371:

> 369:      */
> 370:     // TODO this should not affect the skin input map, but perhaps place NULL for each found KeyBinding
> 371:     public void unregister(FunctionTag tag) {

For symmetry (and better clarity), I still think the name should have `KeyBindingsFor` in the name, perhaps `removeKeyBindingsFor` or `unregisgterKeyBindingsFor`.

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/RichParagraph.java line 210:

> 208: 
> 209:         /**
> 210:          * Adds a wave underline (typically used as a spell checker indicator) with the given color.

Minor: "wave" --> "wavy"

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/RichParagraph.java line 216:

> 214:          * @return this {@code Builder} instance
> 215:          */
> 216:         public Builder wavyUnderline(int start, int length, Color color) {

Should the name start with "add" (e.g., to match `addHighlight`)?

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/RichTextModel.java line 132:

> 130:     @Override
> 131:     protected void insertParagraph(int index, Supplier<Region> generator) {
> 132:         throw new UnsupportedOperationException();

Should this be documented?

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/SimpleViewOnlyStyledModel.java line 99:

> 97:      * Appends a text segment to the last paragraph.
> 98:      * The caller must ensure that the {@code text} does not contain newline symbols, as the behavior might be
> 99:      * undefined.

Minor: I would say "is undefined" (instead of "might be") meaning that we don't define or specify the behavior.

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/SimpleViewOnlyStyledModel.java line 191:

> 189: 
> 190:     /**
> 191:      * Adds a wave underline (typically used as a spell checker indicator) to the specified range within the last paragraph.

Minor: "wave" --> "wavy"

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 140:

> 138:      * Returns the plain text string for the specified paragraph.  The returned text string cannot be null
> 139:      * and must not contain any control characters other than TAB.
> 140:      * The caller must not attempt to ask for a paragraph outside of the valid range.

It might be worth adding the note about "doing otherwise might result in an exception..." as you mentioned below.

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 150:

> 148:      * Returns a {@link RichParagraph} at the given model index.
> 149:      * The callers must ensure that the value of {@code index} is within the valid document range,
> 150:      * since doing otherwise might result in an exception or undetermied behavior.

Typo: "undetermied" --> "undetermined"

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

PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2509752650
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889030681
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889046058
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889034881
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889053342
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889055776
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889057083
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889058306
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889059447
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889061153
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889061933