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

[Kevin Rushforth]

On Thu, 12 Dec 2024 22:05:48 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> 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 addSquiggly(int start, int length, Color color) {
>> 
>> We need a better name.
>
> this is an internal method.

Nope. It's a public method in the public class, and it shows up in the javadocs.

>> 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:         // TODO
>> 
>> What is the implication of not having implemented this yet?
>
> For now, the RichTextModel does not support arbitrary Regions only text.

Then should this throw an UnsupportedOperationException?

>> modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/SimpleViewOnlyStyledModel.java line 96:
>> 
>>> 94:     /**
>>> 95:      * Appends a text segment to the last paragraph.
>>> 96:      * The {@code text} cannot contain newline ({@code \n}) symbols.
>> 
>> What if it does contain newline chars? Does it throw an Exception? is the newline ignored? Something else?
>
> `The caller must ensure that the {@code text} does not contain newline symbols.`
> 
> I would rather not add checking to this method.

OK. We might want to say that the behavior is undefined if the string contains a newline.

>> modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 138:
>> 
>>> 136:      * Returns the plain text string for the specified paragraph.  The returned text string cannot be null
>>> 137:      * and must not contain any control characters other than TAB.
>>> 138:      * The caller should never attempt to ask for a paragraph outside of the valid range.
>> 
>> Would it be better to specify an IOOBE?
>
> This is an abstract method, so the functionality is delegated to the model.
> If the model decides to throw an exception, so be it.
> 
> Not sure what to say here.  "The return value is undefined if the index falls outside the valid range"?

We could still specify it, but I can see why you'd rather not. In that case, maybe add a comment like "...might result in an IndexOutOfBoundsException"?

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1883022781
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1883031840
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1883028631
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1883031130