Skip to content

fix(core): getBoxesForCharRange — textScaler, line height, pre/image tests - #19

Open
DrkXo wants to merge 4 commits into
brewkits:mainfrom
DrkXo:fix/get-boxes-for-char-range
Open

DrkXo wants to merge 4 commits into
brewkits:mainfrom
DrkXo:fix/get-boxes-for-char-range

Conversation

@DrkXo

@DrkXo DrkXo commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Follows up on the review feedback from #17.

Cherry-picked onto main (v1.10.0), devtools files excluded.

Commits included

  • 16b4c23 feat(core): add getBoxesForCharRange
  • f5a313b fix(core): bridge inter-word gaps
  • 05e0245 fix(core): use line top/height for highlight boxes

Changes

  • textScaler: getBoxesForCharRange routes through _getTextPainter() which constructs TextPainter(textScaler: _textScaler) — highlights stay accurate when the user changes system font size.
  • Doc comment: updated to clarify that x-boundaries come from BoxHeightStyle.tight while y-boundaries use line.top / line.height (not the tight box coords).
  • Tests added:
    • white-space: pre — leading spaces at the selection edge are preserved, not trimmed by the isPreformatted guard.
    • Inline image between two words — verifies the 16px maxGap heuristic does not collapse the two word rects into one.

@vietnguyentuan2019

Copy link
Copy Markdown
Contributor

Thanks, this is close! 🙏 textScaler and the doc fix look good, and all 7 tests pass on my side. A few things before merge:

  1. The inline image test passes but doesn't test what its name says. I ran it: getBoxesForCharRange(0, 12) returns ONE rect (0→208), so the two words are still merged over the image. The image is exactly 16px = maxGap, and <= merges it. The test only checks isNotEmpty and width > 10, so it always passes. Could you make it skip the merge when an atom sits between the two fragments, and assert hasLength(2)? (If you think merging is fine for highlights, then rename the test and say so in the doc instead.)

  2. pre test: code is correct (full range is 128 wide vs 80 for just "hello"), but width > 20 would pass even if the spaces got trimmed. Please compare against the rect of "hello" alone.

  3. debugLineFragments() is in the diff but not in the PR description, and its test file is not included. Please drop it from this PR (or add the test + mention it).

Small stuff:

  • doc says "pixel-snapped" but nothing snaps
  • [top] / [height] are not valid doc refs
  • test comment says range 0..11, code uses 0..12
  • could you move the method to render_hyper_box_selection.dart, next to the other selection APIs?

The red "Layout Regression" check is not from your change, the workflow can't post comments on fork PRs. Ignore it, I'll fix it on our side. Thanks again!

This branch has not been deployed

No deployments
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