On Wed, 11 Dec 2024 21:34:49 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 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
