On Fri, 1 Nov 2024 17:40:02 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: > > input map I think you addressed all of my earlier comments. I left a few comments and questions inline. build.gradle line 5996: > 5994: moduleProjList.each { project -> > 5995: if (project.hasProperty("moduleName") && > project.buildModule) { > 5996: def incubating = > project.hasProperty("incubating") && project.ext.incubating After the merge of master, you can remove this (it is unused). I removed it from the PR #1616 (Support JavaFX incubator modules). modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/Params.java line 102: > 100: > 101: /** default preferred height */ > 102: public static final double PREF_HEIGHT = 176; // matches TextArea I see `181` as TextArea height, at least on Windows. I think it is computed from the default number of columns visible. I didn't check on macOS. modules/jfx.incubator.richtext/src/main/java/com/sun/jfx/incubator/scene/control/richtext/Params.java line 105: > 103: > 104: /** default preferred width */ > 105: public static final double PREF_WIDTH = 176; // matches TextArea I see `478` for TextArea width on Windows. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 73: > 71: * <h2>Usage Considerations</h2> > 72: * {@code CodeArea} extends the {@link RichTextArea} class, meaning most > of the functionality is expected to continue > 73: * working. There are some differences that should be mentioned: Suggestion: "... meaning most of the functionality works as it does in the base class" or similar? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 78: > 76: * {@link #applyStyle(TextPos, TextPos, > jfx.incubator.scene.control.richtext.model.StyleAttributeMap) applyStyle()}, > 77: * will be ignored > 78: * <li>Line numbers: the line numbers are provided by setting the {@link > #leftDecoratorProperty()} This could be misconstrued to mean that the app should set the `leftDecoratorProperty` to provide line numbers. I would reword it to make it clear that CodeArea itself sets `leftDecoratorProperty` so applications should not. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 695: > 693: /** > 694: * Validates the model property value. > 695: * The subclass should override this method to check if the model > type is supported and throw a TBD if not. "TBD" --> "IllegalArgumentException". Also, you might want to add something like: "... should override this method if it restricts the type of model that is supported ..." (since a subclass that takes all types of model need not override it). modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java line 696: > 694: * Validates the model property value. > 695: * The subclass should override this method to check if the model > type is supported and throw a TBD if not. > 696: * A {@code null} value should never generate the exception. "the exception" --> "an exception". modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/TextPos.java line 71: > 69: * @return the instance of {@code TextPos} > 70: */ > 71: public static TextPos ofLeading(int index, int offset) { Do you also plan to add an `ofTrailing` convenience method? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/model/BasicTextModel.java line 68: > 66: /** > 67: * This method is called to insert a single text segment at the > given position. > 68: * The caller guarantees that this method is only called when the > content is writable. Since this is a public method that could be called by an application, I think you still need to describe what happens if the caller does call when not writable. ------------- PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2410673583 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826237458 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826292217 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826293494 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826311789 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826313077 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826166666 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826167717 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826181722 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1826175861
