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

RFC: Text Wrapping Design #293

Open
1 of 6 tasks
joshka opened this issue Jul 2, 2023 · 19 comments
Open
1 of 6 tasks

RFC: Text Wrapping Design #293

joshka opened this issue Jul 2, 2023 · 19 comments
Labels
enhancement New feature or request needs specification An issue that isn't yet designed or specificied well enough to implement request for comment

Comments

@joshka
Copy link
Member

joshka commented Jul 2, 2023

Request for comment

The intent of this issue is to generate feedback and incorporate that into the design of wrapping. Recently there have been a few PRs that have attempted to tackle the existing state of the reflow module, and in doing so they change the existing behavior in order to implement new functionality. At the same time, we have multiple overlapping feature requests around wrapping (and scrolling).

Problems

  1. The requirements of wrapping / truncation / boundaries / trimming and how they should interact with text, lines, and widgets are unclear.
  2. The reflow module code is fairly convoluted and difficult to understand, add new functionality to, and to generally maintain. We've recently added some new features, and want to add more.
  3. Without a clear end state for wrapping, the refactoring PRs tend to be held up discussing which features should be supported and how rather than just reviewing the implementation.
  4. Wrapping is currently supported only in the Paragraph widget.
  5. Wrapping does not expose any information about the wrapped state.
  6. Wrapping cannot wrap on a character boundary.
  7. Wrapping does not support adding indentation
  8. We (currently at least) have to write a wrapping implementation rather than relying on e.g. textwrap

Functional Requirements

  1. Configuration is a single type using builders
  2. Individual Lines can be wrapped
  3. Text can be wrapped
  4. Widgets that contain Text / Lines can be wrapped
  5. Wrapping configuration on Line / Text will override Widget wrapping configuration
  6. Support Character and Word boundaries
  7. Operate on a subset of lines (e.g. visible scrolled lines)
  8. Can trim leading space from lines that are wrapped
  9. Can indent lines that are wrapped by a certain amount
  10. Can indent lines that are wrapped using a specific string
  11. Wrapping and Horizontal alignment interact reasonably
  12. Vertical alignment should be considered
  13. (If possible) all wrapping logic should wrap textwrap
  14. Result should be cacheable to avoid duplicating work each frame.

Non-functional requirements

  1. Reasonable complexity. The complexity of the implementation should approach the complexity of the algorithm (right now the code to algorithm size ratio is about 10:1)
  2. Reasonably performant (60fps = 16ms -> should be much less than this)
  3. Backwards compatible. Implement as an addition to the existing code, not a replacement.
  4. Deprecation of existing implementation. We will mark existing wrap methods deprecated to make it easy for developers to upgrade their code to the new approach.
  5. Maintainability is more important than perfect fidelity with previous implementation (i.e. if using textwrap causes wrap points to change so be it if it brings clarity to the implementation).

Questions / Tasks

  • Search for existing related issues / PRs
  • Ensure that all requirements in related issues are noted
  • Are there any missing requirements?
  • Are any requirements not possible?
  • Are any requirements not wanted / problematic
  • Are any requirements unclear?

Links

From tui-rs:

From Ratatui:

(The quantity of issues / PRs related to wrapping should hint at why this is non-trivial)

Solution (WIP)

  1. Define a new type text::Wrap for configuring the wrapping
  2. Add a new method to Line, Text, and any widgets that support wrapping to be configured with Wrap
  3. Convert Paragraph::wrap() to take Into<text::Wrap> and define a conversion from the existing Wrap type.
  4. Implement Line::wrapped_lines(...) which returns an iterator of lines based on the configuration and any other necessary state (e.g. width / height)
  5. Implement Text::wrapped_lines(...) which does similar, (TBD understand how to pass in scroll info)
  6. Implement each Widget's render_wrapped_lines() method.

Edits

2023-06-05: Added more links, split functional / non-functional and simplified reqs

@joshka joshka added enhancement New feature or request request for comment needs specification An issue that isn't yet designed or specificied well enough to implement labels Jul 2, 2023
@joshka
Copy link
Member Author

joshka commented Jul 2, 2023

Pinging @lthoerner @TimerErTim @topaxi

@topaxi
Copy link

topaxi commented Jul 2, 2023

On point 8. We should be able to indent lines that are wrapped by a certain amount. I'd say we (or at least I) would want it to be by a given string/prefix. Amount implies to me it might be a number of spaces, but I have a case where I'd want a specific string as the indentation.

Example:

2023-07-02 | This is my wrapping
             | Text.

So the indentation would be " | ". As the consumer of the API I'd be able to format the string repetition pretty easily.

@haydenflinner
Copy link

I am interested in this refactor, maybe my use-case can help.

I am building a pager (think file-viewer) like less, but with more features and while remaining responsive even on files larger than RAM and with minimal resource usage. To that end I am actually only handing a processed-string-slice to ratatui's Paragraph, having discarded GB of data before and after it myself. This works pretty well because newlines are a solid delimiter for logs. I only use Paragraph to word-wrap and display some chunk of text I've already selected, and I handle scrolling on my end due to complicated requirements.

My problem is that I have no way of knowing how much ratatui actually decided to show on screen, or where it wrapped. This is a problem because I've implemented my own scrolling, so if my madeup wrapping (simple/fast space-splitting) doesn't match ratatui's, then sometimes scrolling down scrolls a visual line (we matched) and sometimes it scrolls a file-line/pre-wrapping line (we didn't match). Further, I'm not sure how to replace the wrapping with my own code, which I'd like to do just because I think the algo that's there is too complicated. I don't imagine user-pluggable wrapping is something really desired at this point, just better tools already there, like a simple space-splitting algo.

@joshka
Copy link
Member Author

joshka commented Jul 3, 2023

My problem is that I have no way of knowing how much ratatui actually decided to show on screen, or where it wrapped.

One approach of solving that is to track both the first (real) line scrolled plus the offset of the line, so the rendering of lines becomes:

lines.iter()
  .skip(index)
  .flat_map(|line| line.wrap(width etc))
  .skip(offset)
  .take(height)
  .enumerate()
  .foreach(|(line, y)| buffer.set_line(line, area.x, area.y + y, line.style());

By using a stateful widget rendering, then we can update the scroll position for situations like where we want to scroll up from (index = 100, offset = 0) -> (index = 100, offset = -1) which will be the same as (index = 99, offset = number_of_wrapped_lines_in_line_99_which_only_render_knows)

@lthoerner
Copy link
Contributor

Existing methods that take the current wrap will transition to taking an Intotext::wrap

This will not really work because the trim field is on Paragraph, not Wrap.

@TimerErTim
Copy link
Contributor

Existing methods that take the current wrap will transition to taking an Intotext::wrap

This will not really work because the trim field is on Paragraph, not Wrap.

Last time I checked trim is a field of Wrap. Has there be any changes to that?

@lthoerner
Copy link
Contributor

Last time I checked trim is a field of Wrap. Has there be any changes to that?

Yeah, the two versions are incompatible because it has been changed from a field of Wrap to a field of Paragraph.

@joshka
Copy link
Member Author

joshka commented Jul 3, 2023

That change to trim is on the PR for wrapping, so It hasn't changed yet. I'm not sure it should be changed given the rationale for it existing (fdehau/tui-rs#327). Briefly stated, the trim option was introduced in order to make it possible to maintain the indentation of lines that will be wrapped to support wrapping e.g.:

First line
  - no wrap
    - A long line that wraps

to:

First line
  - no wrap
    - A long line
that wraps

instead of:

First line
  - no wrap
- A long line
that wraps

i.e. it should only effect lines that are wrapped. Not those that don't. I'd suggest that there's a better way of specifying that behavior that would incorporate the indentation as some enum that could have values like: WrapToFirstColumn, WrapToFirstNonWhitespaceColumn, WrapWithPrefix(" |"), WrapWithFunction(fn(Line)->Span) etc.

@joshka
Copy link
Member Author

joshka commented Jul 5, 2023

Updated with significant amount more links, and better requirements.

@joshka joshka changed the title Conslidated wrapping design RFC: Text Wrapping Design Jul 7, 2023
@joshka
Copy link
Member Author

joshka commented Jul 7, 2023

There's an approach that is suggested in https://docs.rs/textwrap/0.16.0/textwrap/wrap_algorithms/fn.wrap_first_fit.html and https://docs.rs/textwrap/0.16.0/textwrap/core/index.html that would be very useful (and would remove most of the actual wrapping code from Ratatui).

At a high level, we would need to:

  1. Split the Spans into Fragments (which are the smallest pieces of wrappable stuff - e.g. a word / grapheme)
  2. Consider whether to support hyphenation (fine not to start)
  3. Call wrap_first_fit or wrap_optimal_fit on the spans for the line
  4. Render the lines

@lthoerner
Copy link
Contributor

WrapToFirstColumn, WrapToFirstNonWhitespaceColumn, WrapWithPrefix(" |"), WrapWithFunction(fn(Line)->Span)

While I agree with the sentiment behind this, I think it may be too complicated. I like the Wrap::CharBoundary and Wrap::WordBoundary with the trim field. If anything, we should make trim an enum with options like Trim::PreseveIndentation, Trim::DiscardIndentation, and Trim::CustomIndentation(String).

@joshka
Copy link
Member Author

joshka commented Sep 28, 2023

an enum with options like Trim::PreseveIndentation, Trim::DiscardIndentation, and Trim::CustomIndentation(String).

This is definitely better.

@lthoerner do you have any insight into the requirements that are necessary for character based wrapping - perhaps a list of functional requirements like above?. I'd imagine that being able to add indent text would be handy for the terminal like:

~> command "some argument in quotes that w
quote> raps to the next line, by prependin
quote> g the prefix 'quote>'"

@lthoerner
Copy link
Contributor

@lthoerner do you have any insight into the requirements that are necessary for character based wrapping - perhaps a list of functional requirements like above?. I'd imagine that being able to add indent text would be handy for the terminal like:

A few things:

  1. The new wrapping implementation must be built to support both Widget and StatefulWidget. At this time, it is impossible to use Ratatui for text editors without the concession that editing large files is basically unsupported (that is to say, immediate rendering breaks down once thousands of lines must be wrapped on each frame update).
  2. I think the approach we took with Line where the previous implementation was deprecated and will be removed in a release or two is the right one. I don't think we should leave LineComposer in for more than another two versions if at all possible.
  3. LineComposer (or, at least, the current implementation of soft-wrapping) had a sort of unexpected behavior where, if I remember correctly, either a space was trimmed unexpectedly or an extra space was inserted. I think it would be advisable to provide release notes on this just in case people depend on this behavior. Additionally, keep in mind that the current test cases are built to match this behavior and will need to be changed.
  4. I like the idea of using a crate for this but we should investigate how well textwrap supports non-single-width graphemes, as this seems to be somewhat of a priority for Ratatui.

I can't really speak to what implementation I would prefer, but my approach was to implement a trait on text types that allowed them to be wrapped with a method.

@joshka
Copy link
Member Author

joshka commented Sep 29, 2023

Thanks for these

  1. The new wrapping implementation must be built to support both Widget and StatefulWidget. At this time, it is impossible to use Ratatui for text editors without the concession that editing large files is basically unsupported (that is to say, immediate rendering breaks down once thousands of lines must be wrapped on each frame update).

I think this is probably covered by:

  • FR 7. Operate on a subset of lines (e.g. visible scrolled lines).
  • NFR 2. Reasonably performant (60fps = 16ms -> should be much less than this)

If the implementation of this is at the Line / Text level, then I think what you're calling out here is that there needs to be a way to ensure that the scroll position is well handled by the wrapping algorithm

  • I think the approach we took with Line where the previous implementation was deprecated and will be removed in a release or two is the right one. I don't think we should leave LineComposer in for more than another two versions if at all possible.

That approach is definitely the way for public APIs. If we have tests that cover the same behavior, and can show that it's unchanged by the new implementation (or that changes are reasonable fixes for bugs) do you think we can go faster in removing that code?

  • LineComposer (or, at least, the current implementation of soft-wrapping) had a sort of unexpected behavior where, if I remember correctly, either a space was trimmed unexpectedly or an extra space was inserted. I think it would be advisable to provide release notes on this just in case people depend on this behavior. Additionally, keep in mind that the current test cases are built to match this behavior and will need to be changed.

Definitely - The way to go with this is to ensure that the tests adequately cover the edge cases of the reflow module by testing from the perspective of paragraph rather than testing the internals. Doing this prior to making any changes to rip out Line Composer is important. That way any change in behavior shows a simultaneous change in a test rather than the implementation being inseparable from the testing.

  • I like the idea of using a crate for this but we should investigate how well textwrap supports non-single-width graphemes, as this seems to be somewhat of a priority for Ratatui.

Textwrap just cares about the width of the items. The example it uses to highlight that text isn't constricting is to model tasks on a building site which have time to complete, between tasks, and to cleanup at the end of day.

I can't really speak to what implementation I would prefer, but my approach was to implement a trait on text types that allowed them to be wrapped with a method.

At a high level that sounds good - can you remind me how the interface would look like for that?

For character boundary wrapping, I think there's situations where you want to keep the whitespace always right? Are there any edge cases that you're aware of there?

@lthoerner
Copy link
Contributor

I think this is probably covered by:
FR 7. Operate on a subset of lines (e.g. visible scrolled lines).
NFR 2. Reasonably performant (60fps = 16ms -> should be much less than this)

Not entirely. FR 7 as written will end up with potentially unexpected or unpredictable behavior when parts of the text are updated, particularly ones that aren't on screen. It would be a good idea to look at how existing editors do this.

At a high level that sounds good - can you remind me how the interface would look like for that?

All I will say is that I ran into issues with implementing it as a trait where different implementations needed different arguments.

@joshka
Copy link
Member Author

joshka commented Sep 29, 2023

I think this is probably covered by:
FR 7. Operate on a subset of lines (e.g. visible scrolled lines).
NFR 2. Reasonably performant (60fps = 16ms -> should be much less than this)

Not entirely. FR 7 as written will end up with potentially unexpected or unpredictable behavior when parts of the text are updated, particularly ones that aren't on screen. It would be a good idea to look at how existing editors do this.

Got it. That sounds like something that has a part of it out of scope for the wrapping implementation, but the part that matters for wrapping is the interaction with scrolling, so I think you're probably saying that we need to add something like:

  1. Widgets that implement both scrolling and wrapping should have some means of ensuring that that the UI presents a stable view.

Does that sound about right?

@joshka
Copy link
Member Author

joshka commented Jan 25, 2024

Inspired by https://discord.com/channels/1070692720437383208/1070692720437383211/1200159591107932272 an extra requirement to consider for this is to cache the wrapped lines to avoid duplicating work on each frame.

@kxxt
Copy link
Contributor

kxxt commented Apr 27, 2024

I didn't read the entire discussion so sorry if this has been mentioned before.

There are two characters(at least that's what I am aware of) that needs to be handled carefully: NBSP(Non breaking white space, U+00A0) and ZWSP(Zero width white space, U+200B, https://unicode-explorer.com/c/200B).

My use case is wrapping help text that shows the key binding:

[NBSP]Ctrl+S[NBSP][NBSP]Switch[NBSP]Pane[NBSP]
[ZWSP]
[NBSP]↑/↓/←/→/Pg{Up,Dn}[NBSP][NBSP]Navigate[NBSP]
[ZWSP]
[NBSP]Ctrl+<-/->[NBSP][NBSP]Scroll<->[NBSP]
[ZWSP]
[NBSP]G/S[NBSP][NBSP]Grow/Shrink[NBSP]Pane[NBSP]
[ZWSP]
[NBSP]Alt+L[NBSP][NBSP]Layout[NBSP]
...

I don't want a key binding be break down in the middle like this:

image

And in the mean while I inserted ZWSP to indicate a possible text wrapping point.

It seems for now(ratatui 0.26.2) the wrapping feature of Paragraph respect neither of the two special characters.

Peek 2024-04-27 22-36

I think my use case is fairly common so I would appreciate it if anyone have existing workaround for such use cases.

And note that ZWSP is not a white space character recognized by char::is_whitespace, char::is_whitespace('\u{200b}') returns false.

@kxxt
Copy link
Contributor

kxxt commented Apr 27, 2024

And note that ZWSP is not a white space character recognized by char::is_whitespace, char::is_whitespace('\u{200b}') returns false.

Well, it seems that this is the only reason that blocks ZWSP from being supported. I made a PR to fix it: #1074

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs specification An issue that isn't yet designed or specificied well enough to implement request for comment
Projects
None yet
Development

No branches or pull requests

6 participants