RFR: JDK-8314215 Trailing Spaces before Line Breaks Affect the Center Alignment of Text [v7]

[Andy Goryachev]

On Tue, 30 Jan 2024 00:35:24 GMT, John Hendrikx <jhendrikx at 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?

  
![Screenshot 2024-02-06 at 10 47 18](https://github.com/openjdk/jfx/assets/107069028/b52c2b2c-06ba-4d23-a9bf-7cffa93d5f1d)
![Screenshot 2024-02-06 at 10 47 32](https://github.com/openjdk/jfx/assets/107069028/ab999606-5b0e-446b-a3fd-ac3fa45449fa)

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

PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1930558059