Skip to content
This repository has been archived by the owner on Aug 6, 2023. It is now read-only.

Paragraph - line wrapping on word boundaries #103

Merged
merged 3 commits into from
Dec 22, 2018

Conversation

karolinepauls
Copy link
Contributor

There's a bunch of things to improve and test cases to add. LineWrapper is also ugly but I'll be making it prettier when I have tests.

The LineComposer trait implementations cannot be iterators because we attempt to be efficient so we only yield slices of the current line which lives only one "iteration".

The changeset doesn't modify the public API, however the behaviour of .wrap(true) has been changed to do wrapping on word boundaries. Since I used runtime polymorphism, we could allow users to inject their own wrapping logic but the interface is ugly (consumes and passes Style), so I'd keep it private.

We may want to reintroduce opt-in previous behaviour of wrapping regardless of previous behaviour but this would require to make .wrap take an enum.

@fdehau
Copy link
Owner

fdehau commented Dec 11, 2018

That looks very promising. I particularly enjoy the fact that the you were able to move some logic out of the draw method. Moreover, I think that we will be able to use those "line composers" in List too...

I don't mind that that the behavior of the wrap changes since I think it is what people usually expect from text wrapping. My first approach was the simplest solution I could think of with all the shortcomings that this implies ;).

With that, the changes requested to List and all the recent work from you and other contributors, I think we are good for a "0.4" release anyway.

@karolinepauls
Copy link
Contributor Author

So when writing tests I realised I had accidentally rewritten the old implementation of wrapping instead of line truncation. I fixed it by writing another composer but now we're left with the older one, usable but unused. We could actually expose it but it would require changing the public interface - wrap method.

I have to make text wrapping behave sanely when the width is very low - decide how to treat whitespace and whether to display double width characters even if the width is 1 (I don't think so - we should either skip all characters wider than the current width or we should refuse to render below 2 chars of width - but what if there are triple width characters?)

@karolinepauls karolinepauls changed the title [PREVIEW] Paragraph - line wrapping on word boundaries Paragraph - line wrapping on word boundaries Dec 14, 2018
@karolinepauls
Copy link
Contributor Author

Removed the unused composer, added test cases, added skipping of characters wider than the text area, corner cases.

@fdehau Ready for review.

@fdehau
Copy link
Owner

fdehau commented Dec 22, 2018

I finally had some time to review your proposed changes. This is well thought and properly tested. Going through the code, I've not found anything to fix. Thus thank you for this work and looking forward for other contributions of this quality ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants