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

Add virtual text support #417

Closed
wants to merge 2 commits into from
Closed
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
61 changes: 55 additions & 6 deletions helix-term/src/ui/editor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ use helix_core::{
},
movement::Direction,
syntax::{self, HighlightEvent},
unicode::width::UnicodeWidthStr,
unicode::{segmentation::UnicodeSegmentation, width::UnicodeWidthStr},
LineEnding, Position, Range, Selection, Transaction,
};
use helix_view::{
decorations::{TextAnnotation, TextAnnotationKind},
document::{Mode, SCRATCH_BUFFER_NAME},
editor::{CompleteAction, CursorShapeConfig},
graphics::{Color, CursorKind, Modifier, Rect, Style},
Expand Down Expand Up @@ -395,6 +396,11 @@ impl EditorView {
// It's slightly more efficient to produce a full RopeSlice from the Rope, then slice that a bunch
// of times than it is to always call Rope::slice/get_slice (it will internally always hit RSEnum::Light).
let text = doc.text().slice(..);
let last_line = std::cmp::min(
// Saturating subs to make it inclusive zero indexing.
(offset.row + viewport.height as usize).saturating_sub(1),
doc.text().len_lines().saturating_sub(1),
);

let characters = &whitespace.characters;

Expand Down Expand Up @@ -450,6 +456,37 @@ impl EditorView {
}
};

// It's slightly more efficient to produce a full RopeSlice from the Rope, then slice that a bunch
// of times than it is to always call Rope::slice/get_slice (it will internally always hit RSEnum::Light).
let text = text.slice(..);
Comment on lines +459 to +461
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// It's slightly more efficient to produce a full RopeSlice from the Rope, then slice that a bunch
// of times than it is to always call Rope::slice/get_slice (it will internally always hit RSEnum::Light).
let text = text.slice(..);

this is a noop (text is already a RopeSlice

let out_of_bounds = |visual_x: u16| {
visual_x < offset.col as u16 || visual_x >= viewport.width + offset.col as u16
};

let render_annotation =
|annot: &TextAnnotation, line: u16, pos: u16, surface: &mut Surface| {
let mut visual_x = pos;
for grapheme in annot.text.graphemes(true) {
if out_of_bounds(visual_x) {
break;
}
surface.set_string(
viewport.x + visual_x - offset.col as u16,
viewport.y + line,
grapheme,
annot.style,
);
visual_x = visual_x.saturating_add(grapheme.width() as u16);
}
};

let text_annotations = doc
.text_annotations()
.iter()
.flat_map(|(_, annots)| annots)
.filter(|annot| (offset.row..last_line).contains(&annot.line))
.collect::<Vec<_>>();

'outer: for event in highlights {
match event {
HighlightEvent::HighlightStart(span) => {
Expand Down Expand Up @@ -488,22 +525,27 @@ impl EditorView {
use helix_core::graphemes::{grapheme_width, RopeGraphemes};

for grapheme in RopeGraphemes::new(text) {
let out_of_bounds = visual_x < offset.col as u16
|| visual_x >= viewport.width + offset.col as u16;

if LineEnding::from_rope_slice(&grapheme).is_some() {
if !out_of_bounds {
if !out_of_bounds(visual_x) {
// we still want to render an empty cell with the style
surface.set_string(
viewport.x + visual_x - offset.col as u16,
viewport.y + line,
&newline,
style.patch(whitespace_style),
);
visual_x += 1;
Copy link
Member

Choose a reason for hiding this comment

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

Why was this not necessary before, but is now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe because it is due to the addition of virtual text.

But this += 1 does not seemed to support double width characters like CJK, otherwise I think it shouldn't always += 1 and sometimes it should += 2.

Copy link
Member

Choose a reason for hiding this comment

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

I think this works as intended in all cases.

visual_x point is placed after the last character (always incremented by the correct grapheme width in the else clause). The visual_x += 1 only moves the virtual text to the right by one cell so there is space between the annotation and the content of the line. It might be nice to configure the amount of space in the future but I think that can be done in a followup PR.

This was not required before this PR because visual_x is reset back to 0 right after this condition (as a new line starts) on master. However this PR still requires the visual_x (plus the space) to render the EOL annotations.

}

draw_indent_guides(last_line_indent_level, line, surface);

if let Some(annot) = text_annotations
.iter()
.find(|t| t.kind.is_eol() && t.line == offset.row + line as usize)
{
render_annotation(annot, line, visual_x, surface);
}

visual_x = 0;
line += 1;
is_in_indent_area = true;
Expand Down Expand Up @@ -540,7 +582,7 @@ impl EditorView {

let cut_off_start = offset.col.saturating_sub(visual_x as usize);

if !out_of_bounds {
if !out_of_bounds(visual_x) {
// if we're offscreen just keep going until we hit a new line
surface.set_string(
viewport.x + visual_x - offset.col as u16,
Expand Down Expand Up @@ -582,6 +624,13 @@ impl EditorView {
}
}
}

for annot in &text_annotations {
if let TextAnnotationKind::Overlay(visual_x) = annot.kind {
let line = (annot.line - offset.row) as u16;
render_annotation(annot, line, visual_x as u16, surface);
}
}
}

/// Render brace match, etc (meant for the focused view only)
Expand Down
32 changes: 32 additions & 0 deletions helix-view/src/decorations.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
use std::borrow::Cow;

use crate::graphics::Style;

#[derive(Clone, Copy, PartialEq)]
pub enum TextAnnotationKind {
/// Add to end of line
Eol,
/// Replace actual text or arbitary cells with annotations.
/// Specifies an offset from the 0th column.
Overlay(usize),
sudormrfbin marked this conversation as resolved.
Show resolved Hide resolved
}

impl TextAnnotationKind {
pub fn is_eol(&self) -> bool {
*self == Self::Eol
}

pub fn is_overlay(&self) -> bool {
matches!(*self, Self::Overlay(_))
}
}

/// Namespaces and identifes similar annotations
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Namespaces and identifes similar annotations
/// Namespaces to identify similar annotations

pub type TextAnnotationGroup = &'static str;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be an enum instead of a string, since it will be known at compile time anyway.

pub enum TextAnnotationGroup {
    Diagnostic,
    JumpMode,
}

Then hashing or b-tree look up & filtering will be fast, or possibly even faster would be to store annotations directly into a named field:

struct DocumentAnnotations {
    diagnostics: Vec<TextAnnotation>,
    jump_mode: Vec<TextAnnotation>,
}

pub struct Document {
    // -----8<-----
    text_annotations: DocumentAnnotations,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, another review note: Document implements Debug manually, so consider adding the text_annotations field there as well

https://github.com/helix-editor/helix/pull/417/files#diff-36a64f6c432dc47d11c27851879a0b67459d908edfaa70262aefc21171dc62b9R129

Copy link
Contributor

Choose a reason for hiding this comment

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

I think making a TextAnnotationGroup enum would make it hard to use annotations from a plugin, once a plugin system is implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's a fair point! it's certainly not a blocking issue.

to reap benefits (which may be slight?) of the enum approach and allow plugins to add annotations, something like this would work:

/// first-party annotation groups (only used to differentiate access to the Vecs below)
pub enum TextAnnotationGroup {
    Diagnostic,
    JumpMode,
}

/// plugin annotation groups
/// (might have to reserve "diagnostic" and "jump_mode" strs?
pub type TextAnnotationPluginGroup = &'static str;

struct DocumentAnnotations {
    diagnostics: Vec<TextAnnotation>,
    jump_mode: Vec<TextAnnotation>,
    from_plugins: BTreeMap<&'static str, TextAnnotation>,
}

pub struct Document {
    // -----8<-----
    text_annotations: DocumentAnnotations,
}

This comment was marked as off-topic.


pub struct TextAnnotation {
pub text: Cow<'static, str>,
pub style: Style,
pub line: usize,
pub kind: TextAnnotationKind,
}
31 changes: 30 additions & 1 deletion helix-view/src/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ use helix_core::{
DEFAULT_LINE_ENDING,
};

use crate::{DocumentId, Editor, ViewId};
use crate::decorations::TextAnnotationGroup;
use crate::Editor;
use crate::{decorations::TextAnnotation, DocumentId, ViewId};

/// 8kB of buffer space for encoding and decoding `Rope`s.
const BUF_SIZE: usize = 8192;
Expand Down Expand Up @@ -119,6 +121,7 @@ pub struct Document {
pub(crate) modified_since_accessed: bool,

diagnostics: Vec<Diagnostic>,
text_annotations: HashMap<TextAnnotationGroup, Vec<TextAnnotation>>,
Copy link
Member

Choose a reason for hiding this comment

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

Might be more performant to implement this as a BTreeMap in tne future with the lines as keys.
This would make iterating a small range of lines (during rendering) more efficient when there are a large amount of annotations.
Can probably be done in a followup PR tough

language_server: Option<Arc<helix_lsp::Client>>,
}

Expand Down Expand Up @@ -351,6 +354,7 @@ impl Document {
language: None,
changes,
old_state,
text_annotations: HashMap::new(),
diagnostics: Vec::new(),
version: 0,
history: Cell::new(History::default()),
Expand Down Expand Up @@ -1038,6 +1042,31 @@ impl Document {
.map(helix_core::path::get_relative_path)
}

pub fn text_annotations(&self) -> &HashMap<TextAnnotationGroup, Vec<TextAnnotation>> {
&self.text_annotations
}

pub fn push_text_annotations<I: Iterator<Item = TextAnnotation>>(
&mut self,
group: TextAnnotationGroup,
annots: I,
) {
self.text_annotations
.entry(group)
.or_default()
.extend(annots);
}

pub fn clear_text_annotations(&mut self, group: TextAnnotationGroup) {
if let Some(annots) = self.text_annotations.get_mut(group) {
annots.clear()
}
}

// pub fn slice<R>(&self, range: R) -> RopeSlice where R: RangeBounds {
Copy link
Member

Choose a reason for hiding this comment

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

Still needed?

// self.state.doc.slice
// }

// transact(Fn) ?

// -- LSP methods
Expand Down
1 change: 1 addition & 0 deletions helix-view/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
pub mod macros;

pub mod clipboard;
pub mod decorations;
pub mod document;
pub mod editor;
pub mod graphics;
Expand Down