jfx icon indicating copy to clipboard operation
jfx copied to clipboard

8301121: RichTextArea Control (Incubator)

Open andy-goryachev-oracle opened this issue 1 year ago • 8 comments

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, javafx.incubator.controls. 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.

References

[0] Proposal: https://github.com/andy-goryachev-oracle/Test/blob/rich.jep.review/doc/RichTextArea/RichTextArea.md [1] Discussion points: https://github.com/andy-goryachev-oracle/Test/blob/rich.jep.review/doc/RichTextArea/RichTextAreaDiscussion.md [2] API specification (javadoc): https://cr.openjdk.org/~angorya/RichTextArea/javadoc [3] Draft Pull Request for API comments and feedback: https://github.com/openjdk/jfx/pull/1374 [4] RichTextArea RFE: https://bugs.openjdk.org/browse/JDK-8301121 [5] Missing APIs related to rich text control: https://bugs.openjdk.org/browse/JDK-8300569


Progress

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue

Warning

 ⚠️ Patch contains a binary file (tests/manual/RichTextAreaDemo/src/com/oracle/demo/rich/rta/animated.gif)

Issue

  • JDK-8301121: RichTextArea Control (Incubator) (Enhancement - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1374/head:pull/1374
$ git checkout pull/1374

Update a local copy of the PR:
$ git checkout pull/1374
$ git pull https://git.openjdk.org/jfx.git pull/1374/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1374

View PR using the GUI difftool:
$ git pr show -t 1374

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1374.diff

andy-goryachev-oracle avatar Feb 20 '24 22:02 andy-goryachev-oracle

:wave: Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar Feb 20 '24 22:02 bridgekeeper[bot]

❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.

openjdk[bot] avatar Mar 13 '24 20:03 openjdk[bot]

@andy-goryachev-oracle this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8301121.rich.text.area.incubator
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk[bot] avatar Apr 18 '24 22:04 openjdk[bot]

I tried the manual test applications and have few observations: RichTextAreaDemoApp:

  1. Is this app meant to be non editable? I see that the editable checkbox is not changing anything.
  2. There is a Test button in the options pane at the bottom. I couldn't really figure out its usage.
  3. In Mac, horizontal scrolling is not working using the trackpad whereas vertical scrolling is working as expected. This is in all the test apps.

CodeAreaDemoApp:

  1. Selecting the line number checkbox throws NPE and code area becomes blank. Deselecting the checkbox brings back the content.
  2. Changing font size is not changing text size. I observed these issue when I ran the application for the first time. When I relaunched the application, I couldn't reproduce these issues.

In RichEditorDemoApp, bold, underline and italic buttons did not work when the app was launched for the first time. After relaunching the app, only italic button is not working.

karthikpandelu avatar Apr 30 '24 09:04 karthikpandelu

Thank you @karthikpandelu for testing!

  1. some of the issues you've seen with exceptions being thrown are fixed in the later code, I did not want to update in the middle of your review (will update shortly)
  2. the editable checkbox modifies the property in the control, you'd need to have an editable model to be able to edit. the purpose of this property is to disable editing even if the model itself is editable.
  3. Test button appends some text via control's APIs, will fix the button.
  4. The horizontal scrolling is not implemented - totally missed it. Thank you for uncovering this requirements gap!

andy-goryachev-oracle avatar Apr 30 '24 20:04 andy-goryachev-oracle

Thank you @aghaisas for taking a look and asking these questions!

  1. What level of CSS support does the RichTextArea control have? For e.g. TextArea has this level of support - https://docs.oracle.com/javafx/2/api/javafx/scene/doc-files/cssref.html#textarea

A good question.

At the moment, RichTextArea exposes only two properties via CSS:

-fx-content-padding
-fx-wrap-text

While CodeArea adds three more:

-fx-font
-fx-line-spacing
-fx-tab-size

We could add more. The question is - does it make sense to make all the trivial properties styleable? What is the general rule that we should use to make properties styleable?

  1. How is 2B row limit imposed?

Paragraph index is an int.

  1. How is the performance impact as and when model size increases? As it is a sliding window design, I believe, it should become constant at some point of model size.

Correct - for most operations, the performance is a function of the text being laid out in the sliding window. Obviously, very long paragraphs will slow things down. (There is currently a bug where editing the Writing Systems model is slow, I'll be working on that shortly).

Of course, exporting large chunks of text from a very large model will take time proportional to the size of the text being exported. Copying a large selection might throw an OOM Error.

One thing was explicitly added to the layout logic: when useContentHeight is enabled, the layout stops at some arbitrary height so as not to lock up the UI, see Params.MAX_HEIGHT_SAFEGUARD.

andy-goryachev-oracle avatar May 03 '24 23:05 andy-goryachev-oracle

⚠️ @andy-goryachev-oracle This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk[bot] avatar Jun 14 '24 19:06 openjdk[bot]

@aghaisas @kevinrushforth please check out the updated public api and javadoc

https://cr.openjdk.org/~angorya/RichTextArea/javadoc/

andy-goryachev-oracle avatar Jun 14 '24 19:06 andy-goryachev-oracle

Closing this internal review PR in favor of #1524

andy-goryachev-oracle avatar Jul 30 '24 21:07 andy-goryachev-oracle