JDK-8143158 : [Text, TextFlow] Make public API from internal "impl" APIs
  • Type: Enhancement
  • Component: javafx
  • Sub-Component: graphics
  • Affected Version: 9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2015-11-17
  • Updated: 2016-03-15
  • Resolved: 2016-03-14
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 9
9Fixed
Related Reports
Relates :  
Relates :  
Sub Tasks
JDK-8090357 :  
JDK-8136346 :  
JDK-8136350 :  
JDK-8136971 :  
Description
Text and TextFlow contain API with names that include 'impl', which are public but deprecated and meant for internal use only. Some, but not necessarily all, of these should be renamed and made fully supported API.

Approved new API:

New class javafx.scene.text.HitInfo (no public constructor):

public class HitInfo {
    public int getCharIndex();
    public int getInsertionIndex();
    public boolean isLeading();
}


New API in javafx.scene.text.Text (previously known as "impl" methods):

    public final ReadOnlyObjectProperty<PathElement[]> selectionShapeProperty();
    public final PathElement[] getSelectionShape();

    public final IntegerProperty selectionStartProperty();
    public final void setSelectionStart(int value);
    public final int getSelectionStart();

    public final IntegerProperty selectionEndProperty();
    public final void setSelectionEnd(int value);
    public final int getSelectionEnd();

    public final ObjectProperty<Paint> selectionFillProperty();
    public final void setSelectionFill(Paint paint);
    public final Paint getSelectionFill();

    public final ReadOnlyObjectProperty<PathElement[]> caretShapeProperty();
    public final PathElement[] getCaretShape();

    public final IntegerProperty caretPositionProperty();
    public final void setCaretPosition(int value);
    public final int getCaretPosition();

    public final BooleanProperty caretBiasProperty();
    public final void setCaretBias(boolean value);
    public final boolean isCaretBias();

    public final HitInfo hitTest(Point2D point);

    public final PathElement[] caretShape(int charIndex, boolean leading);
    public final PathElement[] rangeShape(int start, int end);
    public final PathElement[] underlineShape(int start, int end);

New API in javafx.scene.text.TextFlow:

    public final PathElement[] caretShape(int charIndex, boolean leading);

    public final HitInfo hitTest(Point2D point);

    public final PathElement[] rangeShape(int start, int end); 
Comments
Changeset: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/7d541576bab0
14-03-2016

API changes look good. Approved. Running gradle test fails to compile the system tests: text.setImpl_selectionStart(1); ^ symbol: method setImpl_selectionStart(int) location: variable text of type Text Note: Some input files use unchecked or unsafe operations. Note: Recompile with -Xlint:unchecked for details. 100 errors :systemTests:compileTestJava FAILED :systemTests:compileTestJava (Thread[main,5,main]) completed. Took 4.347 secs. This will need to be fixed prior to pushing.
11-03-2016

I'll do one more quick pass on the API, but I think so. I'll let you know in a few (got distracted with Jigsaw stuff)
11-03-2016

So, are we good to go?
11-03-2016

Good. I wasn't able to nail down when it had a problem versus when it was OK, but I know on some of Jonathan's new docs, it didn't show up until moved to the xxxProperty().
11-03-2016

I have verified that the generated javadoc does have the new public properties listed correctly in the Properties section.
11-03-2016

Actually, looking at the javadoc for FX 8, it is picking up the property doc even if it isn't directly tied to the xxxProperty() method. Look at the text property, for example. I'll leave it as is for now, to keep the diff more readable, and save the doclint clean-up for later.
11-03-2016

I'll move the comments to the xxxProperty() methods.
11-03-2016

We generally do omit the @param and @return tags for properties (although there may be some value in having them). More importantly, though, the javadoc needs to be associated with the xxxProperty() method itself and not on either the setter or the getter. In some cases doing otherwise will prevent the docs from showing up.
10-03-2016

As a follow-up to this, we have a lot of work to do to make FX docs doclint-clean.
10-03-2016

> Don't we usually omit the @param and @return tags for properties? This is something I was trying to remember yesterday. I think the doclet we use does the generation when it recognises a property. But I'd like Kevin to confirm this. And I wonder what doclint will think about this ?
10-03-2016

Don't we usually omit the @param and @return tags for properties?
10-03-2016

> Added missing @param and @return tags to Text and TextFlow. Strange .. I don't see these all having been updated. For example see this in Text : 800 */ 801 public final void setSelectionStart(int value) { 802 if (value == -1 && 803 (attributes == null || attributes.impl_selectionStart == null)) { 804 return; 805 } 806 selectionStartProperty().set(value); 807 } 808 809 public final int getSelectionStart() { 810 if (attributes == null || attributes.impl_selectionStart == null) { 811 return DEFAULT_SELECTION_START; 812 } 813 return attributes.getImpl_selectionStart(); 814 } 815 816 public final IntegerProperty selectionStartProperty() {
09-03-2016

Added missing @param and @return tags to Text and TextFlow. Synchronized the use of the BreakIterator in HitInfo. I'm not sure whether reusing a synchronized instance really is more efficient than creating one per HitInfo object, though. New webrev: http://cr.openjdk.java.net/~leifs/8143158/webrev.02/
09-03-2016

I'll flesh out the javadoc and clean up as suggested. Answers to questions: > Where do we get the magic numbers like the "-6" here in TextAreaSkin ? > > 402 Point2D p = new Point2D(e.getSceneX() - tp.getX() - pressX + caretHandle.getWidth() / 2, > 403 e.getSceneY() - tp.getY() - pressY - 6); This is a fudge for hit testing on the touch handles. This needs to be normalized. Please file a new bug as it is not related to this fix. > Looking at > public HitInfo getIndex(double x, double y) { > the old version used to call > textNode.setImpl_caretPosition(oldPos) > > whereas I don't see that in the new version. > > So the old version seems like it updated state and the new version just queries. > Can you explain that ? This was a temporary setting of the caret pos to test whether the caret would end up on the previous line. The pos was then restored to the saved value. This is not needed after the caret bias logic was improved.
08-03-2016

Public API for review. [ Edit 2015-11-29: Removed setCaretShape() because the caretShape property is read-only. ] [ Edit 2015-11-30: Replaced TextFlow getCaretShape() and caretShapeProperty() with caretShape(int, boolean). Does not affect Text. ] [ Edit 2016-02-01: Added caretShape() to Text for completeness. ] [Edit 2016-03-08: Moved approved API list to issue description above.]
08-03-2016

Comments on http://cr.openjdk.java.net/~leifs/8143158/webrev.01/index.html : The public API methods should have appropriate @param and @return tags And more javadoc in general. And it would be 'nice' to have a summary of the API being added. There was a bit more than I had realised, eg the shape & fill properties/getters/setters. - The singleton BreakIterator in the HitTest class does not appear to have any protection against being used from multiple threads. Where do we get the magic numbers like the "-6" here in TextAreaSkin ? 402 Point2D p = new Point2D(e.getSceneX() - tp.getX() - pressX + caretHandle.getWidth() / 2, 403 e.getSceneY() - tp.getY() - pressY - 6); Looking at public HitInfo getIndex(double x, double y) { the old version used to call textNode.setImpl_caretPosition(oldPos) whereas I don't see that in the new version. So the old version seems like it updated state and the new version just queries. Can you explain that ? TextFlow has chunks of commented out code. Please clean this up. ---
07-03-2016

New webrev with fixed test. http://cr.openjdk.java.net/~leifs/8143158/webrev.01/ The demo HelloTextFlow exposes a problem with selecting text across Text elements. The rangeShape for drawing the bg is correct, but Text nodes after the first one will fail to highlight the fg (should be white). This is an already existing bug from FX 8, so I will file a separate issue for it.
04-02-2016

Thanks, I'll fix that test. I will also file an issue to write unit tests for the new API.
01-02-2016

I'll take a detailed look later. As a quick comment, the unit tests fail to compile after applying the patch: :graphics:compileTestJavaC:\Users\kcr\javafx\9-kcr\jfx\rt\modules\graphics\src\test\java\test\com\sun\javafx\pgstub\StubTextLayout.java:136: error: constructor Hit in class Hit cannot be applied to given types; return new Hit(0, -1, true, 0F); ^ required: int,int,boolean found: int,int,boolean,float reason: actual and formal argument lists differ in length C:\Users\kcr\javafx\9-kcr\jfx\rt\modules\graphics\src\test\java\test\com\sun\javafx\pgstub\StubTextLayout.java:157: error: constructor Hit in class Hit cannot be applied to given types; return new Hit(offset + charPos, -1, true, 0F); ^ required: int,int,boolean found: int,int,boolean,float reason: actual and formal argument lists differ in length ... 2 errors ... FAILURE: Build failed with an exception.
01-02-2016

I added caretShape(int, boolean) to Text also, as suggested. Webrev for review: http://cr.openjdk.java.net/~leifs/8143158/webrev.00/
01-02-2016

Yes, it would be easy to add caretShape(int, boolean). It would just result in a call to TextLayout.getCaretShape(...). I wonder, though, if we might consider making TextLayout public at some point, in which case this would be a bit redundant. That would probably not happen for 9, however.
29-01-2016

The proposed API looks fine to me. We can add any additional API needed. Do you think it makes sense to also add the caretShape(int, boolean) to Text for consistency with TextFlow? I realize you can get the same functionality without it, but perhaps not as conveniently. Anyway I am OK with or with that addition, given that we could always add it later if needed. The important thing is that we don't paint ourselves into a corner, and it looks like this API doesn't. So I approve the API as is or with the additional caretShape method on Text.
29-01-2016

This was on hold for a bit while we looked at a richer API for TextFlow. We have to be careful not to cause unnecessary event conflicts with the Text children, so I want to stick with the proposed API above for now. We'll need a new JIRA issue to follow-up if a richer API is desired.
29-01-2016

Good catch. In the interest of adding a minimum of new implementation code, and since users already depend on an observable caretShape property in Text, but not in TextFlow, I've updated the proposal and changed the TextFlow API as you suggested. The discrepancy between Text and TextFlow should be addressed in a later release, where we will hopefully have a supported API for accessing the underlying text layout info.
30-11-2015

For TextFlow, if there are no setCaretPosition and setCaretBias methods, then the caretShapeProperty()/getCaretShape() methods are useless. As voiced before elsewhere, I would actually prefer caretShape(int caretPosition, boolean caretBias) and then caretPositionProperty() and caretBiasProperty() become unnecessary.
30-11-2015