shaping: (wrapping) make whitespace trim bidi-aware #175
Conversation
Our previous implementation of trimming the whitespace off of the end of wrapped lines assumed that the end was always the final logical run, and that the final glyph within that run was visually last. Neither of these assumptions are true in real-world bidi text. The final logical run may be embedded wtihin the center of the visual line of text, and sometimes the final glyph is the first within a run, rather than the last. Signed-off-by: Chris Waldon <[email protected]>
whereswaldon
requested review from
benoitkugler and
andydotxyz
as code owners
This commit adds a test case that ensures a space is trimmed when it appears at the end of a line (visually), but not when it occurs within the middle of a line. Signed-off-by: Chris Waldon <[email protected]>
|
@andydotxyz Absolutely there should be; I've added one. |
|
I hit a weird case with the ordering of runs this generates; I'm going to add more tests and see if I can isolate it. The issue may be in Gio instead of typesetting. |
This commit adds a test for visual bidi run ordering and fixes the implementation to pass it. The test itself is migrated from Gio's codebase, where it has been verifying this functionality for years. It caught some problems with the new implementation of this visual ordering algorithm, which are also fixed in this commit. Signed-off-by: Chris Waldon <[email protected]>
|
I've fixed the visual ordering of runs, but as I've been looking into including this version of typesetting into Gio, I've realized there's an additional problem. Our I think the fix is simple, but making it will require some new tests. I'll try to implement those tomorrow. |
|
Sorry I read the messages out of order. Will review again later if there are more changes :) |
|
Thanks @whereswaldon ! I guess I had totally overlooked the fact that, although |
When wrapping lines containing bidi runs, a run that progresses in the opposite direction from the overall paragraph will have its trailing whitespace in the middle of the line, not at the end. As such, it isn't valid to ignore the advance of trailing whitespace for such a run. We can only safely ignore trailing whitespace when the run is the same progression as the paragraph. Signed-off-by: Chris Waldon <[email protected]>
|
Okay, I believe this is now correct. Gio's tests fail because this space-trimming allows much better wrapping that we previously had, and the test text is actually fitting into fewer lines that it used to, but the wrapping is correct. If @benoitkugler is happy with the space aware advance tweak, I think we can merge this. |
|
Yes, this looks great, thank you Chris. I'll wait on @andydotxyz final review to merge it. |
Our previous implementation of trimming the whitespace off of the end of wrapped lines assumed that the end was always the final logical run, and that the final glyph within that run was visually last. Neither of these assumptions are true in real-world bidi text. The final logical run may be embedded wtihin the center of the visual line of text, and sometimes the final glyph is the first within a run, rather than the last.