Uh oh!
There was an error while loading. Please reload this page.
Fixed word wrapping emitting a blank row for trailing whitespace - #177
Merged
fdesbiens merged 1 commit intoAug 27, 2026
Merged
Conversation
b8bb23b (eclipse-threadx#159) fixed issue eclipse-threadx#130 by letting the overflow branch of _gx_multi_line_text_view_display_info_get() run when an ASCII space is the character that overflows the available width. That branch consumes the space and any consecutive spaces without adding them to the row width, which is correct, and then breaks. When the whitespace ran to the end of the source line, the line terminator was left behind: the next call started on it, hit the GX_KEY_LINE_FEED case immediately and returned a row of one byte and zero width, which draws as a blank line that is not in the text. The overflow branch now takes the line terminator with the whitespace when nothing else separates them, handling a bare line feed and a carriage return / line feed pair, and guarding the second byte on the remaining length -- unlike the pre-existing GX_KEY_CARRIAGE_RETURN case earlier in the same loop, which reads ch.gx_string_ptr[1] unchecked. gx_text_display_width is untouched, so the issue eclipse-threadx#130 fix stands. Two such rows appear in the guix_ml_text_view_32bpp fixture, at string offsets 2992 and 23988 of readme_guix_generic.txt: "...Improved internal logic." with twelve trailing spaces, and "...cursor_pos_calculate.c" with one. Both were confirmed with a conditional breakpoint on display_number == 1 and display_width == 0 preceded by a space, which fires exactly twice on the pre-fix code and never after. The two extra rows are why 268 of that test's 300 frames and 144 of guix_bidi_text_draw_32bpp's 429 frames have differed from their golden data since June. Nothing inside GUIX reads gx_text_display_width -- every caller uses only gx_text_display_number, to advance its index -- so the row count alone governs where every row starts, and two extra rows change the scrollbar's value-to-pixel mapping. Every scroll step then lands at a slightly different pixel offset and the whole text block is drawn a few pixels up or down. Of the 300 frames, 297 were a pure vertical shift of otherwise identical text; only 3 were laid out differently, and those 3 are the two places above. No golden data is regenerated. gx_multi_line_text_view_text_total_rows for the long text view goes back from 2226 to 2224, and both tests pass against their existing golden files. guix_ml_text_view_word_wrap_no_output did not protect eclipse-threadx#159's fix: it passed on the pre-eclipse-threadx#159 code as well. Its available_width was one pixel too generous -- a_width + space_width, so the over-wide row the old code produced measured exactly available_width and still satisfied "width <= available_width". It is now a_width + space_width - 1, which makes appending the space genuinely overflow, and three cases are added: a line feed terminator, a carriage return / line feed terminator, and the total row count that _gx_multi_line_text_view_string_total_rows_compute() derives from them. That last one is the quantity the golden frames actually depend on, and the only one of the three a unit test can pin without golden data. The test was verified to discriminate in both directions by rebuilding against each. Against the pre-eclipse-threadx#159 source it fails four width assertions, which is issue eclipse-threadx#130. Against eclipse-threadx#159 as shipped it fails the line feed row (2 bytes, not 3), the carriage return / line feed row (2, not 4) and the row count (3, not 2). It passes only with both fixes in place. Verified on Linux across all eighteen build configurations, 1847 tests, no failures. default_build_coverage 733/733, dynamic_bidi_text_build 3/3 including guix_bidi_text_draw_32bpp, no_utf8_build_coverage 135/135. One coverage boundary worth naming: the unit test builds against the all_widgets demo, which is not in NO_UTF8_DEMOS, so it runs in default_build_coverage and disable_error_check_build but not in the GX_UTF8_SUPPORT-off configurations. The changed code is common to both paths -- only the surrounding character advance differs -- and no_utf8_build_coverage covers it through its 135 golden tests. No documentation change is required. _gx_multi_line_text_view_display_info_get is internal, and the GUIX documentation does not describe multi-line text view wrapping behaviour for trailing whitespace. Assisted-by: Claude Code (Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
b8bb23b8(#159) fixed issue #130 by letting the overflow branch of_gx_multi_line_text_view_display_info_get()run when an ASCII space is the character that overflows the available width. That branch consumes the space and any consecutive spaces without adding them to the row width — correct — and then breaks.When the whitespace ran to the end of the source line, the line terminator was left behind. The next call started on it, hit the
GX_KEY_LINE_FEEDcase immediately, and returned a row of one byte and zero width: a blank line that is not in the text.The overflow branch now takes the line terminator with the whitespace when nothing else separates them, handling both a bare line feed and a
\r\npair, with the second byte guarded on the remaining length.gx_text_display_widthis untouched, so the #130 fix stands.Why two golden tests have been red since June
guix_ml_text_view_32bpp(268 of 300 frames) andguix_bidi_text_draw_32bpp(144 of 429) have failed in every CI run since #159 landed.Two phantom rows appear in the
guix_ml_text_view_32bppfixture, at string offsets 2992 and 23988 ofreadme_guix_generic.txt:...Improved internal logic.with twelve trailing spaces, and...cursor_pos_calculate.cwith one. Both were confirmed with a conditional breakpoint ondisplay_number == 1 && display_width == 0preceded by a space — it fires exactly twice on the pre-fix code and never after.Nothing inside GUIX reads
gx_text_display_width; every caller uses onlygx_text_display_number, to advance its index. So the row count alone governs where every row starts, and two extra rows change the scrollbar's value-to-pixel mapping. Every scroll step then lands at a slightly different pixel offset and the whole text block is drawn a few pixels up or down. Of the 300 frames, 297 were a pure vertical shift of otherwise identical text; only 3 were laid out differently, and those 3 are the two places above.No golden data is regenerated.
gx_multi_line_text_view_text_total_rowsfor the long text view goes back from 2226 to 2224, and both tests pass against their existing golden files.Test work
guix_ml_text_view_word_wrap_no_outputdid not protect #159's fix — it passed on the pre-#159 code too. Itsavailable_widthwas one pixel too generous (a_width + space_width), so the over-wide row the old code produced measured exactlyavailable_widthand still satisfiedwidth <= available_width.It is now
a_width + space_width - 1, and three cases are added:\r\nterminator,_gx_multi_line_text_view_string_total_rows_compute()derives from them — the quantity the golden frames actually depend on, and the only one of the three a unit test can pin without golden data.Verified to discriminate in both directions by rebuilding against each source state:
b8bb23b8width <= available_widthassertions (issue #130)b8bb23b8as shipped\r\nrow (2, not 4), row count (3, not 2)Verification
Linux, all eighteen build configurations, 1847 tests, no failures.
default_build_coverageguix_ml_text_view_32bppgreen against unmodified goldendynamic_bidi_text_buildguix_bidi_text_draw_32bppgreen against unmodified goldenno_utf8_build_coverageOne coverage boundary worth naming: the unit test builds against the
all_widgetsdemo, which is not inNO_UTF8_DEMOS, so it runs indefault_build_coverageanddisable_error_check_buildbut not in theGX_UTF8_SUPPORT-off configurations. The changed code is common to both paths — only the surrounding character advance differs — andno_utf8_build_coveragecovers it through its 135 golden tests.No documentation change is required:
_gx_multi_line_text_view_display_info_getis internal, and the GUIX documentation does not describe multi-line text view wrapping behaviour for trailing whitespace.