RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v11]

[Karthik P K]

On Wed, 14 Feb 2024 04:33:00 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Karthik P K has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Review comments
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 424:
> 
>> 422: 
>> 423:     @Override
>> 424:     public Hit getHitInfo(float x, float y, String text, int textRunStart, int curRunStart, boolean forTextFlow) {
> 
> This method has become huge, with I think upto 7 levels of nesting.  It would benefit from some splitting.
> 
> Suggestions in that area:
> - Split it in one that handles RTL and one for LTR **or**
> - Split it in one that handles `spans != null` (`TextFlow`) and one that handles `Text`
> 
> You can also reduce the nesting of the first `if/else` by returning early:
> 
> 
>         if (lineIndex >= getLineCount()) {
>             charIndex = getCharCount();
>             insertionIndex = charIndex + 1;
> 
>             return new Hit(charIndex, insertionIndex, leading);  // return early here
>         } else {   // no need to nest 150+ lines of code
> 
> 
> More suggestions inline below

I did not find splitting the `getHitInfo` method based on some condition very helpful. Since we are calculating effective value of x and TextRun, many values are dependant on intermediate values and the calculation involves many variables.  Hence I decided to keep the major calculations inside `getHitInfo` method. 
Added 2 methods to move the calculation of values like previous line text width and width of LTR text in RTL text out of `getHitInfo`. 

> You can also reduce the nesting of the first `if/else` by returning early:
> 
Made changes to return early in this case.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1494309979