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

Fri Nov 1 21:36:36 UTC 2024

On Fri, 1 Nov 2024 17:40:02 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:
> 
>   input map

I think you addressed all of my earlier comments. I left a few comments and questions inline.

build.gradle line 5996:

> 5994:                 moduleProjList.each { project ->
> 5995:                     if (project.hasProperty("moduleName") && project.buildModule) {
> 5996:                         def incubating = project.hasProperty("incubating") && project.ext.incubating

After the merge of master, you can remove this (it is unused). I removed it from the PR #1616 (Support JavaFX incubator modules).

modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/Params.java line 102:

> 100: 
> 101:     /** default preferred height */
> 102:     public static final double PREF_HEIGHT = 176; // matches TextArea

I see `181` as TextArea height, at least on Windows. I think it is computed from the default number of columns visible. I didn't check on macOS.

modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/Params.java line 105:

> 103: 
> 104:     /** default preferred width */
> 105:     public static final double PREF_WIDTH = 176; // matches TextArea

I see `478` for TextArea width on Windows.

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 73:

> 71:  * <h2>Usage Considerations</h2>
> 72:  * {@code CodeArea} extends the {@link RichTextArea} class, meaning most of the functionality is expected to continue
> 73:  * working.  There are some differences that should be mentioned:

Suggestion: "... meaning most of the functionality works as it does in the base class" or similar?

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 78:

> 76:  * {@link #applyStyle(TextPos, TextPos, jfx.incubator.scene.control.richtext.model.StyleAttributeMap) applyStyle()},
> 77:  * will be ignored
> 78:  * <li>Line numbers: the line numbers are provided by setting the {@link #leftDecoratorProperty()}

This could be misconstrued to mean that the app should set the `leftDecoratorProperty` to provide line numbers. I would reword it to make it clear that CodeArea itself sets  `leftDecoratorProperty` so applications should not.

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 695:

> 693:     /**
> 694:      * Validates the model property value.
> 695:      * The subclass should override this method to check if the model type is supported and throw a TBD if not.

"TBD" --> "IllegalArgumentException".

Also, you might want to add something like: "... should override this method if it restricts the type of model that is supported ..." (since a subclass that takes all types of model need not override it).

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 696:

> 694:      * Validates the model property value.
> 695:      * The subclass should override this method to check if the model type is supported and throw a TBD if not.
> 696:      * A {@code null} value should never generate the exception.

"the exception" --> "an exception".

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/TextPos.java line 71:

> 69:      * @return the instance of {@code TextPos}
> 70:      */
> 71:     public static TextPos ofLeading(int index, int offset) {

Do you also plan to add an  `ofTrailing` convenience method?

modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/BasicTextModel.java line 68:

> 66:         /**
> 67:          * This method is called to insert a single text segment at the given position.
> 68:          * The caller guarantees that this method is only called when the content is writable.

Since this is a public method that could be called by an application, I think you still need to describe what happens if the caller does call when not writable.

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

PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2410673583
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826237458
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826292217
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826293494
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826311789
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826313077
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826166666
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826167717
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826181722
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826175861