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.

Sign up for GitHub

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

Jump to bottom

Allow truncation of table row text #6410

Closed
wants to merge 5 commits into from
Closed

Allow truncation of table row text #6410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
adb12ad
Truncate paths in the file picker
mWalrus Mar 22, 2023
772fd46
Skip truncation of single character cells in `Table`
mWalrus Mar 22, 2023
978e1a4
Move truncate check into `render_cell`
mWalrus Mar 22, 2023
b23024a
Move `Rect` definition into call to `render_cell`
mWalrus Mar 22, 2023
4992429
Simplify `Buffer::set_spans_truncated`
mWalrus Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions helix-term/src/ui/menu.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ impl<T: Item + 'static> Component for Menu<T> {
offset: scroll,
selected: self.cursor,
},
false,
);

if let Some(cursor) = self.cursor {
Expand Down
1 change: 1 addition & 0 deletions helix-term/src/ui/picker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,7 @@ impl<T: Item + 'static> Component for Picker<T> {
offset: 0,
selected: Some(cursor),
},
self.truncate_start,
);
}

Expand Down
13 changes: 13 additions & 0 deletions helix-tui/src/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,19 @@ impl Buffer {
(x_offset as u16, y)
}

pub fn set_spans_truncated(&mut self, x: u16, y: u16, spans: &Spans, width: u16) -> (u16, u16) {
let mut text = String::new();
let mut styles = vec![];

for span in &spans.0 {
text.push_str(span.content.as_ref());
span.styled_graphemes(span.style)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performing grapheme segmentation here is both slow and incorrect. The idx used below is not a grapheme index but a byte index. Instead you should create an iterator that is transversed whenever the current style range is exhausted.

The current implementation can easily lead to crashes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured this was a slow solution.
Could you elaborate a bit about what you mean and how I could go about correcting this? I'm pretty new to this kind of thing.

.for_each(|grapheme| styles.push(grapheme.style));
}

self.set_string_truncated(x, y, &text, width.into(), |idx| styles[idx], true, true)
}

pub fn set_spans(&mut self, x: u16, y: u16, spans: &Spans, width: u16) -> (u16, u16) {
let mut remaining_width = width;
let mut x = x;
Expand Down
20 changes: 16 additions & 4 deletions helix-tui/src/widgets/table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,13 @@ impl TableState {
impl<'a> Table<'a> {
// type State = TableState;

pub fn render_table(mut self, area: Rect, buf: &mut Buffer, state: &mut TableState) {
pub fn render_table(
mut self,
area: Rect,
buf: &mut Buffer,
state: &mut TableState,
truncate: bool,
) {
if area.area() == 0 {
return;
}
Expand Down Expand Up @@ -401,6 +407,7 @@ impl<'a> Table<'a> {
width: *width,
height: max_header_height,
},
false,
);
col += *width + self.column_spacing;
}
Expand Down Expand Up @@ -457,27 +464,32 @@ impl<'a> Table<'a> {
width: *width,
height: table_row.height,
},
truncate,
);
col += *width + self.column_spacing;
}
}
}
}

fn render_cell(buf: &mut Buffer, cell: &Cell, area: Rect) {
fn render_cell(buf: &mut Buffer, cell: &Cell, area: Rect, truncate: bool) {
buf.set_style(area, cell.style);
for (i, spans) in cell.content.lines.iter().enumerate() {
if i as u16 >= area.height {
break;
}
buf.set_spans(area.x, area.y + i as u16, spans, area.width);
if cell.content.width() > 1 && truncate {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think the emptyness check is required here. Why did you add it?

In any case it should be using byte Len (or is_empty really) instead of grapheme width since it's O(1) and any non-empty string usually has a width (the cases where it doesn't can be negelagted IMO)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this check to not truncate table cells that have a content length of 1. If we don't have this check and try to render the buffer picker you will see that the first two cells (the entry number and the you-are-here indicator) are being truncated as well since my current implementation doesn't distinguish between cells but applies to the entire row.
image

With that said, there are probably better ways of doing this, this was just the fix I could come up with without diving too deep into it.

buf.set_spans_truncated(area.x, area.y + i as u16, spans, area.width);
} else {
buf.set_spans(area.x, area.y + i as u16, spans, area.width);
}
}
}

impl<'a> Widget for Table<'a> {
fn render(self, area: Rect, buf: &mut Buffer) {
let mut state = TableState::default();
Table::render_table(self, area, buf, &mut state);
Table::render_table(self, area, buf, &mut state, false);
}
}

Expand Down