refactor: improve the code by unifying the algorithmn of merging objects using the Merge trait

9 files changed, 124 insertions, 264 deletions

diff --git a/packages/svgbob/src/buffer/cell_buffer.rs b/packages/svgbob/src/buffer/cell_buffer.rs
index b5cdb5b..616cbd5 100644
--- a/packages/svgbob/src/buffer/cell_buffer.rs
+++ b/packages/svgbob/src/buffer/cell_buffer.rs
@@ -1,5 +1,6 @@
 use crate::buffer::fragment_buffer::FragmentSpan;
 use crate::fragment::CellText;
+use crate::Merge;
 use crate::Settings;
 use crate::{
     buffer::{fragment_buffer::FragmentTree, Fragment, StringBuffer},
diff --git a/packages/svgbob/src/buffer/cell_buffer/span.rs b/packages/svgbob/src/buffer/cell_buffer/span.rs
index 24caac3..8ded0ae 100644
--- a/packages/svgbob/src/buffer/cell_buffer/span.rs
+++ b/packages/svgbob/src/buffer/cell_buffer/span.rs
@@ -6,7 +6,7 @@ use crate::{
     },
     fragment,
     map::{circle_map, UNICODE_FRAGMENTS},
-    Cell, Fragment, Settings,
+    Cell, Fragment, Merge, Settings,
 };
 use itertools::Itertools;
 use std::{
@@ -66,9 +66,17 @@ impl Span {
         })
     }
 
-    pub(crate) fn merge(&mut self, other: &Self) {
-        self.extend_from_slice(&*other)
+    /*
+    pub(crate) fn merge(&self, other: &Self) -> Option<Self> {
+        if self.can_merge(other) {
+            let mut cells = self.0.clone();
+            cells.extend(&other.0);
+            Some(Span(cells))
+        } else {
+            None
+        }
     }
+    */
 
     /// paste the other Span at cell location `loc`
     pub fn paste_at(&self, loc: Cell, other: &Self) -> Self {
@@ -214,37 +222,6 @@ impl Span {
         self.iter().any(|(cell, ch)| *cell == needle)
     }
 
-    /// merge span recursively until it hasn't changed the number of spans
-    pub(crate) fn merge_recursive(adjacents: Vec<Span>) -> Vec<Span> {
-        let original_len = adjacents.len();
-        let merged = Self::second_pass_merge(adjacents);
-        // if has merged continue merging until nothing can be merged
-        if merged.len() < original_len {
-            Self::merge_recursive(merged)
-        } else {
-            merged
-        }
-    }
-
-    /// second pass merge is operating on span comparing to other spans
-    fn second_pass_merge(adjacents: Vec<Span>) -> Vec<Span> {
-        let mut new_groups: Vec<Span> = vec![];
-        for span in adjacents.into_iter() {
-            let is_merged = new_groups.iter_mut().rev().any(|new_group| {
-                if new_group.can_merge(&span) {
-                    new_group.merge(&span);
-                    true
-                } else {
-                    false
-                }
-            });
-            if !is_merged {
-                new_groups.push(span);
-            }
-        }
-        new_groups
-    }
-
     /// Convert a group of fragment span
     /// that didn't make it into an endorsed single shape fragment
     /// We try it again for endorsing to circle
@@ -278,6 +255,18 @@ impl Span {
     }
 }
 
+impl Merge for Span {
+    fn merge(&self, other: &Self) -> Option<Self> {
+        if self.can_merge(other) {
+            let mut cells = self.0.clone();
+            cells.extend(&other.0);
+            Some(Span(cells))
+        } else {
+            None
+        }
+    }
+}
+
 impl Bounds {
     pub fn new(cell1: Cell, cell2: Cell) -> Self {
         let (top_left, bottom_right) = Cell::rearrange_bound(cell1, cell2);
diff --git a/packages/svgbob/src/buffer/fragment_buffer.rs b/packages/svgbob/src/buffer/fragment_buffer.rs
index 53a81b0..e9bdddb 100644
--- a/packages/svgbob/src/buffer/fragment_buffer.rs
+++ b/packages/svgbob/src/buffer/fragment_buffer.rs
@@ -1,5 +1,6 @@
 use crate::buffer::Span;
 use crate::Cell;
+use crate::Merge;
 use crate::Settings;
 pub use direction::Direction;
 pub use fragment::Fragment;
@@ -137,12 +138,12 @@ impl FragmentBuffer {
         &self,
         settings: &Settings,
     ) -> Vec<FragmentSpan> {
-        let fragment_spans = self.into_fragment_spans(settings);
-        FragmentSpan::merge_recursive(fragment_spans, settings)
+        let fragment_spans = self.into_fragment_spans();
+        FragmentSpan::merge_recursive(fragment_spans)
     }
 
     /// create a merged of fragments while preserving their cells
-    fn into_fragment_spans(&self, settings: &Settings) -> Vec<FragmentSpan> {
+    fn into_fragment_spans(&self) -> Vec<FragmentSpan> {
         let mut fragment_spans: Vec<FragmentSpan> = vec![];
         for (cell, (ch, fragments)) in self.iter() {
             for frag in fragments.iter() {
@@ -151,9 +152,7 @@ impl FragmentBuffer {
                 let abs_frag = FragmentSpan::new(span, abs_frag);
                 let had_merged =
                     fragment_spans.iter_mut().rev().any(|frag_span| {
-                        if let Some(new_merge) =
-                            frag_span.merge(&abs_frag, settings)
-                        {
+                        if let Some(new_merge) = frag_span.merge(&abs_frag) {
                             *frag_span = new_merge;
                             true
                         } else {
diff --git a/packages/svgbob/src/buffer/fragment_buffer/fragment.rs b/packages/svgbob/src/buffer/fragment_buffer/fragment.rs
index 2080f5f..1bf235d 100644
--- a/packages/svgbob/src/buffer/fragment_buffer/fragment.rs
+++ b/packages/svgbob/src/buffer/fragment_buffer/fragment.rs
@@ -1,3 +1,4 @@
+use crate::Merge;
 use crate::{buffer::Cell, map::unicode_map::FRAGMENTS_UNICODE, Point};
 pub use crate::{Property, Settings, Signal};
 pub use arc::Arc;
@@ -81,7 +82,7 @@ impl Fragment {
     /// check to see if this fragment is a line and that line
     /// can completely overlap line a b
     /// TODO: only checking for solid, also expose API for broken
-    pub(in crate) fn line_overlap(&self, a: Point, b: Point) -> bool {
+    pub(crate) fn line_overlap(&self, a: Point, b: Point) -> bool {
         match self {
             Fragment::Line(line) => line.overlaps(a, b),
             _ => false,
@@ -89,7 +90,7 @@ impl Fragment {
     }
 
     /// check if any of the fragment end point is touching p
-    pub(in crate) fn has_endpoint(&self, p: Point) -> bool {
+    pub(crate) fn has_endpoint(&self, p: Point) -> bool {
         match self {
             Fragment::Line(line) => line.has_endpoint(p),
             Fragment::Arc(arc) => arc.has_endpoint(p),
@@ -99,96 +100,13 @@ impl Fragment {
 
     /// check to see if this fragment is an arc
     /// overlaps from point a to b
-    pub(in crate) fn arcs_to(&self, a: Point, b: Point) -> bool {
+    pub(crate) fn arcs_to(&self, a: Point, b: Point) -> bool {
         match self {
             Fragment::Arc(arc) => arc.arcs_to(a, b),
             _ => false,
         }
     }
 
-    /// merge this fragment to the other fragment if it is possible
-    /// returns None if the fragment can not be merge
-    pub fn merge(&self, other: &Self, settings: &Settings) -> Option<Self> {
-        match (self, other) {
-            // line and line
-            (Fragment::Line(line), Fragment::Line(other_line)) => {
-                if let Some(merged_line) = line.merge(other_line) {
-                    Some(Fragment::Line(merged_line))
-                } else {
-                    None
-                }
-            }
-
-            // line and polygon
-            (Fragment::Line(line), Fragment::Polygon(polygon)) => {
-                if settings.merge_line_with_shapes {
-                    line.merge_line_polygon(polygon)
-                } else {
-                    None
-                }
-            }
-
-            // polygon and line
-            (Fragment::Polygon(polygon), Fragment::Line(line)) => {
-                if settings.merge_line_with_shapes {
-                    line.merge_line_polygon(polygon)
-                } else {
-                    None
-                }
-            }
-
-            // line and marker_line
-            (Fragment::Line(line), Fragment::MarkerLine(mline)) => {
-                if settings.merge_line_with_shapes {
-                    line.merge_marker_line(mline)
-                } else {
-                    None
-                }
-            }
-            // marker_line and line
-            (Fragment::MarkerLine(mline), Fragment::Line(line)) => {
-                if settings.merge_line_with_shapes {
-                    line.merge_marker_line(mline)
-                } else {
-                    None
-                }
-            }
-            (Fragment::MarkerLine(mline), Fragment::Polygon(polygon)) => {
-                if settings.merge_line_with_shapes {
-                    mline.merge_polygon(polygon)
-                } else {
-                    None
-                }
-            }
-            // line and circle
-            (Fragment::Line(line), Fragment::Circle(circle)) => {
-                if settings.merge_line_with_shapes {
-                    line.merge_circle(circle)
-                } else {
-                    None
-                }
-            }
-
-            // circle and line
-            (Fragment::Circle(circle), Fragment::Line(line)) => {
-                if settings.merge_line_with_shapes {
-                    line.merge_circle(circle)
-                } else {
-                    None
-                }
-            }
-            // cell_text and cell_text
-            (Fragment::CellText(ctext), Fragment::CellText(other_ctext)) => {
-                if let Some(merged_ctext) = ctext.merge(other_ctext) {
-                    Some(Fragment::CellText(merged_ctext))
-                } else {
-                    None
-                }
-            }
-            _ => None,
-        }
-    }
-
     /// check whether the other fragment is can be fit in this fragment
     pub(crate) fn can_fit(&self, other: &Self) -> bool {
         let (tl, br) = self.bounds();
@@ -199,45 +117,8 @@ impl Fragment {
             && br.y >= other_br.y
     }
 
-    /// merge fragments recursively until it hasn't changed the number of fragments
-    pub(crate) fn merge_recursive(
-        fragments: Vec<Self>,
-        settings: &Settings,
-    ) -> Vec<Self> {
-        let original_len = fragments.len();
-        let merged = Self::second_pass_merge(fragments, settings);
-        // if has merged continue merging untila nothing can be merged
-        if merged.len() < original_len {
-            Self::merge_recursive(merged, settings)
-        } else {
-            merged
-        }
-    }
-
-    /// second pass merge is operating on fragments comparing to other spans
-    fn second_pass_merge(
-        fragments: Vec<Self>,
-        settings: &Settings,
-    ) -> Vec<Self> {
-        let mut new_groups: Vec<Self> = vec![];
-        for fragment in fragments.into_iter() {
-            let is_merged = new_groups.iter_mut().rev().any(|new_group| {
-                if let Some(new_merged) = new_group.merge(&fragment, settings) {
-                    *new_group = new_merged;
-                    true
-                } else {
-                    false
-                }
-            });
-            if !is_merged {
-                new_groups.push(fragment);
-            }
-        }
-        new_groups
-    }
-
     /// are lines axis align and parallel
-    pub(in crate) fn is_aabb_parallel(&self, other: &Self) -> bool {
+    pub(crate) fn is_aabb_parallel(&self, other: &Self) -> bool {
         match (self, other) {
             (Fragment::Line(line), Fragment::Line(other)) => {
                 line.is_aabb_parallel(other)
@@ -247,7 +128,7 @@ impl Fragment {
     }
 
     #[allow(unused)]
-    pub(in crate) fn is_aabb_perpendicular(&self, other: &Self) -> bool {
+    pub(crate) fn is_aabb_perpendicular(&self, other: &Self) -> bool {
         match (self, other) {
             (Fragment::Line(line), Fragment::Line(other)) => {
                 line.is_aabb_perpendicular(other)
@@ -258,7 +139,7 @@ impl Fragment {
 
     /// check if this fragment is touching the other fragment
     /// therefore can be in a group together
-    pub(in crate) fn is_contacting(&self, other: &Self) -> bool {
+    pub(crate) fn is_contacting(&self, other: &Self) -> bool {
         match self {
             Fragment::Line(line) => match other {
                 Fragment::Line(other) => line.is_touching(other),
@@ -533,6 +414,63 @@ impl Fragment {
     }
 }
 
+impl Merge for Fragment {
+    /// merge this fragment to the other fragment if it is possible
+    /// returns None if the fragment can not be merge
+    fn merge(&self, other: &Self) -> Option<Self> {
+        match (self, other) {
+            // line and line
+            (Fragment::Line(line), Fragment::Line(other_line)) => {
+                if let Some(merged_line) = line.merge(other_line) {
+                    Some(Fragment::Line(merged_line))
+                } else {
+                    None
+                }
+            }
+
+            // line and polygon
+            (Fragment::Line(line), Fragment::Polygon(polygon)) => {
+                line.merge_line_polygon(polygon)
+            }
+
+            // polygon and line
+            (Fragment::Polygon(polygon), Fragment::Line(line)) => {
+                line.merge_line_polygon(polygon)
+            }
+
+            // line and marker_line
+            (Fragment::Line(line), Fragment::MarkerLine(mline)) => {
+                line.merge_marker_line(mline)
+            }
+            // marker_line and line
+            (Fragment::MarkerLine(mline), Fragment::Line(line)) => {
+                line.merge_marker_line(mline)
+            }
+            (Fragment::MarkerLine(mline), Fragment::Polygon(polygon)) => {
+                mline.merge_polygon(polygon)
+            }
+            // line and circle
+            (Fragment::Line(line), Fragment::Circle(circle)) => {
+                line.merge_circle(circle)
+            }
+
+            // circle and line
+            (Fragment::Circle(circle), Fragment::Line(line)) => {
+                line.merge_circle(circle)
+            }
+            // cell_text and cell_text
+            (Fragment::CellText(ctext), Fragment::CellText(other_ctext)) => {
+                if let Some(merged_ctext) = ctext.merge(other_ctext) {
+                    Some(Fragment::CellText(merged_ctext))
+                } else {
+                    None
+                }
+            }
+            _ => None,
+        }
+    }
+}
+
 impl Bounds for Fragment {
     fn bounds(&self) -> (Point, Point) {
         match self {
@@ -776,8 +714,7 @@ mod tests {
         }
         let mut expected = vec![line(k, o), line(c, w)];
         expected.sort();
-        let mut merged_fragments1 =
-            Fragment::merge_recursive(fragments1, &Settings::default());
+        let mut merged_fragments1 = Fragment::merge_recursive(fragments1);
         merged_fragments1.sort();
         assert_eq!(merged_fragments1.len(), 2);
         println!("after merged:");
@@ -795,45 +732,11 @@ mod tests {
         let n = CellGrid::n();
         let o = CellGrid::o();
         let j = CellGrid::j();
-        let mut settings = Settings::default();
-        settings.merge_line_with_shapes = true;
 
-        assert!(line(k, m).merge(&line(m, o), &settings).is_some()); // collinear and connected
-        assert!(!line(k, l).merge(&line(n, o), &settings).is_some()); //collinear but not connected
-        assert!(!line(k, o).merge(&line(o, j), &settings).is_some()); // connected but not collinear
-    }
-
-    /*
-    #[test]
-    fn merge_unicode_triangle_and_line() {
-        let arrow = '▶';
-        let entry = crate::map::UNICODE_FRAGMENTS
-            .get(&arrow)
-            .expect("must have a fragement");
-        let a = CellGrid::a();
-        let y = CellGrid::y();
-
-        let polygon = entry[0].absolute_position(Cell::new(0, 0));
-        let diagonal: Fragment = Line::new_noswap(y, a, false).into();
-        let diagonal = diagonal.absolute_position(Cell::new(1, 1));
-
-        println!("polygon: {:#?}", polygon);
-        println!("diagonal: {:#?}", diagonal);
-        let mut settings = Settings::default();
-
-        settings.merge_line_with_shapes = true;
-        let merged = polygon.merge(&diagonal, &settings);
-
-        let expected = marker_line(
-            Point::new(2.0, 4.0),
-            Point::new(0.0, 0.0),
-            false,
-            None,
-            Some(Marker::Arrow),
-        );
-        assert_eq!(Some(expected), merged);
+        assert!(line(k, m).merge(&line(m, o),).is_some()); // collinear and connected
+        assert!(!line(k, l).merge(&line(n, o),).is_some()); //collinear but not connected
+        assert!(!line(k, o).merge(&line(o, j),).is_some()); // connected but not collinear
     }
-    */
 
     #[test]
     fn merge_line_and_circle() {
@@ -850,9 +753,7 @@ mod tests {
         println!("circle: {:#?}", circle);
         println!("diagonal: {:#?}", diagonal);
 
-        let mut settings = Settings::default();
-        settings.merge_line_with_shapes = true;
-        let merged = circle.merge(&diagonal, &settings);
+        let merged = circle.merge(&diagonal);
 
         let expected = marker_line(
             Point::new(2.0, 4.0),
diff --git a/packages/svgbob/src/buffer/fragment_buffer/fragment/text.rs b/packages/svgbob/src/buffer/fragment_buffer/fragment/text.rs
index e0256e7..8d1e0a4 100644
--- a/packages/svgbob/src/buffer/fragment_buffer/fragment/text.rs
+++ b/packages/svgbob/src/buffer/fragment_buffer/fragment/text.rs
@@ -225,6 +225,7 @@ impl PartialEq for Text {
 
 #[cfg(test)]
 mod tests {
+    use super::*;
     use crate::{
         buffer::{Cell, CellBuffer, Contacts, Span},
         fragment::CellText,
@@ -271,9 +272,10 @@ mod tests {
         assert_eq!(15, groups.len());
         assert_eq!(
             groups[0],
-            Contacts::new(
-                CellText::new(Cell::new(0, 0), "This".to_string()).into()
-            )
+            Contacts::new(Into::<FragmentSpan>::into(CellText::new(
+                Cell::new(0, 0),
+                "This".to_string()
+            )))
         );
         assert_eq!(
             groups[5],
diff --git a/packages/svgbob/src/buffer/fragment_buffer/fragment_span.rs b/packages/svgbob/src/buffer/fragment_buffer/fragment_span.rs
index 135e542..cbac181 100644
--- a/packages/svgbob/src/buffer/fragment_buffer/fragment_span.rs
+++ b/packages/svgbob/src/buffer/fragment_buffer/fragment_span.rs
@@ -1,6 +1,7 @@
 use crate::buffer::Span;
 use crate::Cell;
 use crate::Fragment;
+use crate::Merge;
 use crate::Settings;
 use std::fmt;
 
@@ -32,56 +33,6 @@ impl FragmentSpan {
         }
     }
 
-    pub fn merge(&self, other: &Self, settings: &Settings) -> Option<Self> {
-        if let Some(new_merge) = self.fragment.merge(&other.fragment, settings)
-        {
-            let mut new_span = self.span.clone();
-            new_span.merge(&other.span);
-            Some(Self {
-                span: new_span,
-                fragment: new_merge,
-            })
-        } else {
-            None
-        }
-    }
-
-    pub(crate) fn merge_recursive(
-        fragment_spans: Vec<Self>,
-        settings: &Settings,
-    ) -> Vec<Self> {
-        let original_len = fragment_spans.len();
-        let merged = Self::second_pass_merge(fragment_spans, settings);
-        if merged.len() < original_len {
-            Self::merge_recursive(merged, settings)
-        } else {
-            merged
-        }
-    }
-
-    fn second_pass_merge(
-        fragment_spans: Vec<Self>,
-        settings: &Settings,
-    ) -> Vec<Self> {
-        let mut new_groups: Vec<Self> = vec![];
-        for fragment_span in fragment_spans.into_iter() {
-            let is_merged = new_groups.iter_mut().rev().any(|new_group| {
-                if let Some(new_merged) =
-                    new_group.merge(&fragment_span, settings)
-                {
-                    *new_group = new_merged;
-                    true
-                } else {
-                    false
-                }
-            });
-            if !is_merged {
-                new_groups.push(fragment_span);
-            }
-        }
-        new_groups
-    }
-
     pub(crate) fn is_contacting(&self, other: &Self) -> bool {
         self.fragment.is_contacting(&other.fragment)
     }
@@ -101,3 +52,18 @@ impl FragmentSpan {
         self.span.hit_cell(needle)
     }
 }
+
+impl Merge for FragmentSpan {
+    fn merge(&self, other: &Self) -> Option<Self> {
+        if let Some(new_merge) = self.fragment.merge(&other.fragment) {
+            let new_span =
+                self.span.merge(&other.span).expect("must merge the spans");
+            Some(Self {
+                span: new_span,
+                fragment: new_merge,
+            })
+        } else {
+            None
+        }
+    }
+}
diff --git a/packages/svgbob/src/buffer/fragment_buffer/fragment_tree.rs b/packages/svgbob/src/buffer/fragment_buffer/fragment_tree.rs
index af00295..8e88fb5 100644
--- a/packages/svgbob/src/buffer/fragment_buffer/fragment_tree.rs
+++ b/packages/svgbob/src/buffer/fragment_buffer/fragment_tree.rs
@@ -128,6 +128,7 @@ impl FragmentTree {
     }
 }
 
+//TODO: fix all of this tests
 /*
 #[cfg(test)]
 mod tests {
diff --git a/packages/svgbob/src/buffer/property_buffer.rs b/packages/svgbob/src/buffer/property_buffer.rs
index 5ac9539..67d72b8 100644
--- a/packages/svgbob/src/buffer/property_buffer.rs
+++ b/packages/svgbob/src/buffer/property_buffer.rs
@@ -1,7 +1,7 @@
 use crate::{
     fragment,
     map::{ASCII_PROPERTIES, UNICODE_FRAGMENTS},
-    Cell, Fragment, FragmentBuffer, Settings,
+    Cell, Fragment, FragmentBuffer, Merge, Settings,
 };
 pub use property::{Property, Signal};
 use std::collections::HashMap;
@@ -110,7 +110,7 @@ impl<'p> PropertyBuffer<'p> {
                     bottom_right,
                 );
                 let mut merged_behavioral_fragments =
-                    Fragment::merge_recursive(behavioral_fragments, settings);
+                    Fragment::merge_recursive(behavioral_fragments);
                 merged_behavioral_fragments.sort();
                 merged_behavioral_fragments.dedup();
                 //assert!(merged_behavioral_fragments.is_sorted());
@@ -174,7 +174,7 @@ impl<'p> PropertyBuffer<'p> {
                 //If no match make it a text fragment
                 if let Some(fragments) = UNICODE_FRAGMENTS.get(&property.ch) {
                     let merged_fragments =
-                        Fragment::merge_recursive(fragments.clone(), settings);
+                        Fragment::merge_recursive(fragments.clone());
                     fb.add_fragments_to_cell(
                         *cell,
                         property.ch,
diff --git a/packages/svgbob/src/lib.rs b/packages/svgbob/src/lib.rs
index ccf8ee1..6d4f329 100644
--- a/packages/svgbob/src/lib.rs
+++ b/packages/svgbob/src/lib.rs
@@ -13,6 +13,7 @@ pub use buffer::{
     fragment, fragment::Fragment, Cell, CellBuffer, Direction, FragmentBuffer,
     FragmentSpan, Property, Signal,
 };
+pub use merge::Merge;
 pub use nalgebra;
 pub use point::Point;
 /// reexport sauron