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)
|
I have finished reviewing the model part and aside of small nitpicks that were either dealt with or clarified i haven't found any problems. |
Are you saying that the mechanism in CodeArea:92 is not suitable? edit: not suitable. added |
|
There is another alternative: we can declare RichTextArea, then CodeArea becomes CodeArea. This might actually be a cleaner solution. edit: this idea did not work. |
| @@ -682,6 +690,16 @@ public final StyledTextModel getModel() { | |||
| return model == null ? null : model.get(); | |||
| } | |||
|
|
|||
| /** | |||
| * Validates the model property value. | |||
| * The subclass should override this method to check if the model type is supported and throw a TBD if not. | |||
There was a problem hiding this comment.
"TBD" --> "IllegalArgumentException".
Also, you might want to add something like: "... should override this method if it restricts the type of model that is supported ..." (since a subclass that takes all types of model need not override it).
| /** | ||
| * Validates the model property value. | ||
| * The subclass should override this method to check if the model type is supported and throw a TBD if not. | ||
| * A {@code null} value should never generate the exception. |
There was a problem hiding this comment.
"the exception" --> "an exception".
| @@ -65,6 +65,7 @@ public interface Content { | |||
|
|
|||
| /** | |||
| * This method is called to insert a single text segment at the given position. | |||
| * The caller guarantees that this method is only called when the content is writable. | |||
There was a problem hiding this comment.
Since this is a public method that could be called by an application, I think you still need to describe what happens if the caller does call when not writable.
There was a problem hiding this comment.
the implementor of the Content can do whatever they want - I don't think we should specify that.
In the case of BasicTextModel.Content, I think it's important to mention that the caller (the BasicTextModel) will never do.
maybe caller -> BasicTextModel
Also, I think there should be some code added to the RTA/StyledTextModel to ensure that paragraph indexes and TextPos'itions are never go beyond the range allowed by the model (it's in my list).
| * Because it is immutable, it cannot track locations in the document which is being edited. | ||
| * For that, use {@link Marker}. | ||
| */ | ||
| public final class TextPos implements Comparable<TextPos> { |
There was a problem hiding this comment.
A record seems more natural here, so would be the default choice, but if there is a good reason not to, I don't see it as a requirement.
| * @param offset the text offset | ||
| * @return the instance of {@code TextPos} | ||
| */ | ||
| public static TextPos ofLeading(int index, int offset) { |
There was a problem hiding this comment.
Do you also plan to add an ofTrailing convenience method?
There was a problem hiding this comment.
not necessary - this case is more complex and is covered by the constructor.
| @@ -5795,6 +5993,7 @@ compileTargets { t -> | |||
| def modnames = [] | |||
| moduleProjList.each { project -> | |||
| if (project.hasProperty("moduleName") && project.buildModule) { | |||
| def incubating = project.hasProperty("incubating") && project.ext.incubating | |||
| @@ -99,10 +99,10 @@ public class Params { | |||
| public static final double MIN_VIEWPORT_WIDTH = 10; | |||
|
|
|||
| /** default preferred height */ | |||
| public static final double PREF_HEIGHT = 100; | |||
| public static final double PREF_HEIGHT = 176; // matches TextArea | |||
There was a problem hiding this comment.
I see 181 as TextArea height, at least on Windows. I think it is computed from the default number of columns visible. I didn't check on macOS.
There was a problem hiding this comment.
it is not the goal to match the TextArea, especially if the latter is different on different platforms.
|
|
||
| /** default preferred width */ | ||
| public static final double PREF_WIDTH = 200; | ||
| public static final double PREF_WIDTH = 176; // matches TextArea |
There was a problem hiding this comment.
I see 478 for TextArea width on Windows.
| * | ||
| * <h2>Usage Considerations</h2> | ||
| * {@code CodeArea} extends the {@link RichTextArea} class, meaning most of the functionality is expected to continue | ||
| * working. There are some differences that should be mentioned: |
There was a problem hiding this comment.
Suggestion: "... meaning most of the functionality works as it does in the base class" or similar?
| * <li>Model behavior: any direct changes to the styling, such as | ||
| * {@link #applyStyle(TextPos, TextPos, jfx.incubator.scene.control.richtext.model.StyleAttributeMap) applyStyle()}, | ||
| * will be ignored | ||
| * <li>Line numbers: the line numbers are provided by setting the {@link #leftDecoratorProperty()} |
There was a problem hiding this comment.
This could be misconstrued to mean that the app should set the leftDecoratorProperty to provide line numbers. I would reword it to make it clear that CodeArea itself sets leftDecoratorProperty so applications should not.
|
Currently we use RichTextFX and the problem is that it doesn't support caret form (see FXMisc/RichTextFX#1121) . I've just checked RichTextArea CSS guide and found there |
Thank you for the feedback! I wanted not to complicate the first round with different carets. Nevertheless, it should be possible to either support some of the caret types out of the box, or allow for customization. The default caret shape may need to depend on the input locale and/or presence of RTL or mixed text - something is yet to be fully addressed due to existing bugs, see https://bugs.openjdk.org/browse/JDK-8343557 Another topic is the "block caret", which, in my opinion, only makes sense with the monospaced font, maybe in the And lastly, Also (you know this already, but the wider audience may not): there is ongoing work in https://bugs.openjdk.org/browse/JDK-8341670 to expose more detail about the caret geometry (among other things) that should help third party devs with creating custom carets. Thanks again! |
|
/issue add 8343646 |
|
@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();
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