Skip to content

feat: wrap lines longer than the terminal width #364

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jonathanj
Copy link
Contributor

@jonathanj jonathanj commented May 1, 2025

Previously long lines would be truncated, now instead they are wrapped and a wrap indicator displayed at the front of each line, after the first one.

The wrap indicator and collapse indicators are configurable.

image

Fixes #277

@jonathanj
Copy link
Contributor Author

I have added tests and snapshots for the wrapping behaviour, to demonstrate some aspects of it like the cursor taking up multiple lines, etc. but I haven't updated all the existing snapshots because there is a potentially controversial change:

I opted to widen the "gutter" (where the cursor is displayed) to 2 columns, one for the cursor and one for the wrap indicator. From my perspective showing the wrap indicator is a much better user experience than showing nothing, because otherwise it feels like the cursor behaves inconsistently (since you can't always see what's a wrap without the indicator.)

I wanted to indent the wrapped lines by 1, so that I could make the extra gutter space only when needed, but Paragraph in ratatui does not support this yet. I also tried a suggestion to use the textwrap crate, but that doesn't preserve styled Ratatui spans. There is also this gist that implements custom ratatui wrapping, but that seemed a bit overkill. So widening the gutter by 1 seemed like the lesser evil.

The problem is that this would require me to update all the other snapshots, and I wanted to get your opinion on this before going off and bloating the PR with all of those changes.

@jonathanj
Copy link
Contributor Author

jonathanj commented May 1, 2025

As far as the implementation goes, I don't know if this is a good approach. I thought that using ratatui's Paragraph to render this with wrapping would be fairly quick win (which it was), and the rest of the changes are mostly just making sure that line_index is incremented according to the additional screen lines needed for wrapping, and stretching out the cursor, etc.

measure_paragraph_height might seem a bit odd, but I couldn't find a way to have ratatui tell me correctly how many lines will be required to wrap some text, and that's necessary to configure the render rect/area. I tried out the unstable line_count method, but that kept giving me extra newlines for reasons I don't understand.

image

Edit: I think there is a bug in line_count when a line contains only whitespace, going to report it upstream but it probably means line_count is not a usable option at the moment.

@jonathanj jonathanj force-pushed the 277-text-wrapping branch 2 times, most recently from 04177a3 to 0aa0c2f Compare May 2, 2025 07:35
@jonathanj
Copy link
Contributor Author

I realised that since there's now space at the front of the line, and the first line doesn't show a wrap indicator, it's possible to move the collapsed indicator there instead now. I didn't actually make this change, but here's what it could look like.

image image

Previously long lines would be truncated, now instead they are wrapped and a
wrap indicator displayed at the front of each line, after the first one.

The wrap indicator and collapse indicators are configurable.
@jonathanj jonathanj force-pushed the 277-text-wrapping branch from 0aa0c2f to ee07e89 Compare May 2, 2025 12:08
@altsem
Copy link
Owner

altsem commented May 2, 2025

Text wrapping is a great addition!

About adding an exta column to the gutter, I suppose it's possible.
An alternative idea I got was to replace the blue pipe on the left (selection indicator) with . After all, I suppose you wouldn't be able to select a continuation of a line (ctrl+j, ctrl+k) anyway:

▌@@ -25,9 +25,9 @@ fn copy_hash(r: String) -> Option<Action> {
▌         match &mut state.clipboard {
▌             Some(cb) => {
▌                 cb.set_text(r.clone()).map_err(Error::Clipboard)?;
▌-                state.display_info("Commit hash copied to clipboar
↪d".to_owned());
▌+                state.display_info("Commit hash copied to clipboar
↪d");
▌             }

Or even doing both?

▌ @@ -25,9 +25,9 @@ fn copy_hash(r: String) -> Option<Action> {
▌          match &mut state.clipboard {
▌              Some(cb) => {
▌                  cb.set_text(r.clone()).map_err(Error::Clipboard)?;
▌ -                state.display_info("Commit hash copied to clipboar
 ↪d".to_owned());
▌ +                state.display_info("Commit hash copied to clipboar
 ↪d");
▌              }

One currently annoying thing though, if you double-click to select a whole word in a terminal, say you want to select and copy the commit hash:

▌da1d201 main yada yada

Then this would be selected: ▌da1d201

Termwiz

I've been peeking at perhaps switching over and rewriting the rendering code to use Termwiz entirely instead. It's more low-level, but also feels more intuitive in that widgets can actually have a determined size (Ratatui seems to be all around deciding the size of containers first, and then the widget will have to make use of the size its given).

Navigation

I think there is the assumption in this module when it comes to navigation where it assumes an Item is always a single line. It might be that there's some weird behavior upon navigating if a lot of line-wrapping is in place.
Perhaps some preprocessing could be done to accommodate for this? There is already a notion of mapping visual lines to items like:

Line | Item | Content
6      6      ▌modified   src/state.rs
7      7      ▌@@ -87,15 +87,7 @@ impl State {…
8      24     ▌@@ -108,9 +100,27 @@ impl State {
9      25     ▌             current_cmd_log: CmdLog::new(),

Found here:

gitu/src/screen/mod.rs

Lines 235 to 256 in 6b9e124

fn update_line_index(&mut self) {
self.line_index = self
.items
.iter()
.enumerate()
.scan(None, |collapse_depth, (i, next)| {
if collapse_depth.is_some_and(|depth| depth < next.depth) {
return Some(None);
}
*collapse_depth = if next.section && self.is_collapsed(next) {
Some(next.depth)
} else {
None
};
Some(Some((i, next)))
})
.flatten()
.map(|(i, _item)| i)
.collect();
}

@jonathanj
Copy link
Contributor Author

jonathanj commented May 2, 2025

Or even doing both?

Leaving the cursor/selection out for a continuation is a good idea.

One currently annoying thing though, if you double-click to select a whole word in a terminal, say you want to select and copy the commit hash:

Then this would be selected: ▌da1d201

Adding the extra gutter column seems to improve this, at least in my terminal.

I think there is the assumption in this module when it comes to navigation where it assumes an Item is always a single line. It might be that there's some weird behavior upon navigating if a lot of line-wrapping is in place. Perhaps some preprocessing could be done to accommodate for this? There is already a notion of mapping visual lines to items like:

The navigation seems to work okay, at least as far as I could tell. What could I try to produce a mismatched line issue? I figured that since the line wrapping is happening only at the render point, that what happens in effect is that the virtual lines (for items) are modelled as single lines (that can never wrap), but at the point they become characters in the terminal then they end up being wrapped, although the model logic doesn't know (or care) that the renderer does that.

@jonathanj jonathanj force-pushed the 277-text-wrapping branch from 38016e3 to afd3c4d Compare May 2, 2025 21:28
@altsem
Copy link
Owner

altsem commented May 3, 2025

One example is when navigating below the visible view, the view should scroll along with the cursor. But if enough lines are wrapped, then it would be delayed.

There is logic in the screen module that makes assumptions about navigation from the size of the view. The size property of Screen.

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.

Text wrap git diff
2 participants