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

Avoid wrapping a space onto its own newline #211

Closed
wants to merge 1 commit into from

Conversation

grovesNL
Copy link
Contributor

Fixes #155

There was some behavior added in #78 that intentionally seems to break trailing spaces in this way, but it doesn't seem be necessary in the test cases I've tried (mostly just experimenting with the rich-text example).

If anyone has a test case that this regresses, please let me know! As far as I can tell this is an improvement for typical cases.

@grovesNL
Copy link
Contributor Author

cargo-deny failure is unrelated - caused by multiple version of syn and bitflags

@grovesNL
Copy link
Contributor Author

grovesNL commented Jan 15, 2024

It seems to cause test_english_mixed_with_arabic_paragraph_rendering to fail - looking into this now

Edit: the test output changed from this

Before
some_english_mixed_with_arabic_before

After (more space after render)
some_english_mixed_with_arabic_after

It looks like the space might be wider than expected so this might be a regression, but I don't think that was the intent of the original code path.

@jackpot51
Copy link
Member

@grovesNL unfortunately that does look like a regression. Hopefully it is not too hard to fix.

@alokedesai
Copy link

@grovesNL lmk if there's anyway I can help get this PR past the finish line.

I just spent quite a few quite a few hours debugging a text layout issue in Warp that ultimately seems to be caused by this. Would love to see this fixed and happy to lend a hand if it would useful.

@grovesNL
Copy link
Contributor Author

@alokedesai absolutely, any help looking into this would be great! I was planning to step through the positioning in a debugger to see how we end up with two spaces there but might not have time until another week or so.

Based on the test case failure it seems like there's some kind of unexpected interaction when we have left-to-right and right-to-left text spans that end up on the same visual line.

@grovesNL
Copy link
Contributor Author

grovesNL commented Feb 2, 2024

#226 fixes this without the regression

@grovesNL grovesNL closed this Feb 2, 2024
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.

Wrap::Word inserts newline for trailing space (after word)
3 participants