From 5349571ad786fd5393ec46b08ffaa1b79a22f3d0 Mon Sep 17 00:00:00 2001 From: Wilfred Hughes Date: Sat, 23 Oct 2021 19:33:22 -0700 Subject: [PATCH] Only store prev_opposite_pos on novel positions This is much easier to reason about, rather than overloading opposite_pos and prev_opposite_pos. --- src/inline.rs | 19 ++++++++------ src/lines.rs | 48 +++++++++++++++++++++++++++++----- src/style.rs | 2 +- src/syntax.rs | 71 +++++++++++++++------------------------------------ 4 files changed, 75 insertions(+), 65 deletions(-) diff --git a/src/inline.rs b/src/inline.rs index a331bef58..bb5763657 100644 --- a/src/inline.rs +++ b/src/inline.rs @@ -36,11 +36,13 @@ fn last_lhs_context_line( // If we don't have changes on the LHS, find the line opposite the // last RHS unchanged node in this hunk. for rhs_position in rhs_positions { - if rhs_position.kind.is_unchanged() { - continue; - } + let opposite_pos = match &rhs_position.kind { + MatchKind::Unchanged { opposite_pos, .. } => opposite_pos.0.clone(), + MatchKind::Novel { .. } | MatchKind::ChangedCommentPart { .. } => continue, + MatchKind::UnchangedCommentPart { opposite_pos } => opposite_pos.clone(), + }; - if let Some(pos) = rhs_position.prev_opposite_pos { + if let Some(pos) = opposite_pos.first() { if pos.line.0 > lhs_hunk_end.0 { break; } @@ -88,12 +90,13 @@ fn first_rhs_context_line( let mut lhs_rev_positions: Vec<_> = lhs_positions.into(); lhs_rev_positions.reverse(); for lhs_position in lhs_rev_positions { - match lhs_position.kind { - MatchKind::Unchanged { .. } => {} + let opposite_pos = match lhs_position.kind { + MatchKind::Unchanged { opposite_pos, .. } => opposite_pos.0, + // TODO: handle UnchangedCommentPart here too? _ => break, - } + }; - if let Some(pos) = lhs_position.prev_opposite_pos { + if let Some(pos) = opposite_pos.first() { last_change_line = Some(pos.line); } } diff --git a/src/lines.rs b/src/lines.rs index cf67880d6..e2a9e2e05 100644 --- a/src/lines.rs +++ b/src/lines.rs @@ -1,6 +1,10 @@ //! Manipulate lines of text and groups of lines. -use crate::{intervals::Interval, positions::SingleLineSpan, syntax::MatchedPos}; +use crate::{ + intervals::Interval, + positions::SingleLineSpan, + syntax::{MatchKind, MatchedPos}, +}; use lazy_static::lazy_static; use regex::Regex; use std::{ @@ -164,8 +168,16 @@ impl LineGroup { return true; } } + + let opposite_pos: Vec = match &mp.kind { + MatchKind::Unchanged { opposite_pos, .. } => opposite_pos.1.clone(), + MatchKind::Novel { prev_opposite_pos } => prev_opposite_pos.clone(), + MatchKind::UnchangedCommentPart { opposite_pos } => opposite_pos.clone(), + MatchKind::ChangedCommentPart { prev_opposite_pos } => prev_opposite_pos.clone(), + }; + if let (Some(first_opposite), Some(opposite_group_lines)) = - (mp.prev_opposite_pos, opposite_group_lines) + (opposite_pos.first(), opposite_group_lines) { if first_opposite.line.0 <= opposite_group_lines.end.0 + max_gap { return true; @@ -206,11 +218,37 @@ impl LineGroup { fn add_lhs(&mut self, mp: &MatchedPos) { self.add_lhs_pos(Some(mp.pos)); - self.add_rhs_pos(mp.prev_opposite_pos); + + let opposite_pos: Vec = match &mp.kind { + MatchKind::Unchanged { opposite_pos, .. } => { + // TODO: should we take the open or close delim pos + // here? + opposite_pos.1.clone() + } + MatchKind::Novel { prev_opposite_pos } => prev_opposite_pos.clone(), + MatchKind::UnchangedCommentPart { opposite_pos } => opposite_pos.clone(), + MatchKind::ChangedCommentPart { prev_opposite_pos } => prev_opposite_pos.clone(), + }; + + // TODO: first or last here? + self.add_rhs_pos(opposite_pos.first().copied()); } fn add_rhs(&mut self, mp: &MatchedPos) { self.add_rhs_pos(Some(mp.pos)); - self.add_lhs_pos(mp.prev_opposite_pos); + + let opposite_pos: Vec = match &mp.kind { + MatchKind::Unchanged { opposite_pos, .. } => { + // TODO: should we take the open or close delim pos + // here? + opposite_pos.1.clone() + } + MatchKind::Novel { prev_opposite_pos } => prev_opposite_pos.clone(), + MatchKind::UnchangedCommentPart { opposite_pos } => opposite_pos.clone(), + MatchKind::ChangedCommentPart { prev_opposite_pos } => prev_opposite_pos.clone(), + }; + + // TODO: first or last here? + self.add_lhs_pos(opposite_pos.first().copied()); } pub fn max_visible_lhs(&self) -> LineNumber { @@ -517,7 +555,6 @@ mod tests { start_col: 0, end_col: 1, }, - prev_opposite_pos: None, }]; let rhs_positions = vec![MatchedPos { kind: MatchKind::Unchanged { @@ -529,7 +566,6 @@ mod tests { start_col: 0, end_col: 1, }, - prev_opposite_pos: None, }]; let res = visible_groups(&lhs_positions, &rhs_positions); diff --git a/src/style.rs b/src/style.rs index e7327ac57..f59936fd7 100644 --- a/src/style.rs +++ b/src/style.rs @@ -110,7 +110,7 @@ pub fn apply_colors(s: &str, is_lhs: bool, positions: &[MatchedPos]) -> String { bold: highlight == TokenKind::Atom(AtomKind::Keyword), dimmed: highlight == TokenKind::Atom(AtomKind::Comment), }, - MatchKind::Novel | MatchKind::ChangedCommentPart => Style { + MatchKind::Novel { .. } | MatchKind::ChangedCommentPart { .. } => Style { foreground: if is_lhs { Color::BrightRed } else { diff --git a/src/syntax.rs b/src/syntax.rs index fde855be0..89f2ba00c 100644 --- a/src/syntax.rs +++ b/src/syntax.rs @@ -490,6 +490,7 @@ pub enum TokenKind { Atom(AtomKind), } +/// A matched token (an atom, a delimiter, or a comment word). #[derive(PartialEq, Eq, Debug, Clone)] pub enum MatchKind { Unchanged { @@ -497,11 +498,15 @@ pub enum MatchKind { // as this match could be for a list. opposite_pos: (Vec, Vec), }, - Novel, + Novel { + prev_opposite_pos: Vec, + }, UnchangedCommentPart { opposite_pos: Vec, }, - ChangedCommentPart, + ChangedCommentPart { + prev_opposite_pos: Vec, + }, } impl MatchKind { @@ -514,9 +519,6 @@ impl MatchKind { pub struct MatchedPos { pub kind: MatchKind, pub pos: SingleLineSpan, - // TODO: this is confusing: the previous syntax node with a match - // may be on the current line or a previous one. - pub prev_opposite_pos: Option, } fn split_words(s: &str) -> Vec { @@ -551,13 +553,17 @@ fn split_comment_words( diff::Result::Left(word) => { // This word is novel to this side. res.push(MatchedPos { - kind: MatchKind::ChangedCommentPart, + kind: MatchKind::ChangedCommentPart { + // TODO: this feels wrong. It should be the + // previous comment part position, not the + // previous token position. + prev_opposite_pos: prev_opposite_pos.to_vec(), + }, pos: content_newlines.from_offsets_relative_to( pos, offset, offset + word.len(), )[0], - prev_opposite_pos: prev_opposite_pos.first().copied(), }); offset += word.len(); } @@ -576,7 +582,6 @@ fn split_comment_words( opposite_pos: opposite_word_pos, }, pos: word_pos, - prev_opposite_pos: prev_opposite_pos.first().copied(), }); offset += word.len(); opposite_offset += opposite_word.len(); @@ -635,24 +640,18 @@ impl MatchedPos { opposite_pos, } } - Novel => MatchKind::Novel, + Novel => MatchKind::Novel { + prev_opposite_pos: prev_opposite_pos.to_vec(), + }, }; // Create a MatchedPos for every line that `pos` covers. + // TODO: what about opposite pos or prev oppoiste pos? let mut res = vec![]; - for (i, line_pos) in pos.iter().enumerate() { - // Try to take line N on the opposite side for line N in - // this position. If the opposite position is fewer lines, - // just take the last line. - let opposite_line_pos = prev_opposite_pos - .get(i) - .or_else(|| prev_opposite_pos.last()) - .copied(); - + for line_pos in pos { res.push(Self { kind: kind.clone(), pos: *line_pos, - prev_opposite_pos: opposite_line_pos, }); } @@ -1085,35 +1084,6 @@ mod tests { ); } - /// Ensure that we assign prev_opposite_pos even if the change is on the first node. - #[test] - fn test_prev_opposite_pos_first_node() { - let arena = Arena::new(); - - let atom = Syntax::new_atom( - &arena, - vec![SingleLineSpan { - line: 0.into(), - start_col: 2, - end_col: 3, - }], - "foo", - AtomKind::Normal, - ); - atom.set_change(ChangeKind::Novel); - let nodes: Vec<&Syntax> = vec![atom]; - - let positions = change_positions("irrelevant", "also irrelevant", &nodes); - assert_eq!( - positions[0].prev_opposite_pos, - Some(SingleLineSpan { - line: 0.into(), - start_col: 0, - end_col: 0 - }) - ); - } - #[test] fn test_comment_and_atom_differ() { let pos = vec![SingleLineSpan { @@ -1202,13 +1172,14 @@ mod tests { assert_eq!( res, vec![MatchedPos { - kind: MatchKind::ChangedCommentPart, + kind: MatchKind::ChangedCommentPart { + prev_opposite_pos: vec![], + }, pos: SingleLineSpan { line: 0.into(), start_col: 0, end_col: 3 }, - prev_opposite_pos: None, },] ); }