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

Highlight matching text in file picker suggestions #1635

Merged
merged 4 commits into from
Mar 1, 2022
Merged

Highlight matching text in file picker suggestions #1635

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4756606
Highlight matching text in file picker suggestions
Aloso Feb 8, 2022
615cfcd
Remove cache, specialize highlighting code
Aloso Feb 9, 2022
2c717ce
Fix outdated comments
Aloso Feb 25, 2022
fe065b7
Merge remote-tracking branch 'upstream' into highlight-suggestions
Aloso Feb 25, 2022
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
2 changes: 1 addition & 1 deletion helix-term/src/ui/editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,7 @@ impl EditorView {
.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
60 changes: 39 additions & 21 deletions helix-term/src/ui/picker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use crate::ui::{Prompt, PromptEvent};
use helix_core::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 @@ -266,8 +266,8 @@ pub struct Picker<T> {
options: Vec<T>,
// filter: String,
matcher: Box<Matcher>,
/// (index, score)
matches: Vec<(usize, i64)>,
/// (index, score, highlight indices)
Aloso marked this conversation as resolved.
Show resolved Hide resolved
matches: Vec<(usize, i64, Option<Vec<usize>>)>,
Aloso marked this conversation as resolved.
Show resolved Hide resolved
/// Filter over original options.
filters: Vec<usize>, // could be optimized into bit but not worth it now

Expand Down Expand Up @@ -334,13 +334,13 @@ 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 and cached
Aloso marked this conversation as resolved.
Show resolved Hide resolved
self.matcher
.fuzzy_match(&text, pattern)
.map(|score| (index, score))
.map(|score| (index, score, None))
}),
);
self.matches.sort_unstable_by_key(|(_, score)| -score);
self.matches.sort_unstable_by_key(|(_, score, _)| -score);

// reset cursor position
self.cursor = 0;
Expand All @@ -367,13 +367,13 @@ impl<T> Picker<T> {
pub fn selection(&self) -> Option<&T> {
self.matches
.get(self.cursor)
.map(|(index, _score)| &self.options[*index])
.map(|(index, _score, _highlights)| &self.options[*index])
}

pub fn save_filter(&mut self) {
self.filters.clear();
self.filters
.extend(self.matches.iter().map(|(index, _)| *index));
.extend(self.matches.iter().map(|(index, _, _)| *index));
self.filters.sort_unstable(); // used for binary search later
self.prompt.clear();
}
Expand Down Expand Up @@ -465,6 +465,8 @@ impl<T: 'static> Component for Picker<T> {
};

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 @@ -507,29 +509,45 @@ 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
});

for (i, (_index, option)) in files.take(rows as usize).enumerate() {
if i == (self.cursor - offset) {
let files = self
.matches
.iter_mut()
.skip(offset)
.map(|(index, _score, highlights)| {
(*index, self.options.get(*index).unwrap(), highlights)
});

for (i, (_index, option, highlights)) in files.take(rows as usize).enumerate() {
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 highlights = highlights.get_or_insert_with(|| {
self.matcher
.fuzzy_indices(&formatted, &self.prompt.line)
.unwrap_or_default()
.1
});

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
14 changes: 7 additions & 7 deletions helix-tui/src/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ impl Buffer {
where
S: AsRef<str>,
{
self.set_string_truncated(x, y, string, width, style, false, 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
Expand All @@ -301,7 +301,7 @@ impl Buffer {
y: u16,
string: S,
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 •
edited
Loading

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)
Expand All @@ -316,10 +316,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.as_ref().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,7 +331,7 @@ 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();
Expand All @@ -355,7 +355,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 +365,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 Down