8301121: RichTextArea Control (Incubator) #1524
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
|
@andy-goryachev-oracle This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 8 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
|
/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
modules/javafx.graphics/src/main/docs/javafx/scene/doc-files/cssref.html
Show resolved
Hide resolved
| 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)
| * {@link #registerKey(KeyBinding, FunctionTag)}, | ||
| * or registered by the skin. |
There was a problem hiding this comment.
The "or registered by the skin" part seems doesn't seem right to me. Is that really what happens? And if so, why is it the right thing to do?
There was a problem hiding this comment.
this call disables the key binding, regardless of who registered it.
the use case example is Eclipse's "Unbind Command":
| @@ -360,7 +368,7 @@ private Set<KeyBinding> collectKeyBindings(FunctionTag tag) { | |||
| * @param tag the function tag | |||
| */ | |||
| // TODO this should not affect the skin input map, but perhaps place NULL for each found KeyBinding | |||
| public void unbind(FunctionTag tag) { | |||
| public void unregister(FunctionTag tag) { | |||
There was a problem hiding this comment.
For symmetry (and better clarity), I still think the name should have KeyBindingsFor in the name, perhaps removeKeyBindingsFor or unregisgterKeyBindingsFor.
| * | ||
| * @param k the key binding | ||
| */ | ||
| public void unbind(KeyBinding k) { | ||
| public void unregister(KeyBinding k) { | ||
| map.put(k, NULL); |
There was a problem hiding this comment.
Why does this add a NULL rather than remove the mapping? I'm trying to understand the difference between this method and resetKeyBindings (maybe this is related to my earlier question above?). One thing to consider is that allowing NULL values means that there is no way to tell the difference between a key with a NULL value and a key that isn't in the map without calling map.contains, which you don't expose.
There was a problem hiding this comment.
see my previous comment for the use case.
this method disables the key binding, regardless of whether it's done programmatically by the application (as in constructor of the custom RTA), or the skin.
if we are talking about implementation, the NULL basically blocks subsequent lookup in the skin input map.
There was a problem hiding this comment.
I see. In that case, unregister is not a good name, since it is not the inverse of register. What it is really doing is registering a no-op binding. In which case... why is it needed at all? The application can just as easily do this:
register(keyBinding, () -> {});
If there really is a compelling reason to keep this method, then we need to think of a better name. Maybe "disable" or "disableKeyBinding"?
| @@ -207,13 +207,13 @@ public static class Builder { | |||
| } | |||
|
|
|||
| /** | |||
| * Adds a squiggly line (as seen in a spell checker) with the given color. | |||
| * Adds a wave underline (typically used as a spell checker indicator) with the given color. | |||
There was a problem hiding this comment.
Minor: "wave" --> "wavy"
| * @param start the start offset | ||
| * @param length the end offset | ||
| * @param color the background color | ||
| * @return this {@code Builder} instance | ||
| */ | ||
| public Builder addSquiggly(int start, int length, Color color) { | ||
| public Builder wavyUnderline(int start, int length, Color color) { |
There was a problem hiding this comment.
Should the name start with "add" (e.g., to match addHighlight)?
| @@ -129,7 +129,7 @@ protected void removeRange(TextPos start, TextPos end) { | |||
|
|
|||
| @Override | |||
| protected void insertParagraph(int index, Supplier<Region> generator) { | |||
| // TODO | |||
| throw new UnsupportedOperationException(); | |||
There was a problem hiding this comment.
Should this be documented?
| * The caller must ensure that the {@code text} does not contain newline symbols, as the behavior might be | ||
| * undefined. |
There was a problem hiding this comment.
Minor: I would say "is undefined" (instead of "might be") meaning that we don't define or specify the behavior.
| @@ -184,14 +188,14 @@ public SimpleViewOnlyStyledModel highlight(int start, int length, Color c) { | |||
| } | |||
|
|
|||
| /** | |||
| * Adds a squiggly line (typically used as a spell checker indicator) to the specified range within the last paragraph. | |||
| * Adds a wave underline (typically used as a spell checker indicator) to the specified range within the last paragraph. | |||
There was a problem hiding this comment.
Minor: "wave" --> "wavy"
| @@ -135,7 +137,7 @@ public interface Listener { | |||
| /** | |||
| * Returns the plain text string for the specified paragraph. The returned text string cannot be null | |||
| * and must not contain any control characters other than TAB. | |||
| * The caller should never attempt to ask for a paragraph outside of the valid range. | |||
| * The caller must not attempt to ask for a paragraph outside of the valid range. | |||
There was a problem hiding this comment.
It might be worth adding the note about "doing otherwise might result in an exception..." as you mentioned below.
| @@ -144,10 +146,13 @@ public interface Listener { | |||
|
|
|||
| /** | |||
| * Returns a {@link RichParagraph} at the given model index. | |||
| * The callers must ensure that the value of {@code index} is within the valid document range, | |||
| * since doing otherwise might result in an exception or undetermied behavior. | |||
There was a problem hiding this comment.
Typo: "undetermied" --> "undetermined"
|
Refreshed the javadoc/apidiff attachments in the CSR. |
|
Thank you, @arapte ! |
|
Raising the limit to include my "R"eviewing of:
/reviewers 4 reviewers |
|
@lukostyra |
There was a problem hiding this comment.
Checked my sections and gave it some random testing, all looks good. Good work @andy-goryachev-oracle !
| * | ||
| * @since 24 | ||
| */ | ||
| public class KeyBinding |
There was a problem hiding this comment.
There's also existing classes like javafx.scene.input.KeyCodeCombination, which have a different API. Will it increase maintenance burden and/or cause more friction for users to have two different APIs for a very similar purpose? Are there plans to consolidate these APIs in the future?
There was a problem hiding this comment.
Good question!
I've decided to create a new class for the incubator for two main reasons:
- the only way to construct
KeyCombinationis from aString, which incurs the penalty of parsing KeyCombinationdoes not differentiate key pressed / key released / key typed events
While 1) can be easily addressed by adding a Builder, introducing 2) may cause issues with the existing code, since KeyCombination is an existing public API.
Any suggestions?
There was a problem hiding this comment.
KeyCombination is just the abstract base class. Subclasses like javafx.scene.input.KeyCodeCombination provide normal constructors without any parsing:
public KeyCodeCombination(final @NamedArg("code") KeyCode code,
final @NamedArg("modifiers") Modifier... modifiers) { /* ... */ }The abstract class provides a match method which subclasses can (and do) override, so it could be possible to create a subclass that implements the method based on pressed/typed:
@Override
public boolean match(final KeyEvent event) {
// KeyEvent has e.g. KEY_PRESSED and KEY_RELEASED, so should be possible to check here...
return checkPressedReleasedTyped(event) && super.match(event);
}There was a problem hiding this comment.
You are right, in theory it is possible to coerce KeyCombination to do what's needed.
In practice, it's going to be rather clunky. The main issue is that we can't use KeyCombination in the API calls because the base class has no concept or key pressed/released/typed. Basically, we would need to do one of the two things:
- create a child class(es) of
KeyCombinationthat contain the event type and accept only those, or - accept
KeyCombinationbut do a check at runtime of whether the right class is passed, throwing an exception or ignoring the whole thing which is not optimal.
Another alternative is to add three more modifiers (something like KeyPressed, KeyReleased, KeyTyped), maybe defaulting to KeyPressed when none is specified.
Doing this, however, brings up another issue of possible backward compatibility when the new KeyCombination is used elsewhere - for example, what happens when the MenuItem::setAccelerator is called with a KeyTyped or KeyReleased modifier? The fact that the modified KeyCombination now encodes the type of the event may cause the existing code to misbehave, and we don't want to introduce regression.
All that, and the fact that we already have a non-public KeyBinding class for the purposes of the existing non-public InputMap, is basically the rationale behind the proposed design.
| USER_HIGH(6000), | ||
| USER_KB(5000), | ||
| SKIN_KB(4000), | ||
| SKIN_HIGH(3000), |
There was a problem hiding this comment.
Just curious about the logic behind the priorities. Like why for skin kb (is it keyboard?) if above the high priority but for user high is about kb? I understand that this is just a search order in which event handlers will be called and if higher priority handler consumes event it will not be proposed to the next priority level handler?
There was a problem hiding this comment.
excellent question!
the idea is to have the handler for key bindings (KB) to be checked before other skin handlers, which typically deal with multiple keys or catch-all cases.
|
❤️ |
There was a problem hiding this comment.
Nice work! Approved with some feedback that can be addressed in follow-up issues. I left a couple comments inline and have a few general comments below.
Functional issues
I noticed the following minor functional bugs:
- Default width of RTA is less than half the size of TextArea (176 vs 478 on my Windows 11 system). Earlier the height was also too small, but you already fixed that.
- When running the demos, the current line highlight overlaps and obscures focus outline on right side (verified on Windows and Linux).
I can file the above bugs, unless you want to.
Tests
We need more automated unit tests. Please file follow-up test bugs to create additional tests for each of the following:
- RichTextArea API tests
- RichTextArea behavioral tests
- InputMap tests
Demos
Please file the following demo build issues:
- Ability to build and run the samples easily from the command line (possibly via ant / gradle). They are modular apps, so it isn't currently straight-forward.
- Consider hooking the samples up to the build (so they are built as part of the "gradle apps" task. This depends on the previous.
| * <pre>{@code | ||
| * SimpleViewOnlyStyledModel m = new SimpleViewOnlyStyledModel(); | ||
| * // add text segment using CSS style name (requires a stylesheet) | ||
| * m.addWithStyles("RichTextArea ", "HEADER"); |
There was a problem hiding this comment.
This example needs to be updated to reflect an earlier change in the API:
addWithStyles --> addWithStyleNames
| @@ -0,0 +1,218 @@ | |||
| /* | |||
There was a problem hiding this comment.
This class (and thus the entire test/** directory tree) is unused.
| // TODO remove once a real test which needs the shim is added. | ||
| @Test | ||
| public void testShim() { |
There was a problem hiding this comment.
Can you add a note to the follow-up bug that you will file to track adding these tests?
|
/integrate |
|
Going to push as commit 7312ad1.
Your commit was automatically rebased without conflicts. |
|
@andy-goryachev-oracle Pushed as commit 7312ad1. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
Thank you all for taking time and effort testing, reviewing, and offering your help and providing suggestions! The followup for jfx24 will be addressed here - JDK-8347305 . I expect more bugs to come in from application developers as they start playing with the demos and code. Other follow-up tickets:
|
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