On Tue, 30 Jan 2024 00:12:52 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> There are a number of tickets open related to text rendering: >> >> https://bugs.openjdk.org/browse/JDK-8314215 >> >> https://bugs.openjdk.org/browse/JDK-8145496 >> >> https://bugs.openjdk.org/browse/JDK-8129014 >> >> They have in common that wrapped text is taking the trailing spaces on each >> wrapped line into account when calculating where to wrap. This looks okay >> for text that is left aligned (as the spaces will be trailing the lines and >> generally aren't a problem, but looks weird with CENTER and RIGHT >> alignments. Even with LEFT alignment there are artifacts of this behavior, >> where a line like `AAA BBB CCC` (note the **double** spaces) gets split up >> into `AAA `, `BBB ` and `CCC`, but if space reduces further, it will wrap >> **too** early because the space is taken into account (ie. `AAA` may still >> have fit just fine, but `AAA ` doesn't, so the engine wraps it to `AA` + `A >> ` or something). >> >> The fix for this is two fold; first the individual lines of text should not >> include any trailing spaces into their widths; second, the code that is >> taking the trailing space into account when wrapping should ignore all >> trailing spaces (currently it is ignoring all but one trailing space). With >> these two fixes, the layout in LEFT/CENTER/RIGHT alignments all look great, >> and there is no more early wrapping due to a space being taking into account >> while the actual text still would have fit (this is annoying in tight >> layouts, where a line can be wrapped early even though it looks like it >> would have fit). >> >> If it were that simple, we'd be done, but there may be another issue here >> that needs solving: wrapped aligned TextArea's. >> >> TextArea don't directly support text alignment (via a setTextAlignment >> method like Label) but you can change it via CSS. >> >> For Left alignment + wrapping, TextArea will ignore any spaces typed before >> a line that was wrapped. In other words, you can type spaces as much as you >> want, and they won't show up and the cursor won't move. The spaces are all >> getting appended to the previous line. When you cursor through these >> spaces, the cursor can be rendered out of the control's bounds. To >> illustrate, if you have the text `AAA BBB CCC`, and the text >> gets wrapped to `AAA`, `BBB`, `CCC`, typing spaces before `BBB` will not >> show up. If you cursor back, the cursor may be outside the control bounds >> because so many spaces are trailing `AAA`. >> >> The above behavior has NOT changed, is pretty standard for wrapped text >> controls,... > > 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, otherwise assume it is `0`. - Mark everything final / private that can be - Remove any unused fields or fields/parameters that always have the same value (isCanonical, perhaps more) ------------- PR Comment: https://git.openjdk.org/jfx/pull/1236#issuecomment-1915835190
