Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[shaping] Trailing spaces #164

Merged
merged 2 commits into from
May 5, 2024
Merged

[shaping] Trailing spaces #164

merged 2 commits into from
May 5, 2024

Conversation

benoitkugler
Copy link
Contributor

@benoitkugler benoitkugler commented May 4, 2024

Due to a missing reference operator (&), the trailing whitespaces were not collapsed properly.

An interesting side effect of this change was to discover that some tests used one Output several times, without copying its glyphs, leading to spurious errors when the glyphs were mutated.

@benoitkugler benoitkugler added the bug Something isn't working label May 4, 2024
@benoitkugler benoitkugler changed the title [shaping] Trailing space [shaping] Trailing spaces May 4, 2024
Copy link
Contributor

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

I don't fully understand the ramification around the copy but this seems sensible.

@benoitkugler
Copy link
Contributor Author

I don't fully understand the ramification around the copy but this seems sensible.

Some tests use the same Output as input value for line wrapping, mutating this input, so that the next test has an input which is not the one we intended. In pseudo code, this is :

input := Output{...}
result1 := LineWrap(input, settings1) // woups, we are actually mutating the input glyph slice
result2 := LineWrap(input, settings2) // test failed, since input has changed

@benoitkugler
Copy link
Contributor Author

I'll go ahead and merge, since this is a bug fix with no external API change.

@benoitkugler benoitkugler merged commit 6b7f526 into main May 5, 2024
20 checks passed
Copy link
Member

@whereswaldon whereswaldon left a comment

Choose a reason for hiding this comment

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

Thanks @benoitkugler !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants