From 0e2cce29beec69f536b782e3520045177b9f6dd9 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Sat, 4 Jul 2026 03:19:10 -0400 Subject: [PATCH] fix(text): honor element color/anchor on parse_colors path and drop O(n^2) fitting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A6: unmarked text on the parse_colors path now uses the element-level color instead of a hardcoded black. parse_colored_text() gains a default_color arg. - A7: the vertical anchor component (e.g. 'mm', 'mb') is now applied to single-line parse_colors text, matching the plain path — previously it drew at the raw y with a 'lt' anchor and shifted down by half the text height. - B2: replace the character-at-a-time truncate (O(n^2), seconds on the event loop for long strings) with a binary-search cut (O(n log n)), and wrap by accumulating precomputed word widths instead of re-measuring the whole line per word. - B7: remove the two textbbox() calls in draw_multiline whose results were discarded, and the per-segment textbbox() calls on the parse_colors paths of draw_text (pos_y now derives from font metrics / precomputed block height). Adds regression tests for A6, A7, and the B2 helpers. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UVpLN4Rzdv7y3ud7QzutVJ --- src/odl_renderer/elements/text.py | 133 ++++++++++++++++-------- tests/integration/test_text_advanced.py | 35 +++++++ tests/unit/test_text_wrapping.py | 48 +++++++++ 3 files changed, 174 insertions(+), 42 deletions(-) create mode 100644 tests/unit/test_text_wrapping.py diff --git a/src/odl_renderer/elements/text.py b/src/odl_renderer/elements/text.py index 291bddb..1fda507 100644 --- a/src/odl_renderer/elements/text.py +++ b/src/odl_renderer/elements/text.py @@ -58,28 +58,9 @@ async def draw_text(ctx: DrawingContext, element: dict[str, Any]) -> None: final_text = text if max_width is not None: if element.get("truncate", False): - if draw.textlength(text, font=font) > max_width: - ellipsis = "..." - truncated = text - while truncated and draw.textlength(truncated + ellipsis, font=font) > max_width: - truncated = truncated[:-1] - final_text = truncated + ellipsis + final_text = _truncate_to_width(text, font, max_width) else: - words = text.split() - lines: list[str] = [] - current_line: list[str] = [] - - for word in words: - test_line = " ".join(current_line + [word]) - if not current_line or draw.textlength(test_line, font=font) <= max_width: - current_line.append(word) - else: - lines.append(" ".join(current_line)) - current_line = [word] - - if current_line: - lines.append(" ".join(current_line)) - final_text = "\n".join(lines) + final_text = _wrap_to_width(text, font, max_width) # Set appropriate anchor based on line count if not anchor: @@ -87,7 +68,7 @@ async def draw_text(ctx: DrawingContext, element: dict[str, Any]) -> None: # Draw the text if element.get("parse_colors", False): - segments = parse_colored_text(final_text) + segments = parse_colored_text(final_text, element.get("color", "black")) # Check if text contains newlines has_newlines = "\n" in final_text @@ -103,7 +84,6 @@ async def draw_text(ctx: DrawingContext, element: dict[str, Any]) -> None: adjusted_y = calculate_anchor_offset_y(y, total_height, anchor) # Draw each line - max_y = adjusted_y for line_segments, line_y_offset in zip(seg_lines, line_y_positions): # Calculate horizontal positions for this line line_segments, line_width = calculate_segment_positions(line_segments, font, x, align, anchor) @@ -114,7 +94,6 @@ async def draw_text(ctx: DrawingContext, element: dict[str, Any]) -> None: # Draw each segment in the line for segment in line_segments: color = ctx.colors.resolve(segment.color) - bbox = draw.textbbox((segment.start_x, line_y), segment.text, font=font, anchor="lt") draw.text( (segment.start_x, line_y), segment.text, @@ -125,22 +104,19 @@ async def draw_text(ctx: DrawingContext, element: dict[str, Any]) -> None: stroke_width=stroke_width, stroke_fill=stroke_fill, ) - max_y = max(max_y, int(bbox[3])) - ctx.pos_y = max_y + ctx.pos_y = adjusted_y + total_height else: + # Apply the vertical anchor component (e.g. 'mm', 'mb') to the single + # line too — Pillow honours it on the non-parse path, so parse_colors + # must not silently drop it. + line_height = _line_height(font) + adjusted_y = calculate_anchor_offset_y(y, line_height, anchor) segments, total_width = calculate_segment_positions(segments, font, x, align, anchor) - max_y = y for segment in segments: color = ctx.colors.resolve(segment.color) - bbox = draw.textbbox( - (segment.start_x, y), - segment.text, - font=font, - anchor="lt", - ) draw.text( - (segment.start_x, y), + (segment.start_x, adjusted_y), segment.text, fill=color, font=font, @@ -149,8 +125,7 @@ async def draw_text(ctx: DrawingContext, element: dict[str, Any]) -> None: stroke_width=stroke_width, stroke_fill=stroke_fill, ) - max_y = max(max_y, int(bbox[3])) - ctx.pos_y = max_y + ctx.pos_y = adjusted_y + line_height else: bbox = draw.textbbox((x, y), final_text, font=font, anchor=anchor, spacing=spacing, align=align) draw.text( @@ -206,12 +181,11 @@ async def draw_multiline(ctx: DrawingContext, element: dict[str, Any]) -> None: max_y = current_y for line in lines: if element.get("parse_colors", False): - segments = parse_colored_text(str(line)) + segments = parse_colored_text(str(line), element.get("color", "black")) segments, total_width = calculate_segment_positions(segments, font, x, align, anchor) for segment in segments: color = ctx.colors.resolve(segment.color) - draw.textbbox((segment.start_x, current_y), segment.text, font=font, anchor="lt") draw.text( (segment.start_x, current_y), segment.text, @@ -222,7 +196,6 @@ async def draw_multiline(ctx: DrawingContext, element: dict[str, Any]) -> None: stroke_fill=stroke_fill, ) else: - draw.textbbox((x, current_y), str(line), font=font, anchor=anchor, align=align) draw.text( (x, current_y), str(line), @@ -238,6 +211,80 @@ async def draw_multiline(ctx: DrawingContext, element: dict[str, Any]) -> None: ctx.pos_y = max_y +def _line_height(font: ImageFont.FreeTypeFont) -> int: + """Return a line height in pixels using ascender/descender-bearing sample chars.""" + bbox = font.getbbox("Ay") + return int(bbox[3] - bbox[1]) + + +def _truncate_to_width(text: str, font: ImageFont.FreeTypeFont, max_width: float, ellipsis: str = "...") -> str: + """Truncate *text* with an ellipsis so it fits within *max_width* pixels. + + Binary-searches the cut point (O(n log n)) instead of trimming one character at + a time and re-measuring the whole string on each step (O(n^2)), which could take + seconds on the event loop for a long templated string. + + Args: + text: The text to fit. + font: Font used to measure widths. + max_width: Maximum width in pixels. + ellipsis: String appended to indicate truncation. + + Returns: + The original text if it already fits, otherwise the longest prefix plus the + ellipsis that fits within ``max_width``. + """ + if font.getlength(text) <= max_width: + return text + # Largest prefix length whose text + ellipsis still fits (monotonic in length). + lo, hi = 0, len(text) + while lo < hi: + mid = (lo + hi + 1) // 2 + if font.getlength(text[:mid] + ellipsis) <= max_width: + lo = mid + else: + hi = mid - 1 + return text[:lo] + ellipsis + + +def _wrap_to_width(text: str, font: ImageFont.FreeTypeFont, max_width: float) -> str: + """Word-wrap *text* to fit *max_width*, measuring each word only once. + + Accumulates precomputed word and space widths rather than re-measuring the whole + current line for every word (which is quadratic per line). + + Args: + text: The text to wrap. + font: Font used to measure widths. + max_width: Maximum line width in pixels. + + Returns: + The text with ``\\n`` inserted at wrap points. + """ + words = text.split() + if not words: + return "" + space_width = font.getlength(" ") + lines: list[str] = [] + current_words: list[str] = [] + current_width = 0.0 + for word in words: + word_width = font.getlength(word) + if not current_words: + current_words = [word] + current_width = word_width + elif current_width + space_width + word_width <= max_width: + current_words.append(word) + current_width += space_width + word_width + else: + lines.append(" ".join(current_words)) + current_words = [word] + current_width = word_width + if current_words: + lines.append(" ".join(current_words)) + return "\n".join(lines) + + def get_wrapped_text(text: str, font: ImageFont.ImageFont, line_length: int) -> str: """Wrap text to fit within a given width. @@ -261,7 +308,7 @@ def get_wrapped_text(text: str, font: ImageFont.ImageFont, line_length: int) -> return "\n".join(lines) -def parse_colored_text(text: str) -> List[TextSegment]: +def parse_colored_text(text: str, default_color: str = "black") -> List[TextSegment]: """Parse text with color markup into text segments. Breaks text with color markup like "[red]text[/red]" into segments @@ -269,6 +316,8 @@ def parse_colored_text(text: str) -> List[TextSegment]: Args: text: Text with color markup + default_color: Color applied to text outside of any ``[color]`` markup + (i.e. the element-level color). Returns: List[TextSegment]: List of text segments with colors @@ -283,14 +332,14 @@ def parse_colored_text(text: str) -> List[TextSegment]: for match in re.finditer(pattern, text, re.DOTALL): # Add any text before the match with default color if match.start() > current_pos: - segments.append(TextSegment(text=text[current_pos : match.start()], color="black")) + segments.append(TextSegment(text=text[current_pos : match.start()], color=default_color)) # Add the matched text with the specified color segments.append(TextSegment(text=match.group(2), color=match.group(1))) current_pos = match.end() # Add any remaining text with default color if current_pos < len(text): - segments.append(TextSegment(text=text[current_pos:], color="black")) + segments.append(TextSegment(text=text[current_pos:], color=default_color)) return segments diff --git a/tests/integration/test_text_advanced.py b/tests/integration/test_text_advanced.py index dcdc969..dfc5376 100644 --- a/tests/integration/test_text_advanced.py +++ b/tests/integration/test_text_advanced.py @@ -1,4 +1,5 @@ import pytest +from PIL import Image, ImageChops from odl_renderer import generate_image @@ -7,6 +8,11 @@ def text(**kwargs): return {"type": "text", "x": 10, "y": 10, "value": "Hello", "font": "ppb", "size": 16, **kwargs} +def content_bbox(image: Image.Image) -> tuple[int, int, int, int] | None: + """Bounding box of the non-white (drawn) pixels of a white-background image.""" + return ImageChops.invert(image.convert("L")).getbbox() + + def multiline(**kwargs): return { "type": "multiline", @@ -132,6 +138,35 @@ async def test_parse_colors_anchor_middle(self): ) assert image.size == (200, 100) + async def test_parse_colors_uses_element_color(self): + """Unmarked text on the parse_colors path uses the element color, not black (A6).""" + image = await generate_image( + width=200, + height=100, + elements=[text(value="plain text", parse_colors=True, color="red")], + ) + colors = {c for _, c in image.convert("RGB").getcolors(maxcolors=100000)} + assert (255, 0, 0) in colors, "element color 'red' was ignored on the parse_colors path" + + async def test_parse_colors_honors_vertical_anchor(self): + """A 'mm' anchor must position single-line parse_colors text like the plain path (A7).""" + with_parse = await generate_image( + width=200, + height=100, + elements=[text(value="Middle", parse_colors=True, anchor="mm", x=100, y=50, color="black")], + ) + plain = await generate_image( + width=200, + height=100, + elements=[text(value="Middle", anchor="mm", x=100, y=50, color="black")], + ) + parse_box = content_bbox(with_parse) + plain_box = content_bbox(plain) + assert parse_box is not None and plain_box is not None + # Vertical top should match within a couple of pixels, not be shifted down by + # half the text height as it was when the anchor was dropped. + assert abs(parse_box[1] - plain_box[1]) <= 2 + async def test_parse_colors_anchor_bottom(self): image = await generate_image( width=200, diff --git a/tests/unit/test_text_wrapping.py b/tests/unit/test_text_wrapping.py new file mode 100644 index 0000000..f245c69 --- /dev/null +++ b/tests/unit/test_text_wrapping.py @@ -0,0 +1,48 @@ +"""Unit tests for the text truncation/wrapping helpers (finding B2).""" + +from odl_renderer.elements.text import _truncate_to_width, _wrap_to_width + + +def test_truncate_to_width_fits_and_adds_ellipsis(load_font): + font = load_font("ppb", 16) + text = "word " * 200 # far wider than 100px + result = _truncate_to_width(text, font, 100) + assert result.endswith("...") + assert font.getlength(result) <= 100 + + +def test_truncate_to_width_short_text_unchanged(load_font): + font = load_font("ppb", 16) + assert _truncate_to_width("Hi", font, 500) == "Hi" + + +def test_truncate_to_width_matches_linear_result(load_font): + """Binary search must produce the same cut as trimming one char at a time.""" + font = load_font("ppb", 16) + ellipsis = "..." + text = "The quick brown fox jumps over the lazy dog again and again" + max_width = 90 + + expected = text + if font.getlength(text) > max_width: + truncated = text + while truncated and font.getlength(truncated + ellipsis) > max_width: + truncated = truncated[:-1] + expected = truncated + ellipsis + + assert _truncate_to_width(text, font, max_width) == expected + + +def test_wrap_to_width_lines_fit(load_font): + font = load_font("ppb", 16) + text = "This is a long sentence that should wrap onto several lines" + wrapped = _wrap_to_width(text, font, 100) + assert "\n" in wrapped + for line in wrapped.split("\n"): + # A single word longer than max_width is allowed to overflow its own line. + assert font.getlength(line) <= 100 or " " not in line + + +def test_wrap_to_width_empty_string(load_font): + font = load_font("ppb", 16) + assert _wrap_to_width(" ", font, 100) == ""