On Tue, 3 Dec 2024 19:08:44 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 Here are my comments on the updated KeyBinding / KeyBinding.Builder API. Mostly it's just documentation suggestions with a couple method renaming suggestions. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 214: > 212: * and the {@code shift} + shortcut key modifier ({@code ⌘ command} > on macOS, {@code ctrl} elsewhere). > 213: * <p> > 214: * This method returns {@code null} on non-macOS platforms. This comment is wrong, since this method is valid on all platforms. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 459: > 457: // } > 458: > 459: /** Key bindings builder */ Can you expand this a bit? Also, end with a period. At a minimum, something like this: * A builder for {@code KeyBinding} objects. but maybe also adding an additional sentence or two along with a pointer to the `KeyBinding.builder` methods and the `build` method of this class? modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 473: > 471: * @return the Builder instance > 472: */ > 473: public Builder onKeyReleased() { We use methods named `onXxxx` for handers, so the `on` is misleading here. I recommend one of the following alternatives: * `keyReleased` / `keyTyped` (for symmetry, we might consider adding `keyPressed` even though it is default) * `eventType(EventType<KeyEvent>)` The first alternative seems more convenient. If you stick with that (basically a renamed version of what you have), I recommend adding a sentence that it will clear `KEY_PRESSED` and `KEY_TYPED`. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 501: > 499: > 500: /** > 501: * Sets the {@code alt} key down condition (the {@code Option} > key on macOS). The docs for this method could be clarified, especially since there is also a no-arg `alt()` method with exactly the same description. I presume that if `condition` is true then it sets the alt key down condition and if `condition` is false then it clears the alt key down condition? If so, say that (or something similar). This applies to all of the modifer methods that take a boolean. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 513: > 511: > 512: /** > 513: * Sets {@code command} key down condition. You need to document what this method does on non-macOS platforms. Maybe something like * Sets {@code command} key down condition on macOS. * * <p> * Setting this condition on non-macOS platforms will result in the * {@code build} method returning {@code null}. This comment also applies to the other macOS-specific key modifier methods. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 537: > 535: * @return this Builder > 536: */ > 537: public Builder ctrl() { s/ctrl/control/ modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 653: > 651: > 652: /** > 653: * Creates a new {@link KeyBinding} instance. Suggestion: add "from the current settings" after "instance" ------------- PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2476565912 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868288678 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868367573 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868276670 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868282333 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868355962 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868345590 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1868358133
