On Tue, 17 Dec 2024 21:06:16 GMT, Andy Goryachev <ango...@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:
> 
>   added html charset

Nice work! Approved with some feedback that can be addressed in follow-up 
issues. I left a couple comments inline and have a few general comments below.

### Functional issues

I noticed the following minor functional bugs:

* Default width of RTA is less than half the size of TextArea (176 vs 478 on my 
Windows 11 system). Earlier the height was also too small, but you already 
fixed that.
* When running the demos, the current line highlight overlaps and obscures 
focus outline on right side (verified on Windows and Linux).

I can file the above bugs, unless you want to.

### Tests

We need more automated unit tests. Please file follow-up test bugs to create 
additional tests for each of the following:

* RichTextArea API tests
* RichTextArea behavioral tests
* InputMap tests

### Demos

Please file the following demo build issues:

* Ability to build and run the samples easily from the command line (possibly 
via ant / gradle). They are modular apps, so it isn't currently 
straight-forward.
* Consider hooking the samples up to the build (so they are built as part of 
the "gradle apps" task. This depends on the previous.

apps/samples/RichTextAreaDemo/test/test/com/oracle/demo/richtext/codearea/TestJavaSyntaxDecorator.java
 line 1:

> 1: /*

This class (and thus the entire `test/**` directory  tree) is unused.

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

> 113:  *     SimpleViewOnlyStyledModel m = new SimpleViewOnlyStyledModel();
> 114:  *     // add text segment using CSS style name (requires a stylesheet)
> 115:  *     m.addWithStyles("RichTextArea ", "HEADER");

This example needs to be updated to reflect an earlier change in the API:

`addWithStyles` --> `addWithStyleNames`

modules/jfx.incubator.richtext/src/test/java/test/com/sun/jfx/incubator/scene/control/richtext/TestRichTextArea.java
 line 42:

> 40:     // TODO remove once a real test which needs the shim is added.
> 41:     @Test
> 42:     public void testShim() {

Can you add a note to the follow-up bug that you will file to track adding 
these tests?

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

Marked as reviewed by kcr (Lead).

PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2538362284
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1907909939
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1907907103
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1907942520

Reply via email to