From: Simon Tatham <anakin@pobox.com>
Date: Tue, 9 Jan 2024 06:27:19 +0000 (+0000)
Subject: Restructure ColouredString via a common trait.
X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ian/git?a=commitdiff_plain;h=7cc91bf4f9d31805cbb629584dd04ad8d13f32ed;p=mastodonochrome.git

Restructure ColouredString via a common trait.

Now ColouredString and ColouredStringSlice both implement the trait
ColouredStringCommon, and as many methods as possible are provided by
that trait, depending only on the unimplemented trait methods to get
the text and colours and &str.

This reduces the amount of code, and also makes points of use more
flexible, because I can implement the common trait not just for both
the actual types, but also references to them.

The only disappointment is that I wasn't able to also fold the four
identical impls of std::ops::Add into one via a single
doubly-quantified impl. But instead I got to deploy my first
macro_rules!, so that's something.

In the next commits I'll clean up now-unnecessary boilerplate at call
sites.
---

diff --git a/src/coloured_string.rs b/src/coloured_string.rs
index 885649f..b36c2fc 100644
--- a/src/coloured_string.rs
+++ b/src/coloured_string.rs
@@ -1,160 +1,193 @@
 use unicode_width::UnicodeWidthStr;
 use unicode_width::UnicodeWidthChar;
 
-#[derive(Debug, Clone, PartialEq, Eq)]
-pub struct ColouredString {
-    text: String,
-    colour: String,
-}
+pub trait ColouredStringCommon {
+    fn text(&self) -> & str;
+    fn colours(&self) -> & str;
 
-#[derive(Debug, Clone, PartialEq, Eq)]
-pub struct ColouredStringSlice<'a> {
-    text: &'a str,
-    colour: &'a str,
-}
+    fn is_empty(&self) -> bool { self.text().is_empty() }
+    fn nchars(&self) -> usize { self.text().chars().count() }
+    fn width(&self) -> usize { UnicodeWidthStr::width(self.text()) }
 
-impl ColouredString {
-    pub fn plain(text: &str) -> Self {
-        ColouredString {
-            text: text.to_owned(),
-            colour: " ".repeat(text.chars().count()),
+    fn slice<'a>(&'a self) -> ColouredStringSlice<'a> {
+        ColouredStringSlice {
+            text: self.text(),
+            colours: self.colours(),
         }
     }
-    pub fn uniform(text: &str, colour: char) -> Self {
-        ColouredString {
-            text: text.to_owned(),
-            colour: colour.to_string().repeat(text.chars().count()),
+
+    fn truncate(&self, width: usize) -> ColouredStringSlice {
+        self.split(width).next().unwrap()
+    }
+
+    fn chars<'a>(&'a self) -> ColouredStringCharIterator<'a> {
+        ColouredStringCharIterator {
+            cs: self.slice(),
+            textpos: 0,
+            colourpos: 0,
         }
     }
-    pub fn general(text: &str, colour: &str) -> Self {
-        assert_eq!(text.chars().count(), colour.chars().count(),
-                   "Mismatched lengths in ColouredString::general");
-        ColouredString {
-            text: text.to_owned(),
-            colour: colour.to_owned(),
+    fn frags<'a>(&'a self) -> ColouredStringFragIterator<'a> {
+        ColouredStringFragIterator {
+            cs: self.slice(),
+            textpos: 0,
+            colourpos: 0,
         }
     }
-
-    pub fn repeat(self, count: usize) -> Self {
-        ColouredString {
-            text: self.text.repeat(count),
-            colour: self.colour.repeat(count),
+    fn split<'a>(&'a self, width: usize) -> ColouredStringSplitIterator<'a> {
+        ColouredStringSplitIterator {
+            cs: self.slice(),
+            width,
+            textpos: 0,
+            colourpos: 0,
+            delivered_empty: false,
         }
     }
 
-    pub fn is_empty(&self) -> bool { self.text.is_empty() }
-    pub fn nchars(&self) -> usize { self.text.chars().count() }
-    pub fn width(&self) -> usize { UnicodeWidthStr::width(&self.text as &str) }
-
-    pub fn text(&self) -> &str { &self.text }
-    pub fn colours(&self) -> &str { &self.colour }
-    pub fn recolour(&self, colour: char) -> Self {
-        Self::uniform(&self.text, colour)
+    fn repeat(&self, count: usize) -> ColouredString {
+        ColouredString::general(&self.text().repeat(count),
+                                &self.colours().repeat(count))
     }
 
-    pub fn slice(&self) -> ColouredStringSlice {
-        ColouredStringSlice {
-            text: &self.text,
-            colour: &self.colour,
-        }
+    fn recolour(&self, colour: char) -> ColouredString {
+        ColouredString::uniform(self.text(), colour)
     }
+}
 
-    pub fn truncate(&self, width: usize) -> ColouredStringSlice {
-        self.split(width).next().unwrap()
-    }
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct ColouredString {
+    text: String,
+    colours: String,
+}
 
-    pub fn push_str(&mut self, more: &ColouredStringSlice<'_>) {
-        self.text.push_str(more.text);
-        self.colour.push_str(more.colour);
-    }
+impl ColouredStringCommon for ColouredString {
+    fn text(&self) -> &str { &self.text }
+    fn colours(&self) -> &str { &self.colours }
 }
 
-impl<'a> ColouredStringSlice<'a> {
-    pub fn general(text: &'a str, colour: &'a str) -> Self {
-        assert_eq!(text.chars().count(), colour.chars().count(),
-                   "Mismatched lengths in ColouredStringSlice::general");
-        ColouredStringSlice {
-            text,
-            colour,
-        }
-    }
+impl<'a> ColouredStringCommon for &'a ColouredString {
+    fn text(&self) -> &str { &self.text }
+    fn colours(&self) -> &str { &self.colours }
+}
 
-    pub fn to_owned(&self) -> ColouredString {
+// I'd have liked here to write
+// impl<T: ColouredStringCommon> From<T> for ColouredString { ... }
+// on the basis that this code can sensibly make an owned ColouredString
+// out of anything that provides .text() and .colours().
+//
+// But the problem is that that conflicts with the standard reflexive
+// definition in which ColouredString itself _already_ implements
+// From<ColouredString> via the identity function. Of course that _is_
+// a much more sensible definition in that one case! And I don't know
+// of any language feature that lets me offer the definition below for
+// only everything _else_ satisfying the type bound.
+impl<'a> From<ColouredStringSlice<'a>> for ColouredString {
+    fn from(value: ColouredStringSlice<'a>) -> Self {
         ColouredString {
-            text: self.text.to_owned(),
-            colour: self.colour.to_owned(),
+            text: value.text().to_owned(),
+            colours: value.colours().to_owned(),
         }
     }
-
-    pub fn nchars(&self) -> usize { self.text.chars().count() }
-    pub fn width(&self) -> usize { UnicodeWidthStr::width(self.text as &str) }
-    pub fn text(&self) -> &'a str { self.text }
-
-    pub fn truncate(&'a self, width: usize) -> ColouredStringSlice<'a> {
-        self.split(width).next().unwrap()
-    }
 }
 
-impl std::ops::Add<ColouredString> for ColouredString {
-    type Output = Self;
-    fn add(self, rhs: Self) -> Self {
+impl ColouredString {
+    pub fn plain(text: &str) -> Self {
         ColouredString {
-            text: self.text + &rhs.text,
-            colour: self.colour + &rhs.colour,
+            text: text.to_owned(),
+            colours: " ".repeat(text.chars().count()),
         }
     }
-}
 
-impl std::ops::Add<&ColouredString> for ColouredString {
-    type Output = Self;
-    fn add(self, rhs: &ColouredString) -> Self {
+    pub fn uniform(text: &str, colour: char) -> Self {
         ColouredString {
-            text: self.text + &rhs.text,
-            colour: self.colour + &rhs.colour,
+            text: text.to_owned(),
+            colours: colour.to_string().repeat(text.chars().count()),
         }
     }
-}
 
-impl std::ops::Add<&str> for ColouredString {
-    type Output = Self;
-    fn add(self, rhs: &str) -> Self {
+    pub fn general(text: &str, colours: &str) -> Self {
+        assert_eq!(text.chars().count(), colours.chars().count(),
+                   "Mismatched lengths in ColouredString::general");
         ColouredString {
-            text: self.text + rhs,
-            colour: self.colour + &(" ".repeat(rhs.chars().count())),
+            text: text.to_owned(),
+            colours: colours.to_owned(),
         }
     }
-}
 
-impl std::ops::Add<ColouredString> for &ColouredString {
-    type Output = ColouredString;
-    fn add(self, rhs: ColouredString) -> ColouredString {
+    fn concat<T: ColouredStringCommon, U: ColouredStringCommon>
+        (lhs: T, rhs: U) -> ColouredString
+    {
         ColouredString {
-            text: self.text.to_owned() + &rhs.text,
-            colour: self.colour.to_owned() + &rhs.colour,
+            text: lhs.text().to_owned() + rhs.text(),
+            colours: lhs.colours().to_owned() + rhs.colours(),
         }
     }
+
+    pub fn push_str(&mut self, more: impl ColouredStringCommon) {
+        self.text.push_str(more.text());
+        self.colours.push_str(more.colours());
+    }
 }
 
-impl std::ops::Add<ColouredStringSlice<'_>> for ColouredString {
-    type Output = ColouredString;
-    fn add(self, rhs: ColouredStringSlice<'_>) -> ColouredString {
-        ColouredString {
-            text: self.text + rhs.text,
-            colour: self.colour + rhs.colour,
-        }
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct ColouredStringSlice<'a> {
+    text: &'a str,
+    colours: &'a str,
+}
+
+impl<'a> ColouredStringCommon for ColouredStringSlice<'a> {
+    fn text(&self) -> &str { self.text }
+    fn colours(&self) -> &str { self.colours }
+}
+
+impl<'a> ColouredStringCommon for &ColouredStringSlice<'a> {
+    fn text(&self) -> &str { self.text }
+    fn colours(&self) -> &str { self.colours }
+}
+
+impl<'a> ColouredStringSlice<'a> {
+    pub fn general(text: &'a str, colours: &'a str) -> Self {
+        assert_eq!(text.chars().count(), colours.chars().count(),
+                   "Mismatched lengths in ColouredStringSlice::general");
+
+        ColouredStringSlice { text, colours }
     }
+
+    pub fn to_owned(&self) -> ColouredString { self.clone().into() }
 }
 
-impl std::ops::Add<ColouredString> for &str {
-    type Output = ColouredString;
-    fn add(self, rhs: ColouredString) -> ColouredString {
-        ColouredString {
-            text: self.to_owned() + &rhs.text,
-            colour: (" ".repeat(self.chars().count())) + &rhs.colour,
+// We want to be able to use the + operator to concatenate any two
+// types that impl ColouredStringCommon. The logic of _how_ to do this
+// is already written, in the ColouredString::concat() constructor
+// above. Now we just need to make the + operator invoke it.
+//
+// I'd like to do this by writing a single item along the lines of
+// impl<T: ColouredStringCommon, U: ColouredStringCommon> Add<U> for T { ... }
+// but that's not allowed, because if a user of this module implemented
+// ColouredStringCommon for one of _their_ types, then this would end up
+// trying to implement a trait that's not mine (Add) on a type that's not
+// mine (the user's type).
+//
+// Making ColouredStringCommon into a sealed trait doesn't cause the
+// compiler to recognise that risk as nonexistent, which is a shame.
+// So instead let's do some macro business, to avoid copy-pasting four
+// times.
+macro_rules! impl_Add {
+    ($type:ty, $($life:lifetime)?) => {
+        impl<$($life,)? U: ColouredStringCommon> std::ops::Add<U> for $type {
+            type Output = ColouredString;
+            fn add(self, rhs: U) -> ColouredString {
+                ColouredString::concat(self, rhs)
+            }
         }
-    }
+    };
 }
 
+impl_Add!(ColouredString,);
+impl_Add!(&ColouredString,);
+impl_Add!(ColouredStringSlice<'a>, 'a);
+impl_Add!(&ColouredStringSlice<'a>, 'a);
+
 pub struct ColouredStringCharIterator<'a> {
     cs: ColouredStringSlice<'a>,
     textpos: usize,
@@ -175,64 +208,12 @@ pub struct ColouredStringSplitIterator<'a> {
     delivered_empty: bool,
 }
 
-impl<'a> ColouredStringSlice<'a> {
-    pub fn chars(&self) -> ColouredStringCharIterator<'a> {
-        ColouredStringCharIterator {
-            cs: self.clone(),
-            textpos: 0,
-            colourpos: 0,
-        }
-    }
-    pub fn frags(&self) -> ColouredStringFragIterator<'a> {
-        ColouredStringFragIterator {
-            cs: self.clone(),
-            textpos: 0,
-            colourpos: 0,
-        }
-    }
-    pub fn split(&'a self, width: usize) -> ColouredStringSplitIterator<'a> {
-        ColouredStringSplitIterator {
-            cs: self.clone(),
-            width,
-            textpos: 0,
-            colourpos: 0,
-            delivered_empty: false,
-        }
-    }
-}
-
-impl<'a> ColouredString {
-    pub fn chars(&'a self) -> ColouredStringCharIterator<'a> {
-        ColouredStringCharIterator {
-            cs: self.slice(),
-            textpos: 0,
-            colourpos: 0,
-        }
-    }
-    pub fn frags(&'a self) -> ColouredStringFragIterator<'a> {
-        ColouredStringFragIterator {
-            cs: self.slice(),
-            textpos: 0,
-            colourpos: 0,
-        }
-    }
-    pub fn split(&'a self, width: usize) -> ColouredStringSplitIterator<'a> {
-        ColouredStringSplitIterator {
-            cs: self.slice(),
-            width,
-            textpos: 0,
-            colourpos: 0,
-            delivered_empty: false,
-        }
-    }
-}
-
 impl<'a> Iterator for ColouredStringCharIterator<'a> {
     type Item = ColouredStringSlice<'a>;
     fn next(&mut self) -> Option<Self::Item> {
         let textslice = &self.cs.text[self.textpos..];
         let mut textit = textslice.char_indices();
-        let colourslice = &self.cs.colour[self.colourpos..];
+        let colourslice = &self.cs.colours[self.colourpos..];
         let mut colourit = colourslice.char_indices();
         if let (Some(_), Some(_)) = (textit.next(), colourit.next()) {
             let (textend, colourend) = match (textit.next(), colourit.next()) {
@@ -243,7 +224,7 @@ impl<'a> Iterator for ColouredStringCharIterator<'a> {
             self.colourpos += colourend;
             Some(ColouredStringSlice {
                     text: &textslice[..textend],
-                    colour: &colourslice[..colourend],
+                    colours: &colourslice[..colourend],
                 })
         } else {
             None
@@ -256,7 +237,7 @@ impl<'a> Iterator for ColouredStringFragIterator<'a> {
     fn next(&mut self) -> Option<Self::Item> {
         let textslice = &self.cs.text[self.textpos..];
         let mut textit = textslice.char_indices();
-        let colourslice = &self.cs.colour[self.colourpos..];
+        let colourslice = &self.cs.colours[self.colourpos..];
         let mut colourit = colourslice.char_indices();
         if let Some((_, colour)) = colourit.next() {
             // Expect this not to run out, because colour didn't
@@ -291,7 +272,7 @@ impl<'a> Iterator for ColouredStringSplitIterator<'a> {
     fn next(&mut self) -> Option<Self::Item> {
         let textslice = &self.cs.text[self.textpos..];
         let mut textit = textslice.char_indices();
-        let colourslice = &self.cs.colour[self.colourpos..];
+        let colourslice = &self.cs.colours[self.colourpos..];
         let mut colourit = colourslice.char_indices();
         match (textit.next(), colourit.next()) {
             (None, None) => {
@@ -329,7 +310,7 @@ impl<'a> Iterator for ColouredStringSplitIterator<'a> {
                 self.colourpos += colourend;
                 Some(ColouredStringSlice {
                         text: &textslice[..textend],
-                        colour: &colourslice[..colourend],
+                        colours: &colourslice[..colourend],
                     })
             }
             _ => panic!("length mismatch in CSSI"),
@@ -356,10 +337,6 @@ fn test_concat() {
     assert_eq!(ColouredString::general("xyz", "pqr") +
                ColouredString::general("abcde", "ijklm"),
                ColouredString::general("xyzabcde", "pqrijklm"));
-    assert_eq!(ColouredString::general("xyz", "pqr") + "abcde",
-               ColouredString::general("xyzabcde", "pqr     "));
-    assert_eq!("abcde" + ColouredString::general("xyz", "pqr"),
-               ColouredString::general("abcdexyz", "     pqr"));
 }
 
 #[test]
diff --git a/src/editor.rs b/src/editor.rs
index f452a77..ebb0e02 100644
--- a/src/editor.rs
+++ b/src/editor.rs
@@ -3,7 +3,7 @@ use unicode_width::UnicodeWidthChar;
 
 use super::activity_stack::{NonUtilityActivity, UtilityActivity};
 use super::client::{Client, ClientError};
-use super::coloured_string::ColouredString;
+use super::coloured_string::*;
 use super::file::SearchDirection;
 use super::text::*;
 use super::tui::{
diff --git a/src/file.rs b/src/file.rs
index ffc1c2b..d33733e 100644
--- a/src/file.rs
+++ b/src/file.rs
@@ -7,7 +7,7 @@ use super::activity_stack::{
     NonUtilityActivity, UtilityActivity, OverlayActivity,
 };
 use super::client::{Client, ClientError, FeedId, FeedExtend, Boosts, Replies};
-use super::coloured_string::ColouredString;
+use super::coloured_string::*;
 use super::text::*;
 use super::tui::{
     ActivityState, CursorPosition, LogicalAction,
diff --git a/src/html.rs b/src/html.rs
index 9e4debe..83faca1 100644
--- a/src/html.rs
+++ b/src/html.rs
@@ -5,7 +5,7 @@ use html2text::render::text_renderer::{
 };
 use std::cell::RefCell;
 
-use super::coloured_string::ColouredString;
+use super::coloured_string::*;
 
 #[derive(Clone, Debug, Default)]
 struct OurDecorator<'a> {
diff --git a/src/text.rs b/src/text.rs
index 2a5fa64..f95a246 100644
--- a/src/text.rs
+++ b/src/text.rs
@@ -9,7 +9,7 @@ use super::html;
 use super::client::{Client, ClientError};
 use super::types::*;
 use super::tui::OurKey;
-use super::coloured_string::{ColouredString, ColouredStringSlice};
+use super::coloured_string::*;
 
 #[derive(Debug, PartialEq, Eq, Clone, Copy)]
 pub enum HighlightType { User, Status, WholeStatus, PollOption }
diff --git a/src/tui.rs b/src/tui.rs
index b34aa73..60c8e20 100644
--- a/src/tui.rs
+++ b/src/tui.rs
@@ -21,7 +21,7 @@ use super::activity_stack::*;
 use super::client::{
     Client, ClientError, FeedId, FeedExtend, StreamId, StreamUpdate,
 };
-use super::coloured_string::{ColouredString, ColouredStringSlice};
+use super::coloured_string::*;
 use super::config::ConfigLocation;
 use super::menu::*;
 use super::file::*;