JDK-8136971 : Review the new TextPosInfo API
  • Type: Sub-task
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: 9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2015-09-22
  • Updated: 2016-03-15
  • Resolved: 2016-03-15
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 :  
JDK-8092327 - [Text] Add API to count chars in a cluster
Relates :  
JDK-8091012 - [Text] No way to distinguish between a hit to the last character and a hit beyond the end of text
Relates :  
JDK-8136350 - [TextFlow] Hit test API for TextFlow
Relates :  
JDK-8077916 - Complete development work for project one: make UI control skins public API
Description
(This is a follow-up to JDK-8077916.)

javafx.scene.control.skin.TextAreaSkin.TextPosInfo needs to be changed to take character clusters into account, as noted in JDK-8092327, point 2. 
As it currently stands, TextPosInfo#getInsertionIndex() is broken: it may return an index inside a character cluster. Indeed, the skins themselves don't use TextPosInfo#getInsertionIndex(), but rather Utils#getHitInsertionIndex(TextPosInfo hit, String text), which is not available to the users, since Utils is in com.sun.javafx.scene.control.skin. Also note that Utils#getHitInsertionIndex in order to work around the limitation of TextPosInfo, needs to see the text again. I think the right thing to do is to make sure the returned TextPosInfo is correct, otherwise it is useless for anything but private implementation again. 

The most reasonable place to fix this seems to be to fix the hit test implementation on Text, rather than keep it broken and then work around it in text input controls. With that, it would also make more sense to put this class in the javafx.scene.text package.

Also note that the current version of TextPosInfo API is good for positioning caret, but not for getting the character under the cursor (JDK-8091012), which is useful e.g. for displaying a tooltip based on the word under the cursor. As I suggested in JDK-8136350, I think a reasonable API for TextPosInfo would be 

    public OptionalInt getCharacterIndex(); 
    public int getInsertionIndex(); 

Note the OptionalInt, to determine whether any character was hit at all.
Comments
Since we're trying to avoid adding new API implementation where existing methods that will now be made public will suffice, I'd say we'll leave getCaretShape(int pos, boolean bias) for another enhancement. You can achieve the same results by setting the caret pos and bias and then calling getCaretPosition(). > [...] the following in Leif's list should be examined to see if we can come up with a better name: > public final PathElement[] getRangeShape(int start, int end); > public final PathElement[] getUnderlineShape(int start, int end); These can be named without the get instead: public final PathElement[] rangeShape(int start, int end); public final PathElement[] underlineShape(int start, int end);
25-11-2015
Along those same lines, the following in Leif's list should be examined to see if we can come up with a better name: public final PathElement[] getRangeShape(int start, int end); public final PathElement[] getUnderlineShape(int start, int end); It isn't a hard-and-fast rule, but it can avoid confusion.
24-11-2015
We generally avoid "getXxxxx" methods that take arguments, especially in classes that also have properties. If we need this for 9 (I'm not sure we do...Leif can comment), then computeCaretShape(int,int) would be a better name.
24-11-2015
> Isn't the missing method you mention just another of the stateless ones? Yes. Here is its possible implementation, adapted from http://hg.openjdk.java.net/openjfx/9-dev/rt/file/a294957d1c43/modules/graphics/src/main/java/javafx/scene/text/Text.java#l1779 public final PathElement[] getCaretShape(int pos, boolean bias) { int length = getTextInternal().length(); if (0 <= pos && pos <= length) { float x = (float)getX(); float y = (float)getY() - getYRendering(); TextLayout layout = getTextLayout(); return layout.getCaretShape(pos, bias, x, y); } return EMPTY_PATH_ELEMENT_ARRAY; } It's just factoring it out of that caretShapeProperty(), so it's not really a "new" one.
24-11-2015
The "new" API is all based on existing internal implementations that the FX controls depend on. I'm sure many external users have been depending on this unsupported internal API for a while. So the stateless version would have to be the ones to be added. We just don't have enough resources available to have completely new API reviewed this week and implemented by next Friday (feature freeze). Isn't the missing method you mention just another of the stateless ones?
24-11-2015
Not sure we understand each other.All I say is that a subset of the methods you proposed is sufficient (plus the one which I guess got omitted by accident). The other ones, that imply that all Text nodes will have additional state (like selection range, caret position), can be implemented on top of the stateless methods (e.g. in a custom subclass of Text). But if you wish to add them all to public API, that will just be more API to maintain.
24-11-2015
Here's the full set of methods converted to public API in Text for 9. 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 void setCaretShape(PathElement[] shape); 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[] getRangeShape(int start, int end); public final PathElement[] getUnderlineShape(int start, int end);
24-11-2015
Thee stateful properties are useful for binding to. I agree that stateless props can be good to have as well, but this kind of redesign must be outside the scope of this issue.
24-11-2015
Maybe just an oversight, but shouldn't there also be public final PathElement[] getCaretShape(int position, boolean bias); ? I would actually be happy for just the state-less methods, i.e. public final HitInfo hitTest(Point2D point); public final PathElement[] getRangeShape(int start, int end); public final PathElement[] getUnderlineShape(int start, int end); public final PathElement[] getCaretShape(int position, boolean bias); I don't think Text should keep track of, e.g. caret position, especially if Text is not displaying the caret itself.
24-11-2015
Sorry, I'm going to have to withdraw support for "no hit" for this release. There isn't enough time, and there are workarounds. I will post an easier workaround in JDK-8091012. So, we're left with just the one method for hit testing.
24-11-2015
> This is the current behavior. I hoped you were going to incorporate the ability to tell whether a character was hit. Can we try to work out the idea of using two versions of hitTest() instead? Let's do it in JDK-8090357 and JDK-8136350 and maybe we can come to a conclusion for 9.
24-11-2015
> I'm still not convinced about the need to signal "no hit" together with a valid insertionIndex. There's a use case for both. I don't need them both at the same time though. If these are two different API calls, that is fine with me as well. > The discussion in JDK-8091012 seems to suggest that we should open up some TextLayout info instead. The workaround proposed there (and a version of which I am using in RichTextFX), is quite low-level. > For the the use case above, the first hit is (charIndex: n, leading: false) and the second is (charIndex: n+1, leading: true). The insertionIndex will be the same. This is the current behavior. I hoped you were going to incorporate the ability to tell whether a character was hit.
24-11-2015
I think isLeading() must refer to charIndex, since that is supposed to be the character that was hit (or the closest one). It's possible that isLeading() is redundant, since it currently is the same as (getCharIndex() != getInsertionIndex()). I just wanted to preserve the current logic to avoid regressions, as I'm not familiar with the underlying TextLayout code. I'm still not convinced about the need to signal "no hit" together with a valid insertionIndex. The discussion in JDK-8091012 seems to suggest that we should open up some TextLayout info instead. Or maybe an alternative hitTest() that takes a max distance for a hit. Then you'd use the current test for placing the caret and the distance based test for the tooltip use case. Unfortunately, I don't think either solution will make it into 9. For the the use case above, the first hit is (charIndex: n, leading: false) and the second is (charIndex: n+1, leading: true). The insertionIndex will be the same.
24-11-2015
Should isLeading() relate to the insertion index instead of char index? The reason is that insertion index is always defined, since char index can be -1 if no character was hit. Use case: consider a line wrapped between characters n and n+1. What will the HitInfo look like for a click slightly to the right of n-th character? What will it look like for a click slightly to the left of the (n+1)-th character? How can we tell the two HitInfos apart?
24-11-2015
This seems quite reasonable.
23-11-2015
Based on the discussion so far, I suggest the following simplified API for 9, with the possibility of adding more if needed in a later release. Note that there is no public constructor now. public class HitInfo { public int getCharIndex(); public int getInsertionIndex(); public boolean isLeading(); }
23-11-2015
That would work. Though I'm not convinced that the offset method should need to involve BreakIterator. If the user needs to tamper with the HitInfo, it seems more like the hit test did not return a good HitInfo in the first place.
19-11-2015
Yeah, that makes sense. So, maybe an immutable class with a package private constructor that takes a Text or TextFlow reference and calculates the insertion index lazily if it's not provided to the constructor? We might want a public factory method to get a new HitInfo from another, with an offset that is passed to BreakIterator. Like, "back up one symbol" or "leading pos of next symbol".
18-11-2015
If you consider TextPosInfo that would be usable with many different Strings, it makes sense. However, I think the HitInfo use case is always tight to one specific String. > If the insertion index must be stored and a text editor wants to create a HitInfo, then it would need a way to calculate the new insertion index. This would require API somewhere, like in HitInfo/Text/TextFlow, and is why I prefer to calculate it on demand instead. There could be a static helper method for that somewhere, but we can as well be conservative and for the next release go with package private constructor only. And if that is the case, it can as well be an interface. Here's another minor consideration: getInsertionIndex(String text) requires to have the whole string in hand. However, hit test can, in some cases, be implemented without holding the whole text. For example, if TextFlow maintains a list of Lines, then it is, in principle, able to calculate the HitInfo without having to concatenate the Lines into a single String. If the HitInfo is then used to select text, the selection highlight can again be computed without building the whole String. So the complete string does not have to be calculated at all. While I don't think this would ever cause performance problems in any application, there's a part to it that does not feel right. If creating and invoking BreakIterator seems unnecessary, it could possibly be done lazily. In that case, HitInfo would need to keep the reference to text (text of a single Line of a TextFlow would suffice). Considering that HitInfo is only meaningful with respect to the current state of the text control, memory leak is not an issue, since HitInfo would not live past the current state of the control.
18-11-2015
I was envisioning the class to represent a text position with leading/trailing info in a more general sense, which is why I was going with the name TextPosInfo for a while. If the insertion index must be stored and a text editor wants to create a HitInfo, then it would need a way to calculate the new insertion index. This would require API somewhere, like in HitInfo/Text/TextFlow, and is why I prefer to calculate it on demand instead. I'm not sure which way gives better performance. We currently don't use the insertion index when dragging, so it doesn't get called with a high frequency. I don't think BreakIterator is expensive but it just seems unnecessary when the value is not used. If we used insertion index instead when dragging, it would still only be requested once per hit.
18-11-2015
HitInfo contains information about some event (hit test), valid for the instant when the event occurred. If, for example, I store a click event (MouseEvent), then later I can't expect it to be valid, e.g. I can't expect that the node event.getSource() still contains the point [event.getScreenX(), event.getScreenY()].
18-11-2015
> Also, if the data length is stored, then the HitInfo object will become invalid whenever the associated text content changes. After text change, isn't it invalid for all reasonable use-cases anyway?
18-11-2015
> I don't see a difference between a null value and a negative value here. Couldn't we have -1 and a meaningful insertion index? Yes, that would work. But in your sample above HitInfo does not store insertion index separately. (My comment was not related to OptionalInt vs. possibly negative int, but to not keeping track of insertionIndex explicitly.) > For example, TextFieldSkin and TextAreaSkin modifies the hit to back up when a hit happens past a newline. Maybe that can be done some other way. Am I wrong that they can create a new instance, without the need to re-run BreakIterator? > That was for the cases when another class creates a HitInfo (or mutates it). I guess we can do it the way you suggest, but then the constructor would have to be package-scope only. Not necessarily. > Won't there be a performance impact, though, if the data length has to be pre-calculated for every hit test, even when the insertion point is not needed? I'm thinking specifically about dragging to select, where we only use charIndex. Shouldn't dragging to select use insertion index instead of charIndex? I don't know the implementation details of BreakIterator, but by the nature of the problem, calculating insertion index should not depend on the length of text, so should be constant time. Am I right?
18-11-2015
Also, if the data length is stored, then the HitInfo object will become invalid whenever the associated text content changes.
18-11-2015
> does not allow to represent a hit where no character was hit, but there still is a meaningful insertion index, as in a click before the line start or after the line end. I don't see a difference between a null value and a negative value here. Couldn't we have -1 and a meaningful insertion index? >I don't understand the argument for keeping HitInfo mutable, name I don't understand how other classes "depend" on mutating the fields. Would you mind sharing an example of how another class needs to update the fields? Other classes than Text/TextFlow will need to create HitInfo objects. For example, TextFieldSkin and TextAreaSkin modifies the hit to back up when a hit happens past a newline. Maybe that can be done some other way. > Lastly, calculating `charIndex` and `leading` already involved the text, so I don't see the justification for requiring to see the text again in order to compute insertion index. That was for the cases when another class creates a HitInfo (or mutates it). I guess we can do it the way you suggest, but then the constructor would have to be package-scope only. Won't there be a performance impact, though, if the data length has to be pre-calculated for every hit test, even when the insertion point is not needed? I'm thinking specifically about dragging to select, where we only use charIndex.
18-11-2015
The representation int charIndex; // negative when no character hit boolean leading; does not allow to represent a hit where no character was hit, but there still is a meaningful insertion index, as in a click before the line start or after the line end. I don't understand the argument for keeping HitInfo mutable, name I don't understand how other classes "depend" on mutating the fields. Would you mind sharing an example of how another class needs to update the fields? Lastly, calculating `charIndex` and `leading` already involved the text, so I don't see the justification for requiring to see the text again in order to compute insertion index.
18-11-2015
My current proposal is to move HitInfo to javafx.scene.text and to remove the internal workaround class TextPosInfo. The method HitInfo#getInsertionIndex() will now take the Text as argument in order to calculate the data length of clusters, etc. The reason for not storing the data length when creating the HitInfo object is that it allows us to keep HitInfo mutable. There are already several classes that depend on changing the charIndex and leading values. Even if it were immutable, a new instance would still need to use the BreakIterator algorithm on the Text to update the data length. I'd like to avoid using OptionalInt here. A return value < 0 should be used to represent no hit. public class HitInfo { public HitInfo(int charIndex, boolean leading); public int getCharIndex(); public void setCharIndex(int charIndex); public boolean isLeading(); public void setLeading(boolean leading); public int getInsertionIndex(String text); }
18-11-2015