On Tue, 5 Nov 2024 21:47:54 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 52 commits: > > - readme > - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea > - review comments > - input map > - validate model > - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea > - javadoc > - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea > - review comments > - measurement node > - ... and 42 more: https://git.openjdk.org/jfx/compare/bd4bc057...a51ae151 Providing a few comments. I shall continue to review. modules/jfx.incubator.input/README.md line 4: > 2: > 3: This project incubates > 4: > [InputMap](src/main/java/javafx/incubator/scene/control/rich/RichTextArea.java) The link seems to be incorrect. modules/jfx.incubator.input/README.md line 6: > 4: > [InputMap](src/main/java/javafx/incubator/scene/control/rich/RichTextArea.java) > 5: > 6: Please refer to this [README](/tests/manual/RichTextAreaDemo/README.md). May be a pointer to exact sample which uses the new InputMap classes would be good here. modules/jfx.incubator.input/src/main/java/com/sun/jfx/incubator/scene/control/input/EventHandlerPriority.java line 2: > 1: /* > 2: * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights > reserved. Copyright year correction, may be it should be done as the last item before final push. (just in case if the PR reaches year 2025) modules/jfx.incubator.input/src/main/java/com/sun/jfx/incubator/scene/control/input/KeyEventMapper.java line 39: > 37: private static final int TYPED = 0x04; > 38: > 39: private int types; I think, the 3 event types are mutually exclusive, either a key would be pressed or released or typed. so the name of this variable should be a singular form `type` or preferably `eventType` modules/jfx.incubator.input/src/main/java/com/sun/jfx/incubator/scene/control/input/KeyEventMapper.java line 42: > 40: > 41: public KeyEventMapper() { > 42: } May be remove empty default ctor. modules/jfx.incubator.input/src/main/java/com/sun/jfx/incubator/scene/control/input/PHList.java line 46: > 44: * [ USER_HIGH, handler1, handler2, SKIN_KB, SKIN_LOW, handler3 ] > 45: */ > 46: private final ArrayList<Object> items = new ArrayList(4); Is there any specific reason as to why initial size is 4 ? Could use a final constant instead. modules/jfx.incubator.input/src/main/java/com/sun/jfx/incubator/scene/control/input/PHList.java line 49: > 47: > 48: public PHList() { > 49: } Can remove empty ctor. modules/jfx.incubator.input/src/main/java/com/sun/jfx/incubator/scene/control/input/PHList.java line 53: > 51: @Override > 52: public String toString() { > 53: return "PHList" + items; `return "PHList:" + items;` -> `return "PHList{items=" + items + "}";` modules/jfx.incubator.input/src/main/java/com/sun/jfx/incubator/scene/control/input/PHList.java line 72: > 70: if (handler != null) { > 71: insert(++ix, handler); > 72: } Is it necessary to store the priority when the handler is `null`. Will it be safe to simply return at beginning of the method when `handler == null` ? modules/jfx.incubator.input/src/main/java/com/sun/jfx/incubator/scene/control/input/PHList.java line 74: > 72: } > 73: } else { > 74: insert(ix, handler); Is `if (handler != null)` check required here as well? modules/jfx.incubator.input/src/main/java/com/sun/jfx/incubator/scene/control/input/PHList.java line 88: > 86: /** > 87: * Removes all the instances of the specified handler. Returns true > if the list becomes empty as a result. > 88: * Returns true if the list becomes empty as a result of the removal. `Returns true if the list becomes empty as a result` :-> This comment is repeated on line 87 and 88. It can be removed from line 87. modules/jfx.incubator.input/src/main/java/com/sun/jfx/incubator/scene/control/input/PHList.java line 106: > 104: } > 105: } > 106: return items.size() == 0; Use of `ArrayList.isEmpty()` would look nicer. `return items.size() == 0;` -> `return items.isEmpty();` modules/jfx.incubator.input/src/main/java/com/sun/jfx/incubator/scene/control/input/PHList.java line 213: > 211: } > 212: } > 213: return items.size() == 0; Use of ArrayList.isEmpty() would look nicer. return items.size() == 0; -> return items.isEmpty(); modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 69: > 67: CTRL, > 68: /** META modifier, mapped to COMMAND on Mac, META on > Windows/Linux */ > 69: META, The Command key on mac is mapped to both COMMAND(Line#65) and META. Is there a difference of right and left Command key ? If Yes, then may be it could be specified in the comment here. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 97: > 95: > 96: /** > 97: * Utility method creates a KeyBinding corresponding to a key press. Minor: typo / rewording. Similar comment applies to other helper methods of this class. `Utility method creates` -> `Utility method that creates` OR `Utility method to create` modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 257: > 255: */ > 256: public boolean isCommand() { > 257: return modifiers.contains(KCondition.COMMAND); Could safe guard with `if (PlatformUtil.isMac())` check to `return false` if not a mac platform modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 274: > 272: */ > 273: public boolean isOption() { > 274: return modifiers.contains(KCondition.OPTION); Could safe guard with `if (PlatformUtil.isMac())` check to `return false` if not a mac platform modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 708: > 706: } else if (linux) { > 707: replace(KCondition.SHORTCUT, KCondition.CTRL); > 708: } the two else blocks can be combined into one. else if (win || linux) { replace(KCondition.SHORTCUT, KCondition.CTRL); } modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 715: > 713: } else if (m.contains(KCondition.OPTION)) { > 714: return null; > 715: } if else can be combined into a single if statement. if (m.contains(KCondition.COMMAND) || m.contains(KCondition.OPTION)) { return null; } ------------- PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2417914541 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830794361 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830796450 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830799862 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830829104 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830829802 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830898100 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830894968 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1831039599 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1831043789 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1831046085 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830902022 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830920903 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1831069876 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830843738 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830871042 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830874251 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830875391 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830890067 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1830892125
