RFR: 8301121: RichTextArea Control (Incubator) [v61]
Kevin Rushforth
kcr at openjdk.org
Thu Dec 12 20:03:04 UTC 2024
On Wed, 11 Dec 2024 21:34:49 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 with a new target base due to a merge or a rebase. The pull request now contains 83 commits:
>
> - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea
> - legal
> - unicode license
> - add handler
> - review comments
> - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea
> - clamp
> - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea
> - review comments
> - review comments
> - ... and 73 more: https://git.openjdk.org/jfx/compare/b76c05b9...e5814b21
Here are some more API doc comments, and a few API naming suggestions.
modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 71:
> 69: * When such a mapping exists, the found function tag is matched to a function registered either by
> 70: * the application or by the skin.
> 71: * This mechanism allows for customizing the key mappings and the underlying functions independently and separately.
Additionally, the `register(KeyBinding,Runnable)` method allows mapping to a function directly, bypassing the function tag. You probably want to document this. You might also say that a `KeyBinding` is only mapped to a single value: either a FunctionTag or a Runnable (last one wins).
modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 102:
> 100:
> 101: /**
> 102: * Adds an event handler for the specified event type, at the control level.
Minor: The phrase "at the control level" seems redundant (and a little awkward), since this is the input map for the control.
modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 279:
> 277:
> 278: /**
> 279: * Unbinds the specified key binding.
There is a lack of symmetry in the name (also, we generally use "unbind" when talking about property bindings). Since "register" is used to create a mapping from a `KeyBinding` to a `FunctionTag` or `Runnable`, maybe rename this to "unregister"?
For that matter, what is the practical difference between this and `restoreDefaultKeyBinding(KeyBinding)`?
modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 363:
> 361: */
> 362: // TODO this should not affect the skin input map, but perhaps place NULL for each found KeyBinding
> 363: public void unbind(FunctionTag tag) {
This needs a different name than the other "unbind" method since the semantics are quite different. Since this removes all key bindings for a given function tag, I recommend renaming it to `removeKeyBindingsFor`, which goes nicely with `getKeyBindingsFor`.
And shouldn't there be an `unregisterFunction(FunctionTag)` method that is the inverse of `registerFunction(FunctionTag, Runnable)`? Or is that what `restoreDefaultFunction(FunctionTag)` does already?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 168:
> 166: * {@link InputMap#register(jfx.incubator.scene.control.input.KeyBinding, FunctionHandler)}.
> 167: */
> 168: public static class Tags {
Should this be `Tag` instead of `Tags`? Each instance is a single `FunctionTag`.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 523:
> 521:
> 522: /**
> 523: * Indicates whether this RichTextArea can be edited by the user, provided the model is also editable.
Minor: "model is also _writable_ ..."
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 1114:
> 1112: * possibly breaking up the grapheme clusters.
> 1113: * <p>
> 1114: * This method does nothing if either the control or the model is not editable,
Minor: "does nothing if the control is not editable or the model is not writable" (here and in other places in this file)
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/ContentChange.java line 108:
> 106: /**
> 107: * Determines whether the change is an edit (true) or affects styling only (false).
> 108: * @return true if change affects stylint only
This is backwards. Also, you spelled "styling" wrong, but since you need to remove it anyway, that problem will go away.
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.
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?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/RichTextModel.java line 182:
> 180: // TODO remove later
> 181: @Deprecated
> 182: public static void dump(StyledTextModel model, PrintStream out) {
It's time to remove this.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/RtfFormatHandler.java line 48:
> 46: public class RtfFormatHandler extends DataFormatHandler {
> 47: /** The singleton instance of {@code RtfFormatHandler}. */
> 48: public static final RtfFormatHandler INSTANCE = new RtfFormatHandler();
A better pattern is to provide a static `getInstance()` getter rather than making the singleton field itself public. That's what we do elsewhere in the API. This applies to the other subclasses as well.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/SimpleViewOnlyStyledModel.java line 63:
> 61:
> 62: /**
> 63: * Creates the model from the supplied text string by breaking it down into individual text segments.
Breaking it down, how? Splitting on newline characters? If so, then say that.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/SimpleViewOnlyStyledModel.java line 68:
> 66: * @throws IOException if an I/O error occurs
> 67: */
> 68: public static SimpleViewOnlyStyledModel from(String text) throws IOException {
Would "of" be a better method name?
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?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/SimpleViewOnlyStyledModel.java line 194:
> 192: * @return this model instance
> 193: */
> 194: public SimpleViewOnlyStyledModel squiggly(int start, int length, Color c) {
We need a netter name than "squiggly". CSS uses "wavy", so maybe "wavyUnderline"?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/SimpleViewOnlyStyledModel.java line 414:
> 412: * @param color the background color
> 413: */
> 414: void addSquiggly(int start, int length, Color color) {
Need a better name.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 119:
> 117: * {@code undo()},
> 118: * {@code redo()}
> 119: * methods, i.e. editing via UI.
Either spell out the `i.e` as "`that is,`" or change it to a parenthetical comment. As it is, the first sentence is truncated after the "i.e".
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 124:
> 122: * and fire the change events as a response, for example, to changes in its backing data storage.
> 123: *
> 124: * @return true if the model supports content modifications via the UI
It isn't just UI operations that cannot be done here, right?
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?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 150:
> 148: *
> 149: * @param index paragraph index in the range (0...{@link #size()})
> 150: * @return a new instance of TextCell created
Typo: this doesn't return a TextCell. Also, does it really always create a "new instance"? The description implies that it may or may not return a new instance.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 223:
> 221: /**
> 222: * Returns the {@link StyleAttributeMap} of the first character at the specified position.
> 223: * When at the end of the document, returns the attributes of the last character.
I don't know what you mean by "first" in "the first character at the specified position". Shouldn't this just be "the character at the specified position"?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 370:
> 368: * @return supported formats
> 369: */
> 370: public final DataFormat[] getSupportedDataFormats(boolean forExport) {
Would a List be better here?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 443:
> 441: /**
> 442: * Exports the stream of {@code StyledSegment}s in the given range to the specified
> 443: * {@code StyledOutput}.
Does it close the stream?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 841:
> 839: * @throws UnsupportedOperationException if the model is not {@link #isWritable() writable}
> 840: */
> 841: public final TextPos[] undo(StyleResolver resolver) {
Should this be a List?
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModelViewOnlyBase.java line 46:
> 44:
> 45: @Override
> 46: public final boolean isWritable() {
Consider adding a javadoc comment saying that this always returns false.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2493615952
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882471175
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882445317
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882491871
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882504267
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882592809
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882610654
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882612646
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882802145
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882713468
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882771077
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882769846
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882790081
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882660242
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882719757
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882662445
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882704457
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882712259
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882646685
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882734264
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882730692
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882727781
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882738554
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882742703
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882745332
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882752787
PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1882652057
More information about the openjfx-dev
mailing list