On Tue, 30 Jan 2024 00:35:24 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> John Hendrikx has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix bug which confused char index with glyph index > > I think that it may be wise to do some clean-up of the code surrounding > `TextRun` in some future PR. It's a mish-mash of things currently: > > - TextRun seems to be used in two ways. > - The "normal" way where multiple runs are constructed from a character > array, and it retains references to the start/end in that array (although not > a reference to the array itself). Because it has no reference to the chars > from which it was created, a lot of functionality which could be encapsulated > is externalized, and many internals of `TextRun` need to be exposed to feed > those external functions. > - For javafx-web, which just wants to render a bunch of glyphs, unrelated > to any text (so there is no character array it is based on, and its start/end > values are bogus) > - There is a lot of code that pretends there is a difference between a > `GlyphList` (clearly intended for rendering pure glyphs only) and `TextRun`, > but there is only one implementation of `GlyphList` (`TextRun`). The code > that does the actual rendering in `NGText` bluntly always casts a `GlyphList` > to a `TextRun`. It does this for the sole purpose of finding out the start > character position of the original characters the `TextRun` was created from > (needed for selection rendering), yet, for `TextRun`s created by javafx-web > this is irrelevant (it just always returns `0` for the start character > position). > - It would have been better to do a conditional cast to `TextRun` in > `NGText` so that javafx-web can use a much simpler implementation of > `GlyphList` not based on `TextRun`. > - In a lot of places in this code and surrounding code, fields have been > failed to be marked `private` or `final` even though they can be; this may > have implications for what JIT can do. > - There are unused fields (`isCanonical` always returns `false`, `TextLine` > is only stored to get its `RectBounds`, could just store that directly, it's > `final`) > > A clean-up would entail: > - Include a reference to the characters it was created from in `TextRun` > - Encapsulate a lot of code into `TextRun` (moved from `PrismTextLayout`) > that does calculations that needed both internal information of `TextRun` > (that no longer needs to be exposed) and the original characters. > - Have javafx-web just implement `GlyphList` and change `NGText` to work with > `GlyphList`. This class would be much smaller (`TextRun` has over a dozen > fields). > - In `NGText` do an `instanceof` check to see if it is a `TextRun`, in which > case its start offset can be used, oth... @hjohn are these screenshots show expected layout (with the CENTER alignment and regular spaces 0x20)? Shouldn't the leading/trailing whitespace be ignored?   ------------- PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1930558059
