8301121: RichTextArea Control (Incubator) #1524
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
/reviewers 3 |
|
@andy-goryachev-oracle |
| * @author Andy Goryachev | ||
| * @since 999 TODO | ||
| */ | ||
| public class RichTextArea extends Control { |
There was a problem hiding this comment.
The old TextField and TextArea controls extend TextInputControl.
I wonder whether there is a a good reason not doing the same for RichTextArea?
Doing so would allow to more easily switch between one or the other.
There was a problem hiding this comment.
Thank you for a very good question!
The main reason is that RichTextArea (RTA) can work with a large text model which needs to be represented differently from TextInputControl's simple String text property. Other things, like position within the text is also very different, using {paragraphIndex,charOffset} instead of a simple int offset. The fact that the model is separated from the control also makes it very different.
At the end, there is simply no benefit in dragging the TextInputControl into the picture.
There was a problem hiding this comment.
I was hoping to be able to easily replace one (old) control with the new one. I mean RichTextArea still fulfilling the requirements of TextInputControl. Anyhow, I understand you rational not going along this path 👍
Let me provide the use-case I have in mind (which may explain better what I am looking for).
I would like to use/keep textfields and textareas with pure text. Anyhow, having the possibility to squiggly parts of the text showing spelling mistakes. Since I assume that loading many RichTextArea controls (i.e., in tables) will take much longer I expect to offer spelling suggestions (and RichTextArea loading) on request only.
There was a problem hiding this comment.
It looks like in this particular case you do want to preserve the semantics and APIs of TextArea, rather than switch to a new API.
You could simply extend TextArea/Skin and add functionality to superimpose the squiggly on top of TextArea. You'll need to listen to several properties and make sure the clipping is set up correctly and you handle the scrolling et cetera, but I think it's doable.
Of course, once you decide that you need to further decorate your text, with let's say highlighted areas, then you might as well use the new component.
By the way, CodeArea is specifically designed for that purpose, even has getText() and setText() - but no textProperty because it's designed to work with large documents.
There was a problem hiding this comment.
You might be right about impact on performance when trying to use many RichTextArea (RTA) controls as table cells - it is indeed a heavier component, but as long as you ensure there are no memory leaks and your table does not show 1000s of cells on screen we should be ok.
If performance is an issue, a full blown editor might indeed be overkill, and you might be better off created a subclass of Labeled adorned with decorations instead. You can always set up RTA as an editor if you need to.
|
/csr |
|
@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request. @andy-goryachev-oracle please create a CSR request for issue JDK-8301121 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
|
/reviewers 2 reviewers |
|
@andy-goryachev-oracle |
|
/reviewers 3 |
|
@andy-goryachev-oracle |
|
how do I set 3 reviewers and at least 2 "R"eviewers? |
You can't. You can set it to |
|
/reviewers 2 reviewers |
|
@andy-goryachev-oracle |
| * Should not be called with localY outside of this layout sliding window. | ||
| */ | ||
| private int binarySearch(double localY, int low, int high) { | ||
| //System.err.println(" binarySearch off=" + off + ", high=" + high + ", low=" + low); // FIX |
There was a problem hiding this comment.
Looks like remnants of the debugging session. Do you still need it?
There was a problem hiding this comment.
yes, indeed it is (to be cleaned up later).
I would encourage the reviewers to focus on the public API rather than the implementation at this stage.
| @@ -446,6 +446,16 @@ <h2>Contents</h2> | |||
| </li> | |||
| </ul> | |||
| </li> | |||
| <li><a href="#incubator-modules">Incubator Modules</a> | |||
| <ul> | |||
| <li>jfx.incubator.scene.control.rich | |||
There was a problem hiding this comment.
module name typo : rich -> richtext
| <table class="package" width="100%"> | ||
| <tbody> | ||
| <tr> | ||
| <td>jfx.incubator.scene.control.rich</td> |
There was a problem hiding this comment.
module name typo : rich -> richtext
| <li>jfx.incubator.scene.control.rich | ||
| <ul> | ||
| <li><a href="#codearea">CodeArea</a></li> | ||
| <li><a href="#richtextarea">RichTextArea</a></li> |
There was a problem hiding this comment.
The order of these 2 items should be reversed to match order present in the description.
There was a problem hiding this comment.
I think the general convention is to list the classes in alphabetical order.
| public interface SyntaxDecorator { | ||
| /** | ||
| * Creates a {@link RichParagraph} from the paragraph plain text. | ||
| * The text string is guaranteed to contain neither newline nor carriage return symbols. |
There was a problem hiding this comment.
Comment seems unclear to me. Which text string ? There is no String parameter here, only a CodeTextModel with a paragraph index...
There was a problem hiding this comment.
I am going to rework the javadoc here. Thank you!
| * This content provides in-memory storage in an {@code ArrayList} of {@code String}s. | ||
| */ | ||
| public static class InMemoryContent implements Content { | ||
| private final ArrayList<String> paragraphs = new ArrayList<>(); |
There was a problem hiding this comment.
Since paragraphs may be inserted anywhere, I would guess that using a LinkedList should be more efficient here. Otherwise, when adding a line break somewhere in the middle of a long text, a lot of array members must be copied.
There was a problem hiding this comment.
You are right, but this is just a default implementation. you can still implement your own BasicTextModel.Content and pass it to the constructor.
Do you want to see an in-memory implementation optimized for large number of paragraphs or should it better be left up to the app developers?
There was a problem hiding this comment.
Yes, we should definitely provide a very efficient implementation, because most app developers will not want to change it. We should definitely avoid another "TextArea situation", where the application breaks apart, if the text grows too big.
There was a problem hiding this comment.
I am strongly against LinkedList here: not only it adds 2 pointers to each element, but mostly because it provides no random access.
ArrayList should be sufficient for simple cases, and the developers are free to implement a better Content for their use case. Maybe it's a skip list, or BPlusTree, or something else.
I would also like to mention that the performance issue with TextArea is not the storage (at least it is not the first order issue), but the fact that it renders all the text (in a single Text node). RTA renders only the sliding window, which should be much faster. Of course, RTA will have issues with very long paragraphs, but that's a different story (and is mentioned in the Non-Goals section)
https://github.com/andy-goryachev-oracle/Test/blob/main/doc/RichTextArea/RichTextArea.md#non-goals
| } | ||
|
|
||
| @Override | ||
| public RichParagraph createRichParagraph(CodeTextModel model, int index) { |
There was a problem hiding this comment.
When implementing "real" syntax highlighting, the style of a paragraph will depend on the style of it's predecessors (e.g. multiline strings or multiline comments)...
There was a problem hiding this comment.
Correct, this is just a demo.
The information passed to the SyntaxDecorator interface should be sufficient to support any "real" syntax, at least I hope it is. We are passing the model to each method, so it can re/construct the syntax or cache it as needed.
There was a problem hiding this comment.
Consider this situation: When a user types some text into paragraph 2, it may change the styling in paragraph 2, 3, 4, ...
Will the createRichParagraph - method be called for each paragraph after each typed character? That would probably be inefficient; On the other hand, if it is only called for the paragraph the user typed in, that may not be enough.
There was a problem hiding this comment.
In this case the model fires a change event which defines the affected area. The view (the skin) determines which visible elements need to be updated, by calling StyledTextModel::getParagraph().
I don't think there will be efficiency issues because
- these visuals are likely to be updated anyway
- the number of requested paragraphs is limited by the size of the sliding window (i.e. screen size + some margin)
|
@andy-goryachev-oracle |
@PavelTurk there was a discussion re: caret with mixed text here This PR is closed now, but the issue (https://bugs.openjdk.org/browse/JDK-8296266) is still there and I hope to eventually resolve it. |
| # Incubator | ||
|
|
||
| This project incubates | ||
| [InputMap](src/main/java/javafx/incubator/scene/control/rich/RichTextArea.java) |
| [InputMap](src/main/java/javafx/incubator/scene/control/rich/RichTextArea.java) | ||
|
|
||
| Please refer to this [README](/tests/manual/RichTextAreaDemo/README.md). |
There was a problem hiding this comment.
May be a pointer to exact sample which uses the new InputMap classes would be good here.
| @@ -0,0 +1,52 @@ | |||
| /* | |||
| * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved. | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
the copyright dates reflect the years the code became visible to the public.
it will be updated in 2025 if necessary.
| private static final int RELEASED = 0x02; | ||
| private static final int TYPED = 0x04; | ||
|
|
||
| private int types; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
all these are private fields of an internal implementation
| public KeyEventMapper() { | ||
| } |
There was a problem hiding this comment.
May be remove empty default ctor.
There was a problem hiding this comment.
why? this class is not exposed.
| } | ||
| } | ||
| } | ||
| return items.size() == 0; |
There was a problem hiding this comment.
Use of ArrayList.isEmpty() would look nicer.
return items.size() == 0; -> return items.isEmpty();
|
|
||
| @Override | ||
| public String toString() { | ||
| return "PHList" + items; |
There was a problem hiding this comment.
return "PHList:" + items; -> return "PHList{items=" + items + "}";
| if (handler != null) { | ||
| insert(++ix, handler); | ||
| } |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
yes, it's important to store the priority.
this is an implementation detail
| insert(++ix, handler); | ||
| } | ||
| } else { | ||
| insert(ix, handler); |
There was a problem hiding this comment.
Is if (handler != null) check required here as well?
| } | ||
| } | ||
| } | ||
| return items.size() == 0; |
There was a problem hiding this comment.
Use of ArrayList.isEmpty() would look nicer.
return items.size() == 0; -> return items.isEmpty();
|
Here are my first batch of comments on the "InputMap (Incubator)" API.
SUGGESTION: Given the above, and since most of the concerns raised about the InputMap API are also around the Behavior and Skins, I recommend that you consider making the Skin / Behavior part of the Input Map incubator module non-public in the first version? You could make it public in v2, which would give more time to address these and other issues. |
Good idea, thanks! These classes were designed to help the custom controls' developers, and are not essential for the purpose of collecting feedback from the rich text application developers. |
|
Thanks, I'll review the now-much-smaller public API surface of the input map module. |
There was a problem hiding this comment.
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?
| * The {@code InputMap} The InputMap provides an ordered repository of event handlers, | ||
| * working together with {@link SkinInputMap} supplied by the skin, which | ||
| * guarantees the order in which handers are invoked. |
There was a problem hiding this comment.
SkinInputMap is not public, so maybe say something like "the input map managed by the Skin" (or similar).
| * <p> | ||
| * The class supports the following scenarios: | ||
| * <ul> | ||
| * <li>map a key binding to a function, provided either by the application or the skin |
There was a problem hiding this comment.
I think you can remove "or the skin" for this version.
| * <li>un-map a key binding | ||
| * <li>map a new function to an existing key binding | ||
| * <li>obtain the default function | ||
| * <li>add an event handler at specific priority (applies to application-defined and skin-defined handlers) |
There was a problem hiding this comment.
There is no API to control the priority (which is a good thing to have eliminated). I would remove this bullet.
|
|
||
| /** | ||
| * Registers a function for the given key binding. This mapping will take precedence | ||
| * over any such mapping set by the skin. |
There was a problem hiding this comment.
Maybe "any such mapping set internally by the skin"? Or "the default mapping set by the skin"?
This comment applies here and elsewhere.
| } | ||
|
|
||
| /** | ||
| * Collects all mapped key bindings (set either by the user or the behavior). |
There was a problem hiding this comment.
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).
|
|
||
| /** | ||
| * Creates a {@link Builder} with the specified KeyCode. | ||
| * @param character the character |
There was a problem hiding this comment.
Since the type is String what if it is more than one character?
There was a problem hiding this comment.
Good point. Added a link to KeyEvent.getCharacter()
| * @param c key code | ||
| * @return Builder instance | ||
| */ | ||
| public static Builder with(KeyCode c) { |
There was a problem hiding this comment.
This is redundant. We don't need both builder and with methods that do exactly the same thing. I recommend removing this one.
| * @param c character pressed | ||
| * @return Builder instance | ||
| */ | ||
| public static Builder with(String c) { |
There was a problem hiding this comment.
This method is redundant -- it's identical to builder(String). I recommend removing it.
| import javafx.event.EventType; | ||
|
|
||
| /** | ||
| * This interface enables wider control in specifying conditional matching logic when adding skin/behavior handlers. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
yes, it can be hidden for now.
| import com.sun.javafx.PlatformUtil; | ||
|
|
||
| /** | ||
| * Key binding provides a way to map key event to a hash table key for easy matching. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
KeyMapping is a totally different beast designed for a different purpose.
KeyBinding is more like KeyCombination, except we cannot use KeyCombination because:
- it does not capture KEY_PRESSED/KEY_TYPED/KEY_RELEASED property
- it has a sole KeyCombination(String) constructor which requires parsing
- it's got a narrow and weird API, unusable for our purpose
Also, this hints at the fact that the documentation needs to be improved, thanks.
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
Progress
Warnings
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1524/head:pull/1524$ git checkout pull/1524Update a local copy of the PR:
$ git checkout pull/1524$ git pull https://git.openjdk.org/jfx.git pull/1524/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1524View PR using the GUI difftool:
$ git pr show -t 1524Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1524.diff
Using Webrev
Link to Webrev Comment