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

Truncate the starts of file paths instead of the ends in picker #951

Merged
merged 5 commits into from
Nov 4, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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/picker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ impl<T: 'static> Component for Picker<T> {
text_style
},
true,
true,
);
}
}
Expand Down
79 changes: 57 additions & 22 deletions helix-tui/src/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,12 +266,14 @@ impl Buffer {
where
S: AsRef<str>,
{
self.set_string_truncated(x, y, string, width, style, false)
self.set_string_truncated(x, y, string, width, style, false, false)
}

/// Print at most the first `width` characters of a string if enough space is available
/// until the end of the line. If `markend` is true appends a `…` at the end of
/// truncated lines.
/// until the end of the line. If `ellipsis` is true appends a `…` at the end of
/// truncated lines. If `truncate_start` is `true`, truncate the beginning of the string
/// instead of the end.
#[allow(clippy::too_many_arguments)]
pub fn set_string_truncated<S>(
&mut self,
x: u16,
Expand All @@ -280,6 +282,7 @@ impl Buffer {
width: usize,
style: Style,
ellipsis: bool,
truncate_start: bool,
) -> (u16, u16)
where
S: AsRef<str>,
Expand All @@ -289,28 +292,60 @@ impl Buffer {
let width = if ellipsis { width - 1 } else { width };
let graphemes = UnicodeSegmentation::graphemes(string.as_ref(), true);
let max_offset = min(self.area.right() as usize, width.saturating_add(x as usize));
for s in graphemes {
let width = s.width();
if width == 0 {
continue;
if !truncate_start {
for s in graphemes {
let width = s.width();
if width == 0 {
continue;
}
// `x_offset + width > max_offset` could be integer overflow on 32-bit machines if we
// change dimenstions to usize or u32 and someone resizes the terminal to 1x2^32.
if width > max_offset.saturating_sub(x_offset) {
break;
}

self.content[index].set_symbol(s);
self.content[index].set_style(style);
// Reset following cells if multi-width (they would be hidden by the grapheme),
for i in index + 1..index + width {
self.content[i].reset();
}
index += width;
x_offset += width;
}
// `x_offset + width > max_offset` could be integer overflow on 32-bit machines if we
// change dimenstions to usize or u32 and someone resizes the terminal to 1x2^32.
if width > max_offset.saturating_sub(x_offset) {
break;
if ellipsis && x_offset - (x as usize) < string.as_ref().width() {
self.content[index].set_symbol("…");
}

self.content[index].set_symbol(s);
self.content[index].set_style(style);
// Reset following cells if multi-width (they would be hidden by the grapheme),
for i in index + 1..index + width {
self.content[i].reset();
} else {
let mut truncated = false;
let mut end = Vec::new();
Copy link
Contributor

@pickfire pickfire Nov 2, 2021

Choose a reason for hiding this comment

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

I think we can do better than collecting iterating over this twice and having a separate allocation.

I think probably can just get the index and display the rest. Didn't look closely.

Good idea but I think for files it might be better to truncate the directories, like single character for each directory in path. How fish truncate the directory may be a better idea. But that would probably require separate allocation.

This is a good start and should be fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since individual graphemes can have different widths, I'm not sure how to do this without iterating over it in reverse and checking the total width.

single character for each directory in path

Ah, that's a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to just calculate the total width using unicode_width::UnicodeWidthStr, no need to segment it into graphemes:

UnicodeWidthStr::width(g).max(1)

We re-export the library as helix_core::unicode::width

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, it's the same method you're already using (.width()). Should be enough to call string.width(), then if it's larger than width we iterate graphemes from reverse, rendering them into content (without using the end vector)

Copy link
Member

Choose a reason for hiding this comment

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

I pushed a rewrite with ^ but it's untested. Probably needs more work.

Copy link
Contributor Author

@Omnikar Omnikar Nov 4, 2021

Choose a reason for hiding this comment

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

And it panics when the paths are long enough to actually need to be truncated. Or rather, panics when compiling in debug mode, since it's a subtract overflow panic, which just rolls over in release mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, addressing this fixes the panic.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I guess that codepath always gets called even if the string is shorter.

Copy link
Member

Choose a reason for hiding this comment

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

We'd preferably use the original code under !truncate_start if the width is shorter

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 think I've figured out what to do.

let mut total_width = 0;
for s in graphemes.rev() {
let width = s.width();
if total_width + width > max_offset.saturating_sub(x_offset) {
truncated = true;
break;
}
end.push(s);
total_width += width;
}
if ellipsis && truncated {
self.content[index].set_symbol("…");
index += 1;
}
for s in end.into_iter().rev() {
let width = s.width();
if width == 0 {
continue;
}
self.content[index].set_symbol(s);
self.content[index].set_style(style);
for i in index + 1..index + width {
self.content[i].reset();
}
index += width;
x_offset += width;
}
index += width;
x_offset += width;
}
if ellipsis && x_offset - (x as usize) < string.as_ref().width() {
self.content[index].set_symbol("…");
}
(x_offset as u16, y)
}
Expand Down