Skip to content

Underflow guard added, boundary test fixed - #639

Merged
hannesa2 merged 2 commits into
masterfrom
ReviewOf625
Sep 15, 2026
Merged

Underflow guard added, boundary test fixed#639
hannesa2 merged 2 commits into
masterfrom
ReviewOf625

Conversation

@hannesa2

@hannesa2 hannesa2 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

It's the splittet follow up of #625 (comment)

Two minor ones, take or leave:

  • testASubjectOfExactly40CharactersIsNotTruncated names a boundary that isn't one.
    The early return is length <= targetLength with targetLength 43, so 41 through 43 are untruncated too.
    Asserting 43-untruncated and 44-truncated would pin the choice down.

  • Pre-existing and not from this PR: any targetLength below the indicator length underflows NSUInteger
    and crashes all three modes.
    Unreachable at 43, but cheap to guard now that the file has tests.

@hannesa2
hannesa2 requested a review from goneng September 14, 2026 05:34
@hannesa2 hannesa2 added the test Test related label Sep 14, 2026
@hannesa2

hannesa2 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Screenshot differs: emulatorApi=arm64 with 4 screenshot(s)

setpoint (old) | diff | actual screenshot (new)

00-commit-context-menu.pngscreenshot

01-history-view.pngscreenshot

02-main-window.pngscreenshot

03-staging-view.pngscreenshot

@goneng

goneng commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

@hannesa2, looks like this solves issues with "truncate end" (some tex...) and "truncate start" (...ome text) and but "truncate center" (som...ext) may still fail.
This is not a real issue at the moment because the only place we use this now is with "truncate end" that will work fine.
I can push a small change on-top of this one, here, or we can do that later as a separate PR, as not so urgent.
Your call.

@hannesa2

hannesa2 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Please add it on top of this. Thank you

@goneng
Fix Center truncation still crashing on a too-small target length
540f552 
Center derives its tail cut as stringLength - targetLength/2 + ilength,
which runs off the end of the string instead of underflowing, so
clamping targetLength up to the indicator length never rescues it: any
target below twice the indicator still raises NSRangeException.

- Derive Center's head and tail lengths separately, and keep no tail at
  all once the indicator alone fills half the budget
- Cap the head at the string length, for an indicator longer than the
  string it is truncating
- Cover all three modes at target lengths 0 through 10, the range that
  throws without this change
@hannesa2

hannesa2 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Screenshot differs: emulatorApi=arm64 with 4 screenshot(s)

setpoint (old) | diff | actual screenshot (new)

00-commit-context-menu.pngscreenshot

01-history-view.pngscreenshot

02-main-window.pngscreenshot

03-staging-view.pngscreenshot

@goneng

goneng commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

@hannesa2 , "center truncation" is also covered now - I think we can merge this one. Thanks!

@hannesa2
hannesa2 merged commit f096405 into master Sep 15, 2026
3 checks passed
@hannesa2
hannesa2 deleted the ReviewOf625 branch September 15, 2026 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goneng goneng Awaiting requested review from goneng

Assignees

No one assigned

Labels

test Test related

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hannesa2 @goneng