Skip to content

Fix optimality when negative values are in use#105

Merged
robertknight merged 1 commit intorobertknight:masterfrom
basil:fix
Aug 3, 2025
Merged

Fix optimality when negative values are in use#105
robertknight merged 1 commit intorobertknight:masterfrom
basil:fix

Conversation

@basil
Copy link
Contributor

@basil basil commented Aug 2, 2025

@robertknight I spoke too soon in #103. We were fine against restriction 2, but not against restriction 1: a particular optimization relied on restriction 1. I've fixed that here by removing that optimization when restriction 1 is present. It is demonstrated in a new unit test, which fails like this before the fix to src/layout.ts:

Chrome Headless 127.0.6533.88 (Linux x86_64) layout breakLines does not lose the optimum when negative values are present FAILED
        AssertionError: expected [ +0, 1, 6 ] to deeply equal [ +0, 3, 6 ]
            at Context.<anonymous> (build/tests.bundle.js:13506:27)

After the fix to src/layout.ts, the new unit test passes. While I was here, I also added comments explaining the reasoning for why we are safe with regard to both Restrictions 1 and 2.

Copy link
Owner

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

Thanks again for the detailed notes.

//
// Line-length = 10
//
// ┌─12─┐○───────○┌─-2─┐○────○┌──9──┐○──────○
Copy link
Owner

Choose a reason for hiding this comment

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

I missed that the middle box had a width of -2 rather than 2 until I read the code below.

@robertknight robertknight merged commit baa5d24 into robertknight:master Aug 3, 2025
1 check passed
@basil
Copy link
Contributor Author

basil commented Aug 3, 2025

Thanks @robertknight! Could this please be released?

@robertknight
Copy link
Owner

@basil
Copy link
Contributor Author

basil commented Aug 4, 2025

Thank you very much!

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