#65 clean up functionality for summing song plays across albums

I think some days/weeks ago maybe in a Primeagen video I heard
about having a boolean switch like this being bad.
I can't remember it very well, if I do I'll post it in the issue/comment
But I agree, and I didn't like this solution. I think this is much
better.
I also added summing song plays across albums for plotting top songs.

endsong_ui/src/main.rs

@@ -37,7 +37,7 @@ fn main() {
         "macos" => "/Users/filip/Other/Endsong/",
         _ => "/mnt/c/temp/Endsong/",
     };
-    let last: u8 = 0;
+    let last: u8 = 9;
     let paths: Vec<String> = (0..=last)
         .map(|i| format!("{root}endsong_{i}.json"))
         .collect();
@@ -57,9 +57,9 @@ fn main() {
 /// tests various [`print`][crate::print] and [`endsong::gather`] functions
 #[allow(dead_code)]
 fn test(entries: &SongEntries) {
-    print::top(entries, Aspect::Songs, 10, false);
-    print::top(entries, Aspect::Albums, 10, false);
-    print::top(entries, Aspect::Artists, 10, false);
+    print::top(entries, Aspect::Songs(false), 10);
+    print::top(entries, Aspect::Albums, 10);
+    print::top(entries, Aspect::Artists, 10);
 
     let powerwolf = Artist::new("Powerwolf");
     print::top_from_artist(entries, Mode::Songs, &powerwolf, 10);

endsong_ui/src/print.rs

@@ -26,15 +26,18 @@ pub enum Aspect {
     Artists,
     /// to print top albums
     Albums,
-    /// to print top songs
-    Songs,
+    /// to print top songs, the [`bool`] inside is used
+    /// to decide whether or not to
+    /// ignore the album (and use only track/artist name) when gathering plays
+    /// (false by default when using `.parse()`)
+    Songs(bool),
 }
 impl Display for Aspect {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             Aspect::Artists => write!(f, "artists"),
             Aspect::Albums => write!(f, "albums"),
-            Aspect::Songs => write!(f, "songs"),
+            Aspect::Songs(_) => write!(f, "songs"),
         }
     }
 }
@@ -44,7 +47,7 @@ impl FromStr for Aspect {
         match s {
             "artist" | "artists" => Ok(Aspect::Artists),
             "album" | "albums" => Ok(Aspect::Albums),
-            "song" | "songs" => Ok(Aspect::Songs),
+            "song" | "songs" => Ok(Aspect::Songs(false)),
             _ => Err(AspectParseError),
         }
     }
@@ -109,16 +112,17 @@ impl DurationUtils for TimeDelta {
 ///    for top albums and [`Aspect::Artists`] for top artists
 /// * `num` - number of displayed top aspects.
 ///   Will automatically change to total number of that aspect if `num` is higher than that
-/// * `sum_songs_from_different_albums` - only matters if `asp` is [`Aspect::Songs`].
-///   If set to true, it will sum up the plays of
-///   one song across multiple albums it may be in.
-///   The album displayed in the parantheses will be the one it has the
-///   highest amount of listens from.
-pub fn top(entries: &[SongEntry], asp: Aspect, num: usize, sum_songs_from_different_albums: bool) {
+pub fn top(entries: &[SongEntry], asp: Aspect, num: usize) {
     match asp {
-        Aspect::Songs => {
+        Aspect::Songs(true) => {
+            println!("=== TOP {num} SONGS SUMMED ACROSS ALBUMS ===");
+            // The album displayed in the parantheses will be the
+            // one it has the highest amount of listens from.
+            top_helper(gather::songs_summed_across_albums(entries), num);
+        }
+        Aspect::Songs(false) => {
             println!("=== TOP {num} SONGS ===");
-            top_helper(gather::songs(entries, sum_songs_from_different_albums), num);
+            top_helper(gather::songs(entries), num);
         }
         Aspect::Albums => {
             println!("=== TOP {num} ALBUMS ===");

endsong_ui/src/trace.rs

@@ -54,6 +54,33 @@ pub fn absolute<Asp: Music>(entries: &SongEntries, aspect: &Asp) -> TraceType {
     TraceType::Absolute(trace)
 }
 
+/// Creates a trace of the absolute amount of plays of a song
+/// with its plays summed across all album it's in
+///
+/// Creates an empty trace if `song` is not in `entries`
+#[must_use]
+pub fn absolute_ignore_album(entries: &SongEntries, song: &Song) -> TraceType {
+    let mut times = Vec::<String>::with_capacity(entries.len());
+    let mut plays = Vec::<usize>::with_capacity(entries.len());
+
+    // since each date represents a single listen, we can just count up
+    let mut song_plays = 0;
+
+    for entry in entries
+        .iter()
+        .filter(|entry| song.album.artist.name == entry.artist && song.name == entry.track)
+    {
+        song_plays += 1;
+        times.push(format_date(&entry.timestamp));
+        plays.push(song_plays);
+    }
+
+    let title = format!("{song}");
+    let trace = Scatter::new(times, plays).name(title);
+
+    TraceType::Absolute(trace)
+}
+
 /// Module for relative traces
 ///
 /// Either to all plays, the artist or the album

endsong_ui/src/ui/mod.rs

@@ -303,9 +303,9 @@ fn match_input(
         "print album date" | "palbd" => match_print_album_date(entries, rl)?,
         "print song date" | "psond" => match_print_song_date(entries, rl)?,
         "print songs date" | "psonsd" => match_print_songs_date(entries, rl)?,
-        "print top artists" | "ptarts" => match_print_top(entries, rl, Aspect::Artists, false)?,
-        "print top albums" | "ptalbs" => match_print_top(entries, rl, Aspect::Albums, false)?,
-        "print top songs" | "ptsons" => match_print_top(entries, rl, Aspect::Songs, true)?,
+        "print top artists" | "ptarts" => match_print_top(entries, rl, Aspect::Artists)?,
+        "print top albums" | "ptalbs" => match_print_top(entries, rl, Aspect::Albums)?,
+        "print top songs" | "ptsons" => match_print_top(entries, rl, Aspect::Songs(false))?,
         "plot" | "g" => match_plot(entries, rl)?,
         "plot rel" | "gr" => match_plot_relative(entries, rl)?,
         "plot compare" | "gc" => match_plot_compare(entries, rl)?,
@@ -544,33 +544,23 @@ fn match_print_top(
     entries: &SongEntries,
     rl: &mut Editor<ShellHelper, FileHistory>,
     asp: Aspect,
-    ask_for_sum: bool,
 ) -> Result<(), UiError> {
-    rl.helper_mut().unwrap().reset();
+    let asp = match asp {
+        Aspect::Songs(_) => {
+            // prompt: ask if you want to sum songs from different albums
+            let ignore_album = read_whether_to_sum_songs(rl)?;
+            Aspect::Songs(ignore_album)
+        }
+        _ => asp,
+    };
+
     // prompt: top n
+    rl.helper_mut().unwrap().reset();
     println!("How many Top {asp}?");
     let usr_input_n = rl.readline(PROMPT_MAIN)?;
     let num: usize = usr_input_n.parse()?;
 
-    let mut sum_songs_from_different_albums = false;
-    if ask_for_sum {
-        // prompt: ask if you want to sum songs from different albums
-        rl.helper_mut()
-            .unwrap()
-            .complete_list(string_vec(&["yes", "y", "no", "n"]));
-        println!("Do you want to sum songs from different albums? (y/n)");
-        let usr_input_b = rl.readline(PROMPT_SECONDARY)?;
-        sum_songs_from_different_albums = match usr_input_b.as_str() {
-            "yes" | "y" => true,
-            "no" | "n" => false,
-            _ => {
-                println!("Invalid input. Assuming 'no'.");
-                false
-            }
-        }
-    }
-
-    print::top(entries, asp, num, sum_songs_from_different_albums);
+    print::top(entries, asp, num);
     Ok(())
 }
 
@@ -671,21 +661,41 @@ fn match_plot_top(
     let usr_input_asp = rl.readline(PROMPT_MAIN)?;
     let aspect: Aspect = usr_input_asp.parse()?;
 
+    let mut ignore_album = false;
+
+    let aspect = if let Aspect::Songs(_) = aspect {
+        // prompt: ask if you want to sum songs from different albums
+        ignore_album = read_whether_to_sum_songs(rl)?;
+        Aspect::Songs(ignore_album)
+    } else {
+        aspect
+    };
+
     // prompt: top n
     rl.helper_mut().unwrap().reset();
     println!("How many top {aspect} to plot? (recommended: ~5)");
     let usr_input_n = rl.readline(PROMPT_SECONDARY)?;
     let num: usize = usr_input_n.parse()?;
 
-    // TODO prompt: sum songs from different albums?
-
     let traces = match aspect {
         Aspect::Artists => get_traces(entries, &gather::artists(entries), num),
         Aspect::Albums => get_traces(entries, &gather::albums(entries), num),
-        Aspect::Songs => get_traces(entries, &gather::songs(entries, true), num),
+        Aspect::Songs(false) => get_traces(entries, &gather::songs(entries), num),
+        Aspect::Songs(true) => gather::songs_summed_across_albums(entries)
+            .iter()
+            .sorted_unstable_by_key(|t| (std::cmp::Reverse(t.1), t.0))
+            .take(num)
+            .map(|(aspect, _)| trace::absolute_ignore_album(entries, aspect))
+            .collect_vec(),
     };
 
-    plot::multiple(traces, &format!("Top {aspect}"));
+    let title = if ignore_album {
+        "Top songs summed across albums".to_string()
+    } else {
+        format!("Top {aspect}")
+    };
+
+    plot::multiple(traces, &title);
 
     Ok(())
 }
@@ -1002,3 +1012,22 @@ fn read_songs(
         .song(&usr_input_son, &art.name)
         .ok_or(UiError::NotFound("song from this artist"))
 }
+
+/// Used by [`match_print_top`] and [`match_plot_top`] for y/n prompt
+/// if user wants to sum song plays across albums
+fn read_whether_to_sum_songs(rl: &mut Editor<ShellHelper, FileHistory>) -> Result<bool, UiError> {
+    // prompt: ask if you want to sum songs from different albums
+    rl.helper_mut()
+        .unwrap()
+        .complete_list(string_vec(&["yes", "y", "no", "n"]));
+    println!("Do you want to sum songs from different albums? (y/n)");
+    let usr_input_b = rl.readline(PROMPT_SECONDARY)?;
+    match usr_input_b.as_str() {
+        "yes" | "y" | "true" => Ok(true),
+        "no" | "n" | "false" => Ok(false),
+        _ => {
+            println!("Invalid input. Assuming 'no'.");
+            Ok(false)
+        }
+    }
+}

src/gather.rs

@@ -39,11 +39,17 @@ use itertools::Itertools;
 use crate::aspect::{Album, Artist, HasSongs, Music, Song};
 use crate::entry::SongEntry;
 
-/// Returns a map with all [`Songs`][Song] and their playcount
+/// Returns a map with all [`Songs`][Song] and their playcount while taking
+/// the album the song is in into account
 ///
-/// `sum_songs_from_different_albums` - with `true` it will summarize the plays
-/// of songs if their name and artist is the same;
-/// with `false` it will also take into account the album the song is in
+/// See [`songs_summed_across_albums`] for a version which ignores the album
+#[must_use]
+pub fn songs(entries: &[SongEntry]) -> HashMap<Song, usize> {
+    entries.iter().map(Song::from).counts()
+}
+
+/// Like [`songs`] but summarizes the number of plays of a song if the song
+/// and artist name are the same -> ignores the album
 ///
 /// It matters because oftentimes the same song will be in many albums (or singles).
 /// But it's still case-sensitive!
@@ -52,11 +58,8 @@ use crate::entry::SongEntry;
 ///
 /// Uses .`unwrap()` but it should never panic
 #[must_use]
-pub fn songs(entries: &[SongEntry], sum_songs_from_different_albums: bool) -> HashMap<Song, usize> {
+pub fn songs_summed_across_albums(entries: &[SongEntry]) -> HashMap<Song, usize> {
     let songs = entries.iter().map(Song::from).counts();
-    if !sum_songs_from_different_albums {
-        return songs;
-    }
 
     // to know which album the song had highest amount of plays from
     // that album will be then displayed in () after the song name