On Fri, 13 Dec 2024 00:08:09 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: > > review comments Most of the changes look good. I left a few comments. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 287: > 285: * {@link #register(KeyBinding, Runnable)}, > 286: * {@link #registerKey(KeyBinding, FunctionTag)}, > 287: * or registered by the skin. The "or registered by the skin" part seems doesn't seem right to me. Is that really what happens? And if so, why is it the right thing to do? modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 292: > 290: */ > 291: public void unregister(KeyBinding k) { > 292: map.put(k, NULL); Why does this add a `NULL` rather than remove the mapping? I'm trying to understand the difference between this method and `resetKeyBindings` (maybe this is related to my earlier question above?). One thing to consider is that allowing `NULL` values means that there is no way to tell the difference between a key with a `NULL` value and a key that isn't in the map without calling `map.contains`, which you don't expose. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 371: > 369: */ > 370: // TODO this should not affect the skin input map, but perhaps place > NULL for each found KeyBinding > 371: public void unregister(FunctionTag tag) { For symmetry (and better clarity), I still think the name should have `KeyBindingsFor` in the name, perhaps `removeKeyBindingsFor` or `unregisgterKeyBindingsFor`. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/RichParagraph.java line 210: > 208: > 209: /** > 210: * Adds a wave underline (typically used as a spell checker > indicator) with the given color. Minor: "wave" --> "wavy" 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 wavyUnderline(int start, int length, Color color) > { Should the name start with "add" (e.g., to match `addHighlight`)? 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: throw new UnsupportedOperationException(); Should this be documented? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/SimpleViewOnlyStyledModel.java line 99: > 97: * Appends a text segment to the last paragraph. > 98: * The caller must ensure that the {@code text} does not contain > newline symbols, as the behavior might be > 99: * undefined. Minor: I would say "is undefined" (instead of "might be") meaning that we don't define or specify the behavior. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/SimpleViewOnlyStyledModel.java line 191: > 189: > 190: /** > 191: * Adds a wave underline (typically used as a spell checker > indicator) to the specified range within the last paragraph. Minor: "wave" --> "wavy" modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 140: > 138: * Returns the plain text string for the specified paragraph. The > returned text string cannot be null > 139: * and must not contain any control characters other than TAB. > 140: * The caller must not attempt to ask for a paragraph outside of the > valid range. It might be worth adding the note about "doing otherwise might result in an exception..." as you mentioned below. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/StyledTextModel.java line 150: > 148: * Returns a {@link RichParagraph} at the given model index. > 149: * The callers must ensure that the value of {@code index} is within > the valid document range, > 150: * since doing otherwise might result in an exception or undetermied > behavior. Typo: "undetermied" --> "undetermined" ------------- PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2509752650 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889030681 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889046058 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889034881 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889053342 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889055776 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889057083 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889058306 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889059447 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889061153 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1889061933
