On Thu, 12 Dec 2024 22:05:48 GMT, Andy Goryachev <ango...@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
