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

[John Hendrikx]

On Thu, 15 Feb 2024 10:36:51 GMT, Karthik P K <kpk at openjdk.org> wrote:

>> I think we should simplify the `getHitInfo` method in the `TextLayout` interface.
>> 
>> The code currently seems to be making distinctions where there are none.  `TextFlow`s provide spans, while `Text`s provide a text. `getHitInfo` can take advantage of this to avoid the `text` and `forTextFlow` parameters altogether.  This will reduce confusion (as there is no distinction) and also avoids cases that are non-sensical (providing `text` while `forTextFlow` is `true` or vice versa).
>> 
>> Previous changes to this code (when the parameter `text` was introduced to `getHitInfo`) should probably be partially undone and simplified with this new knowledge.
>
>> I think we should simplify the `getHitInfo` method in the `TextLayout` interface.
>> 
>> The code currently seems to be making distinctions where there are none. `TextFlow`s provide spans, while `Text`s provide a text. `getHitInfo` can take advantage of this to avoid the `text` and `forTextFlow` parameters altogether. This will reduce confusion (as there is no distinction) and also avoids cases that are non-sensical (providing `text` while `forTextFlow` is `true` or vice versa).
>> 
>> Previous changes to this code (when the parameter `text` was introduced to `getHitInfo`) should probably be partially undone and simplified with this new knowledge.
> 
> Thanks for reviewing @hjohn 
> Certainly we could simplify the `getHitInfo` method but the complication starts when we have to support Text node embedded in TextFlow. 
> Just to differentiate Text node and TextFlow, spans can be used as you suggested. I had to introduce these parameters for the case of Text node embedded in TextFlow. On top of that we need to support emojis and RTL text. This is where it started getting complex and I had to use these new parameters. If you have any thoughts on this, please let me know.
> I'll got through all the comments and incorporate wherever possible.

@karthikpandelu You mentioned there is special casing going on when a `Text` is part of a `TextFlow`. What are those cases and where is this happening? How does it even know that a `Text` is involved and that it is part of a `TextFlow`?

IMHO, this class is in desperate need of simplification (out of scope of course for this PR), which I think can be done by having `Text` supply a (single) `TextSpan`, and removing the option to supply the content as a `String` + `Font`. 

However, that would break down if there are also special cases for `Text` within `TextFlow` -- I can't think of what those would be, and if it indeed is the case, I strongly suspect that `PrismTextLayout` may be overstepping its mandate (ie, such concerns when a `Text` is nested in a `TextFlow` should be the concern of `TextFlow`).  Since I think simplifying this class would be a very good step, I'd just like to ensure we're not introducing more special casing now to make this future work even more impossible.

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

PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1952298422