On Wed, 14 Feb 2024 04:33:00 GMT, John Hendrikx <jhendr...@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
