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

Highlight matching text in file picker suggestions #1635

Merged
merged 4 commits into from
Mar 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 2 additions & 2 deletions helix-term/src/ui/editor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -688,12 +688,12 @@ impl EditorView {
surface.set_string_truncated(
viewport.x + 8, // 8: 1 space + 3 char mode string + 1 space + 1 spinner + 1 space
viewport.y,
title,
&title,
viewport
.width
.saturating_sub(6)
.saturating_sub(right_side_text.width() as u16 + 1) as usize, // "+ 1": a space between the title and the selection info
base_style,
|_| base_style,
true,
true,
);
Expand Down
40 changes: 27 additions & 13 deletions helix-term/src/ui/picker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use crate::ui::{Prompt, PromptEvent};
use helix_core::{movement::Direction, Position};
use helix_view::{
editor::Action,
graphics::{Color, CursorKind, Margin, Rect, Style},
graphics::{Color, CursorKind, Margin, Modifier, Rect, Style},
Document, Editor,
};

Expand Down Expand Up @@ -343,7 +343,7 @@ impl<T> Picker<T> {
}
// TODO: maybe using format_fn isn't the best idea here
let text = (self.format_fn)(option);
// TODO: using fuzzy_indices could give us the char idx for match highlighting
// Highlight indices are computed lazily in the render function
self.matcher
.fuzzy_match(&text, pattern)
.map(|score| (index, score))
Expand Down Expand Up @@ -483,6 +483,8 @@ impl<T: 'static> Component for Picker<T> {

fn render(&mut self, area: Rect, surface: &mut Surface, cx: &mut Context) {
let text_style = cx.editor.theme.get("ui.text");
let selected = cx.editor.theme.get("ui.text.focus");
let highlighted = cx.editor.theme.get("special").add_modifier(Modifier::BOLD);

// -- Render the frame:
// clear area
Expand Down Expand Up @@ -525,29 +527,41 @@ impl<T: 'static> Component for Picker<T> {
// subtract area of prompt from top and current item marker " > " from left
let inner = inner.clip_top(2).clip_left(3);

let selected = cx.editor.theme.get("ui.text.focus");

let rows = inner.height;
let offset = self.cursor - (self.cursor % std::cmp::max(1, rows as usize));

let files = self.matches.iter().skip(offset).map(|(index, _score)| {
(index, self.options.get(*index).unwrap()) // get_unchecked
});
let files = self
.matches
.iter_mut()
.skip(offset)
.map(|(index, _score)| (*index, self.options.get(*index).unwrap()));

for (i, (_index, option)) in files.take(rows as usize).enumerate() {
if i == (self.cursor - offset) {
let is_active = i == (self.cursor - offset);
if is_active {
surface.set_string(inner.x.saturating_sub(2), inner.y + i as u16, ">", selected);
}

let formatted = (self.format_fn)(option);

let (_score, highlights) = self
.matcher
.fuzzy_indices(&formatted, &self.prompt.line)
.unwrap_or_default();

surface.set_string_truncated(
inner.x,
inner.y + i as u16,
(self.format_fn)(option),
&formatted,
inner.width as usize,
if i == (self.cursor - offset) {
selected
} else {
text_style
|idx| {
if highlights.contains(&idx) {
highlighted
} else if is_active {
selected
} else {
text_style
}
},
true,
self.truncate_start,
Expand Down
70 changes: 55 additions & 15 deletions helix-tui/src/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -287,27 +287,24 @@ impl Buffer {
where
S: AsRef<str>,
{
self.set_string_truncated(x, y, string, width, style, false, false)
self.set_string_truncated_at_end(x, y, string.as_ref(), width, style)
}

/// Print at most the first `width` characters of a string if enough space is available
/// 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>(
pub fn set_string_truncated(
&mut self,
x: u16,
y: u16,
string: S,
string: &str,
width: usize,
style: Style,
style: impl Fn(usize) -> Style, // Map a grapheme's string offset to a style
Copy link
Member

Choose a reason for hiding this comment

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

This is a significant hotpath and we'll also get some code bloat from monomorphization. Why not just do this in render() by calling set_string multiple times? I'm only comfortable with this change if the base case (fixed style) gets reasonably inlined by rust.

Copy link
Contributor Author

@Aloso Aloso Feb 9, 2022

Choose a reason for hiding this comment

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

Why not just do this in render() by calling set_string multiple times?

Because then I'd have to implement truncating the string (and adding an ellipsis if necessary) manually.

Code bloat is limited, since set_string_truncated is only called three times. To ensure the hotpath doesn't get slower, I'd actually prefer replacing the usage in set_stringn (which always sets ellipsis and truncate_start to false) with a specialized function, so set_string_truncated is no longer a hotpath. This would be much simpler and might even be more efficient than the current implementation.

@archseer what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @archseer

Copy link
Member

Choose a reason for hiding this comment

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

I'll finish my review later this weekend

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite like the duplication but it's fine for now 👍🏻

ellipsis: bool,
truncate_start: bool,
) -> (u16, u16)
where
S: AsRef<str>,
{
) -> (u16, u16) {
// prevent panic if out of range
if !self.in_bounds(x, y) || width == 0 {
return (x, y);
Expand All @@ -316,10 +313,10 @@ impl Buffer {
let mut index = self.index_of(x, y);
let mut x_offset = x as usize;
let width = if ellipsis { width - 1 } else { width };
let graphemes = UnicodeSegmentation::graphemes(string.as_ref(), true);
let graphemes = string.grapheme_indices(true);
let max_offset = min(self.area.right() as usize, width.saturating_add(x as usize));
if !truncate_start {
for s in graphemes {
for (byte_offset, s) in graphemes {
let width = s.width();
if width == 0 {
continue;
Expand All @@ -331,22 +328,22 @@ impl Buffer {
}

self.content[index].set_symbol(s);
self.content[index].set_style(style);
self.content[index].set_style(style(byte_offset));
// 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;
}
if ellipsis && x_offset - (x as usize) < string.as_ref().width() {
if ellipsis && x_offset - (x as usize) < string.width() {
self.content[index].set_symbol("…");
}
} else {
let mut start_index = self.index_of(x, y);
let mut index = self.index_of(max_offset as u16, y);

let total_width = string.as_ref().width();
let total_width = string.width();
let truncated = total_width > width;
if ellipsis && truncated {
self.content[start_index].set_symbol("…");
Expand All @@ -355,7 +352,7 @@ impl Buffer {
if !truncated {
index -= width - total_width;
}
for s in graphemes.rev() {
for (byte_offset, s) in graphemes.rev() {
let width = s.width();
if width == 0 {
continue;
Expand All @@ -365,7 +362,7 @@ impl Buffer {
break;
}
self.content[start].set_symbol(s);
self.content[start].set_style(style);
self.content[start].set_style(style(byte_offset));
for i in start + 1..index {
self.content[i].reset();
}
Expand All @@ -375,6 +372,49 @@ impl Buffer {
(x_offset as u16, y)
}

/// Print at most the first `width` characters of a string if enough space is available
/// until the end of the line.
pub fn set_string_truncated_at_end(
&mut self,
x: u16,
y: u16,
string: &str,
width: usize,
style: Style,
) -> (u16, u16) {
// prevent panic if out of range
if !self.in_bounds(x, y) {
return (x, y);
}

let mut index = self.index_of(x, y);
let mut x_offset = x as usize;
let max_x_offset = min(self.area.right() as usize, width.saturating_add(x as usize));

for s in string.graphemes(true) {
let width = s.width();
if width == 0 {
continue;
}
// `x_offset + width > max_offset` could be integer overflow on 32-bit machines if we
// change dimensions to usize or u32 and someone resizes the terminal to 1x2^32.
if width > max_x_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 as u16, y)
}

pub fn set_spans<'a>(&mut self, x: u16, y: u16, spans: &Spans<'a>, width: u16) -> (u16, u16) {
let mut remaining_width = width;
let mut x = x;
Expand Down