Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8301121: RichTextArea Control (Incubator) #1374

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

andy-goryachev-oracle
Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle commented Feb 20, 2024

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: #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)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

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

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 20, 2024

👋 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.

@openjdk
Copy link

openjdk bot commented Mar 13, 2024

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

@openjdk
Copy link

openjdk bot commented Apr 18, 2024

@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 openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Apr 18, 2024
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Apr 18, 2024
@karthikpandelu
Copy link
Member

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.

@andy-goryachev-oracle
Copy link
Contributor Author

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!

Copy link
Collaborator

@aghaisas aghaisas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started taking a look. A few general 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

  2. How is 2B row limit imposed?

  3. 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.

public static final StyleAttribute<Double> LINE_SPACING = new StyleAttribute<>("LINE_SPACING", Double.class, true);

/** Right-to-left orientation paragraph attribute */
public static final StyleAttribute<Boolean> RIGHT_TO_LEFT = new StyleAttribute<>("RIGHT_TO_LEFT", Boolean.class, true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be ORIENTATION and it's value either be RIGHT_TO_LEFT or LEFT_TO_RIGHT (default)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question. Originally, it was intended as a simple boolean flag, but really it might have 3 states: LTR, RTL, INHERIT (or null), as a way to explicitly override the paragraph orientation.

We still lack proper support for RTL/mixed text, so perhaps I ought to make this attribute private until we get RTL fixed.


/**
* An immutable object containing style attributes.
* TODO name: StyleSet? (though it's not a set)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some attributes listed under StyleAttrs are related to Paragraph and some are related to text (like font size etc). We can segregate them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you suggest?
Place in different classes, re-arrange in two blocks, or something else?

@andy-goryachev-oracle
Copy link
Contributor Author

andy-goryachev-oracle commented May 3, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants