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

Add support to set text wrapping #13

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

brooksvb
Copy link

Closes issue #5

Pretty straightforward, just adding a way to set text wrapping on the internal Paragraph widget created when rendering the TextArea.

I tested with the examples, and tested with the search function using the editor example. Text wrapping worked as expected without any apparent interference with the search functionality. Passes tests and linting checks.

Feedback is welcome.

@brooksvb
Copy link
Author

brooksvb commented Mar 29, 2023

I almost decided to just have a bool for the wrap field, with the assumption that if you want text wrapping, you don't want trim: true, otherwise space at the beginning and end of lines does not get rendered. Instead of making this assumption, I allowed users to directly use the Wrap struct instead, so they can make that decision.

@rhysd
Copy link
Owner

rhysd commented Apr 4, 2023

Thank you for trying to make a patch. However this doesn't work because tui-textarea is managing scroll position by itself and it assumes height is equal to number of lines. I think you would find it if you run cargo run --example editor --features=search tui-textarea/README.md in a narrow terminal and scroll down the screen by Ctrl+N.

@brooksvb
Copy link
Author

brooksvb commented Apr 5, 2023

I actually just came across this in my own program using my patch. Came back here to comment about it and saw you responded today. Sorry I didn't catch it sooner; I'll look into the scrolling part to understand it and fix the issue.

@brooksvb
Copy link
Author

brooksvb commented Apr 9, 2023

I've been cooking my brain over this for days and I'm finally starting to see the light.

The behavior I have aimed for is as follows:
1.1. The top row shown in the viewport will always be the start of a line (this has an undesirable side effect, but simplifies some of the logic for now). This was done because the surrounding implementation sort of requires that the top row is set to a line index.

1.2. The cursor line must always fit inside the viewport, including the wrapped lines. This may require scrolling multiple rows at a time until the entire cursor line fits

1.3 I changed the wrap setting back to a boolean, assuming that the user will never want wrap with whitespace trimming. It seems like a really niche use-case and will complicate calculations for line wraps.

1.4 Any changes for wrapping should have nearly no impact on the performance of non-wrapping use-cases

Known issues:
2.1. The calculation used for the number of line wraps a line will result in is not accurate in all cases:
In this example, it calculates the line as occupying 2 rows:
image

2.2. As a side effect of the earlier point 1, when a line that wraps goes up out of the viewport, it will scroll multiple lines at once. While this seems to make sense for the case of point 1.2, it feels a little more jarring when multiple lines get scrolled for a cursor line that only occupies a single line. I think fixing this will require scrolling the paragraph widget vertically, and knowing how much to scroll.

2.3 On point 1.2, it is probably undesirable to scroll the entire line into view in an application where a user is writing long paragraphs of prose. In some cases I think it is nice, so perhaps the behavior should be an option that can be set. However, to implement this behavior while keeping the cursor on screen, there will need to be calculations done for awareness of which wrapped row the cursor is on within a line. I'm not yet sure I want to implement that in this PR, as it's going to be another chunk of work.

I plan to iron out these issues, and then try to clean up the code to make it as clear as possible. If you have any feedback on what I have so far, I'd appreciate it. I'm particularly concerned about terminology being clear, but maybe that's best left for the end after I make my code as clear and concise as possible.

I will also try writing some tests for my code to validate cursor movements resulting in the correct viewport scroll position.

@joshka
Copy link
Contributor

joshka commented May 11, 2023

I wonder if you'd have any thoughts to add at ratatui/ratatui#174 regarding scrolling from the things you've learnt from this issue?

@erak
Copy link

erak commented Aug 25, 2023

Hey @brooksvb, are you still working on this? I'd be more than happy to help bringing this over the finish line 🙂

@brooksvb
Copy link
Author

Hey @brooksvb, are you still working on this? I'd be more than happy to help bringing this over the finish line 🙂

I wanted to finish it but it continued to balloon in complexity and I ran out of steam while the rest of my life got quite complicated and hectic for a while. 😅

You're more than welcome to take over to get it the rest of the way!

@erak
Copy link

erak commented Aug 28, 2023

Hey @brooksvb, are you still working on this? I'd be more than happy to help bringing this over the finish line 🙂

I wanted to finish it but it continued to balloon in complexity and I ran out of steam while the rest of my life got quite complicated and hectic for a while. 😅

You're more than welcome to take over to get it the rest of the way!

Thanks for your reply! It would be very helpful to get a jump start 🙂. Could you write up a short summary of the current state? Like, things that need to be done, nice-to-haves, known issues etc.

@deepu105
Copy link

deepu105 commented Oct 2, 2023

This would be great to have

@brooksvb
Copy link
Author

@erak sorry for the late response. It's been hectic. I would suggest reading the code I've written to understand the current state. I tried to leave good comments for the confusing parts, and a decent commit history for "blame" investigation.

This comment #5 (comment) also contains some good background for the task. It seems @rhysd is considering working on this feature at some point and had some ideas, so maybe get input from them if you're planning to work on this.

@rhysd rhysd force-pushed the main branch 6 times, most recently from 2202e41 to 39075bb Compare November 29, 2023 13:01
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.

5 participants