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. > > 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. Sure, if you can give me more details as to why this is necessary, perhaps I can follow the logic. From an outside observer, `PrismTextLayout` already seems to have far too much specialization related to how text is supplied (spans/String) and who is supplying it (TextFlow/Text), making the logic hard to follow (but also error prone, and likely has more than a few bugs). I'd expect a class that does text layout measurements to be able to do this with a uniform format without special casing (ie. perhaps `Text` should wrap their text in a `TextSpan` and all the special casing can disappear). Your current changes seems to increase this divide with even more specialization, hence why I'm wondering if this is the best approach. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1945892410
