RFR: 8301121: RichTextArea Control (Incubator) [v28]
Kevin Rushforth
kcr at openjdk.org
Fri Oct 25 18:02:17 UTC 2024
On Mon, 21 Oct 2024 18:53:20 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:
>
> break iterator
Here is another round of comments, which mostly completes my review of the API in the `richtext` package. I left a few comments inline as well as the following general comments.
#### RichTextArea
API: Overall looks good
Basic functionality looks good. I tested that everything now works as expected for both a null and read-only model. The methods that are defined to modify text programmatically throw an exception and the methods that are called from the keyboard event handlers are a no-op.
One thing I noticed is that the default size of RichTextArea is too small; it should match that of TextArea
#### CodeArea
As you note in the code, we need to figure out the best way to constrain the `model` property of the RTA superclass. Here are some possibilities:
* Validate on use, treating a model that isn't CodeTextModel as being effectively null
* Check and throw an exception in the `invalidated` method, using an ad hoc mechanism (such as: an instanceof check on the RTA type or a package-scope validate method that is overridden by `CodeArea`)
* Check and throw an exception in the `invalidated` method, using a `protected boolean validate(StyledTextModel)` method that returns true in the base class and can be overridden by subclasses to enforce constraints (e.g., to require a certain type)
modules/javafx.controls/src/main/resources/com/sun/javafx/scene/control/skin/modena/modena.css line 3444:
> 3442: /*******************************************************************************
> 3443: * *
> 3444: * Rich Text Area *
Add "(Incubator)"
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 55:
> 53:
> 54: /**
> 55: * CodeArea is an editable text component which supports styling (syntax highlighting) of plain text.
Suggestion: add "for example," before "syntax highlighting" -- other types of highlighting could be possible.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 72:
> 70: * </p>
> 71: */
> 72: public class CodeArea extends RichTextArea {
The class docs could use to be expanded a little, especially regarding the superclass methods that behave differently (e.g., the style is ignored when calling any mutator method that accepts a a style) or superclass properties that are bound / set (e.g. `leftDecorator`).
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 219:
> 217: * The font to use for text in the {@code CodeArea}.
> 218: * @return the font property
> 219: * @defaultValue the Monospaced font with size 12.0 px
Would a monospaced font the size of the default system font, `1em`, be a better choice or do you think hard-coding 12 point is best?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineNumberDecorator.java line 51:
> 49: * @param pattern the DecimalFormat pattern to use
> 50: */
> 51: public LineNumberDecorator(String pattern) {
I think it would be more straight-forward to define `LineNumberDecorator(DecimalFormat format)` instead of taking a `String` pattern.
I recommend adding a getter for the format/pattern.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/Marker.java line 112:
> 110:
> 111: /**
> 112: * Returns the insert offset within the paragraph.
Is this an offset in characters? And what do you mean by _insert_ offset, since it isn't only used for cursors?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/SideDecorator.java line 58:
> 56: * measurement Node, whose preferred width will be used to size all the side Nodes (and must, therefore,
> 57: * be wider than any side node in the view). The {@code modelIndex} is this case is the index of
> 58: * the first paragraph in the view.
After reading it a couple of times, I think I now understand what this does. This is used to measure the pref width of the currently visible viewport being displayed by the RichTextArea, right? This could be clarified a bit.
Somewhat related, it might be cleaner and easier to explain / document if you defined a second method, `getMeasurementNode(modelIndex)` rather than using a boolean parameter. What do you think?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/TextPos.java line 34:
> 32: * For that, use {@link Marker}.
> 33: */
> 34: public final class TextPos implements Comparable<TextPos> {
Remind me why this isn't a record? It seems like it could be, unless I'm missing something.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/TextPos.java line 47:
> 45: * @param offset the text offset
> 46: * @param charIndex the character index
> 47: * @param leading true if leading
This needs a better description of what the parameters mean. I can't tell from just reading the docs what the difference is between "offset" and "charIndex". And you don't define "leading" -- I presume you mean related to the cursor? If so, what is its meaning if the TextPos isn't being used as the position of a cursor?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2395929730
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817075621
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817079875
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817081748
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817113864
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817084908
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817101810
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817093659
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817095181
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817097247
More information about the openjfx-dev
mailing list