On Thu, 15 Feb 2024 10:36:51 GMT, Karthik P K <k...@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
