Skip to content

fix(util): split_into_lines merges a word split across 2+ colored spans#6844

Open
chuenchen309 wants to merge 1 commit into
beetbox:masterfrom
chuenchen309:fix/layout-multi-span-word-split
Open

fix(util): split_into_lines merges a word split across 2+ colored spans#6844
chuenchen309 wants to merge 1 commit into
beetbox:masterfrom
chuenchen309:fix/layout-multi-span-word-split

Conversation

@chuenchen309

Copy link
Copy Markdown

Bug

split_into_lines() in beets/util/layout.py reconstructs "words" from ESC_TEXT_REGEX.finditer() matches to correctly wrap colored diff text at whitespace. Each match has pretext/esc/text/reset/posttext groups; posttext is [^ESC]* (greedy), so it always consumes all plain text up to the next escape sequence. That means pretext is structurally always empty for every match after the first — the regex engine's next match simply starts right where the previous one's posttext left off.

The code treats an empty pretext as proof there's a word boundary ("space before") right there:

else:
    # pretext empty, treat as if there is a space before
    space_before_text = True

This is only actually true for the first match (where empty pretext really does mean "start of string"). For every later match, it's a coincidence of the regex structure, not evidence of a real space — so a word containing two or more separately-colored spans (e.g. two highlighted typo-fix characters within one word) gets a spurious space inserted at the second span.

from beets.util.color import uncolorize
from beets.util.layout import split_into_lines

RED, RESET = "\x1b[31m", "\x1b[39;49;00m"
text = f"extra{RED}A{RESET}ordin{RED}B{RESET}ary"   # one word: "extraAordinBary"
lines = split_into_lines(text, 100, 100)
uncolorize(lines[0])
# before: 'extraAordin Bary'  -- word incorrectly split
# after:  'extraAordinBary'   -- correct

split_into_lines is reachable from get_layout_lines/get_column_layout, used by beets/ui/commands/import_/display.py to wrap diff output during beet import when a line exceeds terminal width. The colored text comes from colordiff() (beets/util/diff.py), which produces exactly this multi-span-per-word coloring whenever a correction touches two non-adjacent parts of the same word.

Fix

Track the real boundary state explicitly across finditer iterations, instead of inferring it purely from the current match's pretext being empty:

prev_ends_with_space = True  # start of string is a boundary
for m in ESC_TEXT_REGEX.finditer(string):
    ...
    if m.group("pretext") != "":
        ...
    else:
        space_before_text = prev_ends_with_space
    ...
    # at the end of the loop body:
    if m.group("posttext"):
        prev_ends_with_space = m.group("posttext")[-1] == " "
    else:
        prev_ends_with_space = m.group("text")[-1] == " "

(posttext empty means reset is followed directly by the next escape with nothing in between, so the boundary question falls back to whether the current match's own text ended in a space.)

Testing

Added test_split_into_lines_two_spans_in_one_word to test/util/test_layout.py. Verified red on master (git stash the source fix — the word gets split with a spurious space), green after. Full test/util/ suite (191 passed, 8 skipped — no PIL/ImageMagick/platform-specific) passes with no regressions, including the existing multi-span test cases in test_layout.py (which cover the every-match-has-a-real-space case, e.g. "test " * 3 — confirmed my fix's text[-1]==" " fallback keeps those correctly separated). ruff check/ruff format --check/mypy clean.

Added a changelog entry per CONTRIBUTING.rst's reminder.

Duplicate-work check

No open PR touches layout.py.


This PR was drafted with AI assistance (Claude); I independently reproduced the bug against the real, unmodified module and verified the fix. I reviewed the change and take responsibility for it.

ESC_TEXT_REGEX's greedy posttext group makes pretext structurally empty
for every match after the first, so the boundary-detection logic
("pretext empty => treat as space before") was only actually correct
for the first match. A second colored span mid-word (e.g. two
highlighted typo fixes within one word) got mistaken for the start of
a new word, inserting a spurious space. Track the real boundary state
across iterations instead.
@chuenchen309
chuenchen309 requested a review from a team as a code owner July 16, 2026 00:15
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.61%. Comparing base (17d6798) to head (e0f477b).
⚠️ Report is 34 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6844      +/-   ##
==========================================
+ Coverage   75.56%   75.61%   +0.04%     
==========================================
  Files         163      163              
  Lines       21285    21289       +4     
  Branches     3359     3360       +1     
==========================================
+ Hits        16085    16097      +12     
+ Misses       4411     4403       -8     
  Partials      789      789              
Files with missing lines Coverage Δ
beets/util/layout.py 87.82% <100.00%> (+4.26%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@snejus

snejus commented Jul 16, 2026

Copy link
Copy Markdown
Member

What sort of negative consequences does this bug have?

@chuenchen309

Copy link
Copy Markdown
Author

Display-only — no data is touched — but it makes the import prompt show a string that isn't the candidate.

colordiff() produces multiple spans within one word whenever a correction touches two non-adjacent characters, which is exactly the common typo case. Against current master:

from beets.util.diff import colordiff
from beets.util.color import uncolorize
from beets.util.layout import split_into_lines

left, right = colordiff("Extraordinary", "Extrabordinory")
# right == 'Extra\x1b[1;32mb\x1b[39;49;00mordin\x1b[1;32mo\x1b[39;49;00mry'

[uncolorize(l) for l in split_into_lines(right, 100, 100)]
# master: ['Extrabordin ory']   <- spurious space inside the word
# fix:    ['Extrabordinory']

So at the point where you're deciding whether to accept a match, the candidate renders as Extrabordin ory rather than Extrabordinory. The more highlighted spans a word has, the more spurious spaces appear.

It also mis-wraps: at width 12 master breaks after Extrabordin (11 chars) because it thinks the word ended there, while the fixed version fills the line properly (Extrabordino / ry).

Only reachable when the wrapping path runs (beet import diff output past terminal width), so it's cosmetic in severity — I'm happy to drop it if you'd rather not carry the extra state tracking for a display-only issue.

@snejus snejus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants