RFR: 8301121: RichTextArea Control (Incubator) [v37]
Ambarish Rapte
arapte at openjdk.org
Wed Nov 6 14:05:47 UTC 2024
On Tue, 5 Nov 2024 21:47:54 GMT, Andy Goryachev <angorya at 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
More information about the openjfx-dev
mailing list