On Mon, 21 Oct 2024 18:53:20 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: > > break iterator Here is another round of comments, which mostly completes my review of the API in the `richtext` package. I left a few comments inline as well as the following general comments. #### RichTextArea API: Overall looks good Basic functionality looks good. I tested that everything now works as expected for both a null and read-only model. The methods that are defined to modify text programmatically throw an exception and the methods that are called from the keyboard event handlers are a no-op. One thing I noticed is that the default size of RichTextArea is too small; it should match that of TextArea #### CodeArea As you note in the code, we need to figure out the best way to constrain the `model` property of the RTA superclass. Here are some possibilities: * Validate on use, treating a model that isn't CodeTextModel as being effectively null * Check and throw an exception in the `invalidated` method, using an ad hoc mechanism (such as: an instanceof check on the RTA type or a package-scope validate method that is overridden by `CodeArea`) * Check and throw an exception in the `invalidated` method, using a `protected boolean validate(StyledTextModel)` method that returns true in the base class and can be overridden by subclasses to enforce constraints (e.g., to require a certain type) modules/javafx.controls/src/main/resources/com/sun/javafx/scene/control/skin/modena/modena.css line 3444: > 3442: > /******************************************************************************* > 3443: * > * > 3444: * Rich Text Area > * Add "(Incubator)" modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 55: > 53: > 54: /** > 55: * CodeArea is an editable text component which supports styling (syntax > highlighting) of plain text. Suggestion: add "for example," before "syntax highlighting" -- other types of highlighting could be possible. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 72: > 70: * </p> > 71: */ > 72: public class CodeArea extends RichTextArea { The class docs could use to be expanded a little, especially regarding the superclass methods that behave differently (e.g., the style is ignored when calling any mutator method that accepts a a style) or superclass properties that are bound / set (e.g. `leftDecorator`). modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/CodeArea.java line 219: > 217: * The font to use for text in the {@code CodeArea}. > 218: * @return the font property > 219: * @defaultValue the Monospaced font with size 12.0 px Would a monospaced font the size of the default system font, `1em`, be a better choice or do you think hard-coding 12 point is best? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/LineNumberDecorator.java line 51: > 49: * @param pattern the DecimalFormat pattern to use > 50: */ > 51: public LineNumberDecorator(String pattern) { I think it would be more straight-forward to define `LineNumberDecorator(DecimalFormat format)` instead of taking a `String` pattern. I recommend adding a getter for the format/pattern. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/Marker.java line 112: > 110: > 111: /** > 112: * Returns the insert offset within the paragraph. Is this an offset in characters? And what do you mean by _insert_ offset, since it isn't only used for cursors? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/SideDecorator.java line 58: > 56: * measurement Node, whose preferred width will be used to size all > the side Nodes (and must, therefore, > 57: * be wider than any side node in the view). The {@code modelIndex} > is this case is the index of > 58: * the first paragraph in the view. After reading it a couple of times, I think I now understand what this does. This is used to measure the pref width of the currently visible viewport being displayed by the RichTextArea, right? This could be clarified a bit. Somewhat related, it might be cleaner and easier to explain / document if you defined a second method, `getMeasurementNode(modelIndex)` rather than using a boolean parameter. What do you think? modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/TextPos.java line 34: > 32: * For that, use {@link Marker}. > 33: */ > 34: public final class TextPos implements Comparable<TextPos> { Remind me why this isn't a record? It seems like it could be, unless I'm missing something. modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/TextPos.java line 47: > 45: * @param offset the text offset > 46: * @param charIndex the character index > 47: * @param leading true if leading This needs a better description of what the parameters mean. I can't tell from just reading the docs what the difference is between "offset" and "charIndex". And you don't define "leading" -- I presume you mean related to the cursor? If so, what is its meaning if the TextPos isn't being used as the position of a cursor? ------------- PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2395929730 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817075621 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817079875 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817081748 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817113864 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817084908 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817101810 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817093659 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817095181 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1817097247
