On Wed, 20 Nov 2024 17:48:40 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 66 commits: > > - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea > - hide skin input map > - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea > - whitespace > - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea > - save as > - removed function handler > - removed add handler last > - use focus traversal api > - Merge remote-tracking branch 'origin/master' into 8301121.RichTextArea > - ... and 56 more: https://git.openjdk.org/jfx/compare/3a8a5598...e45be7b7 Here are some comments on the slimmed-down Input Map portion of this PR. My main questions are around the handling of platform-specific modifiers in `KeyBinding` -- most of them are macOS vs others. Do we need platform-specific methods or can we use platform-neutral methods that map to the specific key? modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/EventCriteria.java line 31: > 29: > 30: /** > 31: * This interface enables wider control in specifying conditional > matching logic when adding skin/behavior handlers. Can this interface be hidden as well? It may not need to be in the public API now that SkinInputMap and BehaviorBase aren't. If it still needs to be part of the public API, it will need a better description. One that answers the questions: What is the purpose of this interface? Who implements it? Who uses it? modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/FunctionTag.java line 34: > 32: * control's {@link InputMap}. > 33: * <h2>Example</h2> > 34: * The following example is taken from the {@code TabPane} class: Minor: I recommend using a pseudo-control or `RichTextArea` as your example, since there aren't any function tags for `TabPane`. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/FunctionTag.java line 51: > 49: public final class FunctionTag { > 50: /** Constructs the function tag. */ > 51: public FunctionTag() { Should this be more than a simple "marker" Object with identity, but no state? There is no way to get the information from a particular function tag instance as to what it represents, nor is there a useful "toString()" method. There is also no way to get the set of available FunctionTag objects from the nested <Control>.Tag class. Perhaps we could be informed by the Enum class? If there was value in doing this, maybe it could be a record? public final record FunctionTag(String name) { } This would be mostly useful for tooling, since most applications aren't likely to need it. It might be worth noting, even if you prefer not to make this change. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 53: > 51: * The {@code InputMap} The InputMap provides an ordered repository of > event handlers, > 52: * working together with {@link SkinInputMap} supplied by the skin, which > 53: * guarantees the order in which handers are invoked. `SkinInputMap` is not public, so maybe say something like "the input map managed by the Skin" (or similar). modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 60: > 58: * The class supports the following scenarios: > 59: * <ul> > 60: * <li>map a key binding to a function, provided either by the > application or the skin I think you can remove "or the skin" for this version. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 64: > 62: * <li>map a new function to an existing key binding > 63: * <li>obtain the default function > 64: * <li>add an event handler at specific priority (applies to > application-defined and skin-defined handlers) There is no API to control the priority (which is a good thing to have eliminated). I would remove this bullet. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 236: > 234: /** > 235: * Registers a function for the given key binding. This mapping > will take precedence > 236: * over any such mapping set by the skin. Maybe "any such mapping set _internally_ by the skin"? Or "the _default_ mapping set by the skin"? This comment applies here and elsewhere. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/InputMap.java line 325: > 323: > 324: /** > 325: * Collects all mapped key bindings (set either by the user or the > behavior). I don't think the "or the behavior" part is right (this wouldn't return anything set by the behavior, since that is an implementation detail). modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 36: > 34: > 35: /** > 36: * Key binding provides a way to map key event to a hash table key for > easy matching. What, exactly, is the purpose of this class as public API? The above makes it sound like an implementation detail. Also, what does it provide that KeyMapping doesn't? modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 123: > 121: * and the {@code alt} key modifier ({@code option} on macOS). > 122: * <p> > 123: * This method is equivalent to {@link #option(KeyCode)} on macOS. Then do we need both? modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 134: > 132: /** > 133: * Creates a KeyBinding which corresponds to the key press with the > specified {@code KeyCode} > 134: * and the {@code ctrl} key modifier ({@code control} on macOS). I don't see the need for calling out macOS here. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 145: > 143: /** > 144: * Creates a KeyBinding which corresponds to the key press with the > specified {@code KeyCode} > 145: * and the {@code shift} + {@code ctrl} key modifier ({@code > control} on macOS). I don't see the need for calling out macOS here. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 163: > 161: * @return the KeyBinding, or null > 162: */ > 163: public static KeyBinding option(KeyCode code) { In the documentation of `alt` you say that it is equivalent to `option` on macOS. So what we have is an `alt` method that works on all platforms and an `option` method that does the same thing as `alt` on macOS and returns `null` on other platforms? This seems like an odd situation. Do we really need the `option` method? modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 198: > 196: * @return the KeyBinding, or null > 197: */ > 198: public static KeyBinding shiftOption(KeyCode code) { Is there a `shiftAlt` that would make sense on other platforms? If so, maybe that could supersede this? modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 269: > 267: /** > 268: * Determines whether {@code alt} key is down in this key binding. > 269: * @return true if {@code alt} key is down in this key binding Does this return true if the `option` key is down on macOS? modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 343: > 341: /** > 342: * Creates a {@link Builder} with the specified KeyCode. > 343: * @param character the character Since the type is String what if it is more than one character? modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 375: > 373: * @return Builder instance > 374: */ > 375: public static Builder with(KeyCode c) { This is redundant. We don't need both `builder` and `with` methods that do exactly the same thing. I recommend removing this one. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/KeyBinding.java line 384: > 382: * @return Builder instance > 383: */ > 384: public static Builder with(String c) { This method is redundant -- it's identical to builder(String). I recommend removing it. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/package-info.java line 38: > 36: * <li>guarantees priorities between the application and the skin event > handlers and key mappings > 37: * <li>allows for gradual migration of the existing controls to use the > InputMap > 38: * <li>supports stateful and stateless (fully static) behavior > implementations This should be removed in this version. modules/jfx.incubator.input/src/main/java/jfx/incubator/scene/control/input/package-info.java line 42: > 40: * See > 41: * <a > href="https://github.com/andy-goryachev-oracle/Test/blob/main/doc/InputMap/InputMapV3.md";>Public > InputMap Proposal (v3)</a> > 42: * for more info. I don't think we want a pointer to the JEP here, especially since it isn't in a permanent repository. I would include the relevant information, but not the link. ------------- PR Review: https://git.openjdk.org/jfx/pull/1524#pullrequestreview-2449892430 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1852720301 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851125949 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851128262 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851104483 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851105523 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851106631 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851111535 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851121542 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1852724848 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851134441 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851134610 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851134754 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851136137 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851137520 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851138479 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851139587 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851142008 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851142256 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851122630 PR Review Comment: https://git.openjdk.org/jfx/pull/1524#discussion_r1851123668
