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

Make Merge behave consistently #4042

Closed
wants to merge 9 commits into from
Closed

Make Merge behave consistently #4042

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5b098c3
implement consistent behaviour for highlight overlays
pascalkuthe Sep 29, 2022
ed3cdd3
Apply stylistic suggestions from code review
pascalkuthe Oct 3, 2022
2c6f681
more elegant static dispatch
pascalkuthe Oct 3, 2022
c3f652f
remove unnecessary close from DiagnosticsOverlay
pascalkuthe Oct 3, 2022
25427ce
move unwrap from helix-core to helix-term
pascalkuthe Oct 3, 2022
1f26bca
move contents of overlay/tests.rs to overlay.rs
pascalkuthe Oct 3, 2022
87f1a17
fixup overlay test documentation
pascalkuthe Oct 3, 2022
c2bdbbf
check overlay invariants
pascalkuthe Oct 3, 2022
7ed579d
add missing assert
pascalkuthe Oct 3, 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
173 changes: 36 additions & 137 deletions helix-core/src/syntax.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ use crate::{
auto_pairs::AutoPairs,
chars::char_is_line_ending,
diagnostic::Severity,
graphemes::ensure_grapheme_boundary_next_byte,
regex::Regex,
transaction::{ChangeSet, Operation},
Rope, RopeSlice, Tendril,
};
pub use overlay::{monotonic_overlay, overlapping_overlay, Span};

use arc_swap::{ArcSwap, Guard};
use slotmap::{DefaultKey as LayerId, HopSlotMap};
Expand All @@ -25,6 +27,8 @@ use serde::{Deserialize, Serialize};

use helix_loader::grammar::{get_language, load_runtime_file};

mod overlay;

fn deserialize_regex<'de, D>(deserializer: D) -> Result<Option<Regex>, D::Error>
where
D: serde::Deserializer<'de>,
Expand Down Expand Up @@ -885,7 +889,7 @@ impl Syntax {
source: RopeSlice<'a>,
range: Option<std::ops::Range<usize>>,
cancellation_flag: Option<&'a AtomicUsize>,
) -> impl Iterator<Item = Result<HighlightEvent, Error>> + 'a {
) -> HighlightIter<'a> {
let mut layers = self
.layers
.iter()
Expand Down Expand Up @@ -1142,7 +1146,7 @@ pub enum Error {
}

/// Represents a single step in rendering a syntax-highlighted document.
#[derive(Copy, Clone, Debug)]
#[derive(Copy, Clone, Debug, PartialEq)]
pub enum HighlightEvent {
Source { start: usize, end: usize },
HighlightStart(Highlight),
Expand Down Expand Up @@ -1184,7 +1188,36 @@ struct LocalScope<'a> {
}

#[derive(Debug)]
struct HighlightIter<'a> {
pub struct CharacterHighlightIter<'a>(HighlightIter<'a>);

impl Iterator for CharacterHighlightIter<'_> {
type Item = HighlightEvent;

fn next(&mut self) -> Option<Self::Item> {
let res = match self.0.next()?.unwrap() {
// TODO: use byte slices directly
// convert byte offsets to char offset
HighlightEvent::Source { start, end } => {
let text = self.0.source;
let start = text.byte_to_char(ensure_grapheme_boundary_next_byte(text, start));
let end = text.byte_to_char(ensure_grapheme_boundary_next_byte(text, end));
HighlightEvent::Source { start, end }
}
event => event,
};

Some(res)
}
Copy link
Member

Choose a reason for hiding this comment

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

The old maps on HighlightIter were doing two things:

  1. Unwrapping the result (which currently always returns Ok)
    • I'm concerned that helix_core is not the appropriate place to be unwrapping. HighlightIter returns Result items so that in the future we can implement cancellation of highlighting (timer based for example). There isn't a concrete plan for that AFAIK so it's hard to say how/where the Result should be handled.
  2. Translating bytes to chars in Source events.
    • @archseer does the TODO for using byte slices applies anymore? I think we can just translate bytes to chars in the Iterator implementation for HighlightIter

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm concerned that helix_core is not the appropriate place to be unwrapping. HighlightIter returns Result items so that in the future we can implement cancellation of highlighting (timer based for example). There isn't a concrete plan for that AFAIK so it's hard to say how/where the Result should be handled.

I have moved unwrapping the closure back into helix-term in 25427ce altough it make the code simpler if we simply returned HighlightEvent from the HighlightIterator (panicking directly in the iterator) and then switched to a result once its actually required (because an unwrap is required everywhere the HighlighterIterator is used all call-sites will have to be changed in the future anyway). What do you think is better

@archseer does the TODO for using byte slices applies anymore? I think we can just translate bytes to chars in the Iterator implementation for HighlightIter

In markdown.rs the highlighting directly indexes into a string and therefore uses byte indices. Using char indices and translating back would be inefficient in that case so I am not sure there is a better solution right now.
Rewriting the rendering to use byte indices seems to amount to translating byte indices to chars anyways so there might not be a better approach.

}

impl<'a> HighlightIter<'a> {
pub fn to_chars(self) -> CharacterHighlightIter<'a> {
CharacterHighlightIter(self)
}
}

#[derive(Debug)]
pub struct HighlightIter<'a> {
source: RopeSlice<'a>,
byte_offset: usize,
cancellation_flag: Option<&'a AtomicUsize>,
Expand Down Expand Up @@ -1859,140 +1892,6 @@ fn injection_for_match<'a>(
(language_name, content_node, included_children)
}

pub struct Merge<I> {
iter: I,
spans: Box<dyn Iterator<Item = (usize, std::ops::Range<usize>)>>,

next_event: Option<HighlightEvent>,
next_span: Option<(usize, std::ops::Range<usize>)>,

queue: Vec<HighlightEvent>,
}

/// Merge a list of spans into the highlight event stream.
pub fn merge<I: Iterator<Item = HighlightEvent>>(
iter: I,
spans: Vec<(usize, std::ops::Range<usize>)>,
) -> Merge<I> {
let spans = Box::new(spans.into_iter());
let mut merge = Merge {
iter,
spans,
next_event: None,
next_span: None,
queue: Vec::new(),
};
merge.next_event = merge.iter.next();
merge.next_span = merge.spans.next();
merge
}

impl<I: Iterator<Item = HighlightEvent>> Iterator for Merge<I> {
type Item = HighlightEvent;
fn next(&mut self) -> Option<Self::Item> {
use HighlightEvent::*;
if let Some(event) = self.queue.pop() {
return Some(event);
}

loop {
match (self.next_event, &self.next_span) {
// this happens when range is partially or fully offscreen
(Some(Source { start, .. }), Some((span, range))) if start > range.start => {
if start > range.end {
self.next_span = self.spans.next();
} else {
self.next_span = Some((*span, start..range.end));
};
}
_ => break,
}
}

match (self.next_event, &self.next_span) {
(Some(HighlightStart(i)), _) => {
self.next_event = self.iter.next();
Some(HighlightStart(i))
}
(Some(HighlightEnd), _) => {
self.next_event = self.iter.next();
Some(HighlightEnd)
}
(Some(Source { start, end }), Some((_, range))) if start < range.start => {
let intersect = range.start.min(end);
let event = Source {
start,
end: intersect,
};

if end == intersect {
// the event is complete
self.next_event = self.iter.next();
} else {
// subslice the event
self.next_event = Some(Source {
start: intersect,
end,
});
};

Some(event)
}
(Some(Source { start, end }), Some((span, range))) if start == range.start => {
let intersect = range.end.min(end);
let event = HighlightStart(Highlight(*span));

// enqueue in reverse order
self.queue.push(HighlightEnd);
self.queue.push(Source {
start,
end: intersect,
});

if end == intersect {
// the event is complete
self.next_event = self.iter.next();
} else {
// subslice the event
self.next_event = Some(Source {
start: intersect,
end,
});
};

if intersect == range.end {
self.next_span = self.spans.next();
} else {
self.next_span = Some((*span, intersect..range.end));
}

Some(event)
}
(Some(event), None) => {
self.next_event = self.iter.next();
Some(event)
}
// Can happen if cursor at EOF and/or diagnostic reaches past the end.
// We need to actually emit events for the cursor-at-EOF situation,
// even though the range is past the end of the text. This needs to be
// handled appropriately by the drawing code by not assuming that
// all `Source` events point to valid indices in the rope.
(None, Some((span, range))) => {
let event = HighlightStart(Highlight(*span));
self.queue.push(HighlightEnd);
self.queue.push(Source {
start: range.start,
end: range.end,
});
self.next_span = self.spans.next();
Some(event)
}
(None, None) => None,
e => unreachable!("{:?}", e),
}
}
}

#[cfg(test)]
mod test {
use super::*;
Expand Down
Loading