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

@@ -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);
         }
     }

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<SingleLineSpan> = 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<SingleLineSpan> = 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<SingleLineSpan> = 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);

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 {

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<SingleLineSpan>, Vec<SingleLineSpan>),
     },
-    Novel,
+    Novel {
+        prev_opposite_pos: Vec<SingleLineSpan>,
+    },
     UnchangedCommentPart {
         opposite_pos: Vec<SingleLineSpan>,
     },
-    ChangedCommentPart,
+    ChangedCommentPart {
+        prev_opposite_pos: Vec<SingleLineSpan>,
+    },
 }
 
 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<SingleLineSpan>,
 }
 
 fn split_words(s: &str) -> Vec<String> {
@@ -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,
             },]
         );
     }