feat: make download progress smoother

ci: add some unit tests and run them every commit

.github/workflows/tests.yml

@@ -0,0 +1,25 @@
+name: Rust Unit Tests
+
+on:
+  push:
+    branches:
+      - '**'
+
+  pull_request:
+    branches:
+      - '**'
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v4
+
+      - name: Set up Rust
+        uses: actions/setup-rust@v1
+        with:
+          rust-version: stable
+
+      - name: Run tests
+        run: cargo test --all --verbose

src/audio/waiter.rs

@@ -1,22 +1,29 @@
-use std::{sync::Arc, thread::sleep, time::Duration};
+use std::{sync::Arc, time::Duration};
 
 use rodio::Sink;
 use tokio::{
     sync::{mpsc, Notify},
     task::{self, JoinHandle},
+    time,
 };
 
 pub struct Handle {
-    _task: JoinHandle<()>,
+    task: JoinHandle<()>,
     notify: Arc<Notify>,
 }
 
+impl Drop for Handle {
+    fn drop(&mut self) {
+        self.task.abort();
+    }
+}
+
 impl Handle {
     pub fn new(sink: Arc<Sink>, tx: mpsc::Sender<crate::Message>) -> Self {
         let notify = Arc::new(Notify::new());
 
         Self {
-            _task: task::spawn(Self::waiter(sink, tx, notify.clone())),
+            task: task::spawn(Self::waiter(sink, tx, notify.clone())),
             notify,
         }
     }
@@ -26,15 +33,11 @@ impl Handle {
     }
 
     async fn waiter(sink: Arc<Sink>, tx: mpsc::Sender<crate::Message>, notify: Arc<Notify>) {
-        'main: loop {
+        loop {
             notify.notified().await;
 
             while !sink.empty() {
-                if Arc::strong_count(&notify) <= 1 {
-                    break 'main;
-                }
-
-                sleep(Duration::from_millis(8));
+                time::sleep(Duration::from_millis(8)).await;
             }
 
             if let Err(_) = tx.try_send(crate::Message::Next) {

src/bookmark.rs

@@ -20,7 +20,7 @@ pub enum Error {
 /// Manages the bookmarks in the current player.
 pub struct Bookmarks {
     /// The different entries in the bookmarks file.
-    entries: Vec<String>,
+    pub(crate) entries: Vec<String>,
 }
 
 impl Bookmarks {

src/download.rs

@@ -1,5 +1,5 @@
 use std::{
-    sync::atomic::{self, AtomicU8},
+    sync::atomic::{self, AtomicBool, AtomicU8},
     time::Duration,
 };
 
@@ -11,13 +11,10 @@ use tokio::{
 
 use crate::tracks;
 
+static LOADING: AtomicBool = AtomicBool::new(false);
 static PROGRESS: AtomicU8 = AtomicU8::new(0);
 pub type Progress = &'static AtomicU8;
 
-pub fn progress() -> Progress {
-    &PROGRESS
-}
-
 pub struct Downloader {
     queue: Sender<tracks::Queued>,
     tx: Sender<crate::Message>,
@@ -47,19 +44,14 @@ impl Downloader {
 
     async fn run(self) -> crate::Result<()> {
         loop {
-            let progress = if PROGRESS.load(atomic::Ordering::Relaxed) == 0 {
-                Some(&PROGRESS)
-            } else {
-                None
-            };
-
-            let result = self.tracks.random(&self.client, progress).await;
+            let result = self.tracks.random(&self.client, &PROGRESS).await;
             match result {
                 Ok(track) => {
                     self.queue.send(track).await?;
 
-                    if progress.is_some() {
+                    if LOADING.load(atomic::Ordering::Relaxed) {
                         self.tx.send(crate::Message::Loaded).await?;
+                        LOADING.store(false, atomic::Ordering::Relaxed);
                     }
                 }
                 Err(error) => {
@@ -87,8 +79,8 @@ impl Handle {
         match self.queue.try_recv() {
             Ok(queued) => Output::Queued(queued),
             Err(_) => {
-                PROGRESS.store(0, atomic::Ordering::Relaxed);
-                Output::Loading(Some(progress()))
+                LOADING.store(true, atomic::Ordering::Relaxed);
+                Output::Loading(Some(&PROGRESS))
             }
         }
     }

src/main.rs

@@ -5,6 +5,7 @@ pub mod error;
 use std::path::PathBuf;
 
 use clap::{Parser, Subcommand};
+mod tests;
 pub use error::{Error, Result};
 pub mod message;
 pub mod ui;
@@ -66,8 +67,8 @@ pub struct Args {
     track_list: String,
 
     /// Internal song buffer size.
-    #[clap(long, short = 's', alias = "buffer", default_value_t = 5)]
-    buffer_size: usize,
+    #[clap(long, short = 's', alias = "buffer", default_value_t = 5, value_parser = clap::value_parser!(u32).range(2..))]
+    buffer_size: u32,
 
     /// The command that was ran.
     /// This is [None] if no command was specified.

src/player.rs

@@ -96,7 +96,7 @@ impl Player {
         sink.set_volume(volume.float());
 
         Ok(Self {
-            downloader: Downloader::init(args.buffer_size, list, tx.clone()).await,
+            downloader: Downloader::init(args.buffer_size as usize, list, tx.clone()).await,
             ui: ui::Handle::init(tx.clone(), urx, state, &args).await?,
             waiter: waiter::Handle::new(sink.clone(), tx.clone()),
             bookmarks: Bookmarks::load().await?,

src/tests.rs

@@ -0,0 +1,5 @@
+mod bookmark;
+mod player;
+mod tracks;
+mod ui;
+mod volume;

src/tests/bookmark.rs

@@ -0,0 +1,58 @@
+#[cfg(test)]
+mod bookmark {
+    use crate::{bookmark::Bookmarks, tracks::Info};
+
+    fn test_info(path: &str, display: &str) -> Info {
+        Info {
+            path: path.into(),
+            display: display.into(),
+            width: display.len(),
+            duration: None,
+        }
+    }
+
+    #[tokio::test]
+    async fn toggle_and_check() {
+        let mut bm = Bookmarks { entries: vec![] };
+        let info = test_info("p.mp3", "Nice Track");
+
+        // initially not bookmarked
+        assert!(!bm.bookmarked(&info));
+
+        // bookmark it
+        let added = bm.bookmark(&info).await.unwrap();
+        assert!(added);
+        assert!(bm.bookmarked(&info));
+
+        // un-bookmark it
+        let removed = bm.bookmark(&info).await.unwrap();
+        assert!(!removed);
+        assert!(!bm.bookmarked(&info));
+    }
+
+    #[tokio::test]
+    async fn multiple_bookmarks() {
+        let mut bm = Bookmarks { entries: vec![] };
+        let info1 = test_info("track1.mp3", "Track One");
+        let info2 = test_info("track2.mp3", "Track Two");
+
+        bm.bookmark(&info1).await.unwrap();
+        bm.bookmark(&info2).await.unwrap();
+
+        assert!(bm.bookmarked(&info1));
+        assert!(bm.bookmarked(&info2));
+        assert_eq!(bm.entries.len(), 2);
+    }
+
+    #[tokio::test]
+    async fn duplicate_bookmark_removes() {
+        let mut bm = Bookmarks { entries: vec![] };
+        let info = test_info("x.mp3", "X");
+
+        bm.bookmark(&info).await.unwrap();
+        let is_added = bm.bookmark(&info).await.unwrap();
+
+        assert!(!is_added);
+        assert!(bm.entries.is_empty());
+    }
+}

src/tests/player.rs

@@ -0,0 +1,41 @@
+#[cfg(test)]
+mod current {
+    use crate::player::Current;
+    use std::time::Duration;
+
+    fn test_info(path: &str, display: &str) -> crate::tracks::Info {
+        crate::tracks::Info {
+            path: path.into(),
+            display: display.into(),
+            width: display.len(),
+            duration: Some(Duration::from_secs(180)),
+        }
+    }
+
+    #[test]
+    fn default_is_loading() {
+        let c = Current::default();
+        assert!(c.loading());
+    }
+
+    #[test]
+    fn track_is_not_loading() {
+        let info = test_info("x.mp3", "Track X");
+        let c = Current::Track(info);
+        assert!(!c.loading());
+    }
+
+    #[test]
+    fn loading_without_progress() {
+        let c = Current::Loading(None);
+        assert!(c.loading());
+    }
+
+    #[test]
+    fn current_clone_works() {
+        let info = test_info("p.mp3", "P");
+        let c1 = Current::Track(info);
+        let c2 = c1.clone();
+        assert!(!c2.loading());
+    }
+}

src/tests/tracks.rs

@@ -0,0 +1,192 @@
+#[cfg(test)]
+mod format {
+    use crate::tracks::format::name;
+
+    #[test]
+    fn strips_master_patterns() {
+        let n = name("cool_track_master.mp3").unwrap();
+        assert_eq!(n, "Cool Track");
+    }
+
+    #[test]
+    fn strips_id_prefix() {
+        let n = name("a1 cool beat.mp3").unwrap();
+        assert_eq!(n, "Cool Beat");
+    }
+
+    #[test]
+    fn handles_all_numeric_name() {
+        let n = name("12345.mp3").unwrap();
+        assert_eq!(n, "12345");
+    }
+
+    #[test]
+    fn decodes_url() {
+        let n = name("lofi%20track.mp3").unwrap();
+        assert_eq!(n, "Lofi Track");
+    }
+
+    #[test]
+    fn handles_extension_only() {
+        let n = name(".mp3").unwrap();
+        // Should handle edge case gracefully
+        assert!(!n.is_empty());
+    }
+
+    #[test]
+    fn handles_mixed_case() {
+        let n = name("MyTrack_Master.mp3").unwrap();
+        assert_eq!(n, "Mytrack");
+    }
+}
+
+#[cfg(test)]
+mod queued {
+    use crate::tracks::{format, Queued};
+    use bytes::Bytes;
+
+    #[test]
+    fn queued_uses_custom_display() {
+        let q = Queued::new(
+            "path/to/file.mp3".into(),
+            Bytes::from_static(b"abc"),
+            Some("Shown".into()),
+        )
+        .unwrap();
+
+        assert_eq!(q.display, "Shown");
+        assert_eq!(q.path, "path/to/file.mp3");
+    }
+
+    #[test]
+    fn queued_generates_display_if_none() {
+        let q = Queued::new(
+            "path/to/cool_track.mp3".into(),
+            Bytes::from_static(b"abc"),
+            None,
+        )
+        .unwrap();
+
+        assert_eq!(q.display, format::name("path/to/cool_track.mp3").unwrap());
+    }
+}
+
+#[cfg(test)]
+mod info {
+    use crate::tracks::Info;
+    use unicode_segmentation::UnicodeSegmentation;
+
+    #[test]
+    fn to_entry_roundtrip() {
+        let info = Info {
+            path: "p.mp3".into(),
+            display: "Nice Track".into(),
+            width: 10,
+            duration: None,
+        };
+
+        assert_eq!(info.to_entry(), "p.mp3!Nice Track");
+    }
+
+    #[test]
+    fn info_width_counts_graphemes() {
+        // We cannot create a valid decoder for arbitrary bytes here, so test width through constructor logic directly.
+        let display = "a̐é"; // multiple-grapheme clusters
+        let width = display.graphemes(true).count();
+
+        let info = Info {
+            path: "x".into(),
+            display: display.into(),
+            width,
+            duration: None,
+        };
+
+        assert_eq!(info.width, width);
+    }
+}
+
+#[cfg(test)]
+mod decoded {
+    use crate::tracks::Queued;
+    use bytes::Bytes;
+
+    #[tokio::test]
+    async fn decoded_fails_with_invalid_audio() {
+        let q = Queued::new(
+            "path.mp3".into(),
+            Bytes::from_static(b"not audio"),
+            Some("Name".into()),
+        )
+        .unwrap();
+
+        let result = q.decode();
+        assert!(result.is_err());
+    }
+}
+
+#[cfg(test)]
+mod list {
+    use crate::tracks::List;
+
+    #[test]
+    fn list_base_works() {
+        let text = "http://base/\ntrack1\ntrack2";
+        let list = List::new("test", text, None);
+        assert_eq!(list.base(), "http://base/");
+    }
+
+    #[test]
+    fn list_random_path_parses_custom_display() {
+        let text = "http://x/\npath!Display";
+        let list = List::new("t", text, None);
+
+        let (p, d) = list.random_path();
+        assert_eq!(p, "path");
+        assert_eq!(d, Some("Display".into()));
+    }
+
+    #[test]
+    fn list_random_path_no_display() {
+        let text = "http://x/\ntrackA";
+        let list = List::new("t", text, None);
+
+        let (p, d) = list.random_path();
+        assert_eq!(p, "trackA");
+        assert!(d.is_none());
+    }
+
+    #[test]
+    fn new_trims_lines() {
+        let text = "base\na  \nb ";
+        let list = List::new("name", text, None);
+
+        assert_eq!(list.base(), "base");
+        assert_eq!(list.lines[1], "a");
+        assert_eq!(list.lines[2], "b");
+    }
+
+    #[test]
+    fn list_noheader_base() {
+        let text = "noheader\nhttps://example.com/track.mp3";
+        let list = List::new("test", text, None);
+        // noheader means the first line should be treated as base
+        assert_eq!(list.base(), "noheader");
+    }
+
+    #[test]
+    fn list_custom_display_with_exclamation() {
+        let text = "http://base/\nfile.mp3!My Custom Name";
+        let list = List::new("t", text, None);
+        let (path, display) = list.random_path();
+        assert_eq!(path, "file.mp3");
+        assert_eq!(display, Some("My Custom Name".into()));
+    }
+
+    #[test]
+    fn list_single_track() {
+        let text = "base\nonly_track.mp3";
+        let list = List::new("name", text, None);
+        let (path, _) = list.random_path();
+        assert_eq!(path, "only_track.mp3");
+    }
+}

src/tests/ui.rs

@@ -0,0 +1,164 @@
+#[cfg(test)]
+mod components {
+    use crate::ui;
+
+    use std::time::Duration;
+
+    #[test]
+    fn format_duration_works() {
+        let d = Duration::from_secs(62);
+        assert_eq!(ui::components::format_duration(&d), "01:02");
+    }
+
+    #[test]
+    fn format_duration_zero() {
+        let d = Duration::from_secs(0);
+        assert_eq!(ui::components::format_duration(&d), "00:00");
+    }
+
+    #[test]
+    fn format_duration_hours_wrap() {
+        let d = Duration::from_secs(3661); // 1:01:01
+        assert_eq!(ui::components::format_duration(&d), "61:01");
+    }
+
+    #[test]
+    fn audio_bar_contains_percentage() {
+        let s = ui::components::audio_bar(10, 0.5, "50%");
+        assert!(s.contains("50%"));
+        assert!(s.starts_with(" volume:"));
+    }
+
+    #[test]
+    fn audio_bar_muted_volume() {
+        let s = ui::components::audio_bar(8, 0.0, "0%");
+        assert!(s.contains("0%"));
+    }
+
+    #[test]
+    fn audio_bar_full_volume() {
+        let s = ui::components::audio_bar(10, 1.0, "100%");
+        assert!(s.contains("100%"));
+    }
+
+    #[test]
+    fn controls_has_items() {
+        let s = ui::components::controls(30);
+        assert!(s.contains("[s]"));
+        assert!(s.contains("[p]"));
+        assert!(s.contains("[q]"));
+    }
+}
+
+#[cfg(test)]
+mod window {
+    use crate::ui::window::Window;
+
+    #[test]
+    fn new_border_strings() {
+        let w = Window::new(10, false);
+        assert!(w.borders[0].starts_with('┌'));
+        assert!(w.borders[1].starts_with('└'));
+
+        let w2 = Window::new(5, true);
+        assert!(w2.borders[0].is_empty());
+        assert!(w2.borders[1].is_empty());
+    }
+
+    #[test]
+    fn border_width_consistency() {
+        let w = Window::new(20, false);
+        // borders should have consistent format with width encoded
+        assert!(w.borders[0].len() > 0);
+    }
+
+    #[test]
+    fn zero_width_window() {
+        let w = Window::new(0, false);
+        // Should handle zero-width gracefully
+        assert!(!w.borders[0].is_empty());
+    }
+}
+
+#[cfg(test)]
+mod environment {
+    use crate::ui::Environment;
+
+    #[test]
+    fn ready_and_cleanup_no_panic() {
+        // Try to create the environment but don't fail the test if the
+        // terminal isn't available. We just assert the API exists.
+        if let Ok(env) = Environment::ready(false) {
+            // cleanup should succeed
+            let _ = env.cleanup(true);
+        }
+    }
+
+    #[test]
+    fn ready_with_alternate_screen() {
+        if let Ok(env) = Environment::ready(true) {
+            let _ = env.cleanup(false);
+        }
+    }
+}
+
+#[cfg(test)]
+mod integration {
+    use std::sync::Arc;
+
+    use rodio::OutputStreamBuilder;
+
+    use crate::{player::Current, Args};
+
+    fn try_make_state() -> Option<crate::ui::State> {
+        let stream = OutputStreamBuilder::open_default_stream();
+        if stream.is_err() {
+            return None;
+        }
+
+        let mut stream = stream.unwrap();
+        stream.log_on_drop(false);
+        let sink = Arc::new(rodio::Sink::connect_new(stream.mixer()));
+
+        let args = Args {
+            alternate: false,
+            minimalist: false,
+            borderless: false,
+            paused: false,
+            fps: 12,
+            timeout: 3,
+            debug: false,
+            width: 3,
+            track_list: String::from("chillhop"),
+            buffer_size: 5,
+            command: None,
+        };
+
+        let current = Current::default();
+        Some(crate::ui::State::initial(
+            sink,
+            &args,
+            current,
+            String::from("list"),
+        ))
+    }
+
+    #[test]
+    fn progress_bar_runs() -> Result<(), Box<dyn std::error::Error>> {
+        if let Some(state) = try_make_state() {
+            // ensure we can call progress_bar without panic
+            let _ = crate::ui::components::progress_bar(&state, state.width);
+        }
+
+        Ok(())
+    }
+
+    #[test]
+    fn action_runs() -> Result<(), Box<dyn std::error::Error>> {
+        if let Some(state) = try_make_state() {
+            let _ = crate::ui::components::action(&state, state.width);
+        }
+
+        Ok(())
+    }
+}

src/tests/volume.rs

@@ -0,0 +1,28 @@
+#[cfg(test)]
+mod volume {
+    use crate::volume::PersistentVolume;
+
+    #[test]
+    fn float_converts_percent() {
+        let pv = PersistentVolume { inner: 75 };
+        assert!((pv.float() - 0.75).abs() < f32::EPSILON);
+    }
+
+    #[test]
+    fn float_zero_volume() {
+        let pv = PersistentVolume { inner: 0 };
+        assert_eq!(pv.float(), 0.0);
+    }
+
+    #[test]
+    fn float_full_volume() {
+        let pv = PersistentVolume { inner: 100 };
+        assert_eq!(pv.float(), 1.0);
+    }
+
+    #[test]
+    fn float_mid_range() {
+        let pv = PersistentVolume { inner: 50 };
+        assert!((pv.float() - 0.5).abs() < f32::EPSILON);
+    }
+}

src/tracks/list.rs

@@ -32,7 +32,7 @@ pub struct List {
 
     /// Just the raw file, but seperated by `/n` (newlines).
     /// `lines[0]` is the base/heaeder, with the rest being tracks.
-    lines: Vec<String>,
+    pub lines: Vec<String>,
 
     /// The file path which the list was read from.
     #[allow(dead_code)]
@@ -49,7 +49,7 @@ impl List {
     ///
     /// The second value in the tuple specifies whether the
     /// track has a custom display name.
-    fn random_path(&self) -> (String, Option<String>) {
+    pub fn random_path(&self) -> (String, Option<String>) {
         // We're getting from 1 here, since the base is at `self.lines[0]`.
         //
         // We're also not pre-trimming `self.lines` into `base` & `tracks` due to
@@ -94,10 +94,6 @@ impl List {
                 x.to_owned()
             };
 
-            if let Some(progress) = progress {
-                progress.store(100, Ordering::Relaxed);
-            }
-
             let result = tokio::fs::read(path.clone()).await.track(x)?;
             result.into()
         } else {
@@ -134,13 +130,9 @@ impl List {
     ///
     /// The Result's error is a bool, which is true if a timeout error occured,
     /// and false otherwise. This tells lowfi if it shouldn't wait to try again.
-    pub async fn random(
-        &self,
-        client: &Client,
-        progress: Option<&AtomicU8>,
-    ) -> tracks::Result<Queued> {
+    pub async fn random(&self, client: &Client, progress: &AtomicU8) -> tracks::Result<Queued> {
         let (path, display) = self.random_path();
-        let (data, path) = self.download(&path, client, progress).await?;
+        let (data, path) = self.download(&path, client, Some(progress)).await?;
 
         Queued::new(path, data, display)
     }

src/ui.rs

@@ -10,12 +10,12 @@ use tokio::{
     task::JoinHandle,
     time::Instant,
 };
-mod components;
-mod environment;
+pub mod components;
+pub mod environment;
 pub use environment::Environment;
-mod input;
-mod interface;
-mod window;
+pub mod input;
+pub mod interface;
+pub mod window;
 
 #[cfg(feature = "mpris")]
 pub mod mpris;
@@ -54,7 +54,7 @@ pub struct State {
     pub bookmarked: bool,
     list: String,
     timer: Option<Instant>,
-    width: usize,
+    pub(crate) width: usize,
 }
 
 impl State {

src/ui/components.rs

@@ -80,7 +80,10 @@ impl ActionBar {
             Self::Playing(x) => ("playing", Some((x.display.clone(), x.width))),
             Self::Paused(x) => ("paused", Some((x.display.clone(), x.width))),
             Self::Loading(progress) => {
-                let progress = progress.map(|progress| (format!("{: <2.0}%", progress.min(99)), 3));
+                let progress = match *progress {
+                    None | Some(0) => None,
+                    Some(progress) => Some((format!("{: <2.0}%", progress.min(99)), 3)),
+                };
 
                 ("loading", progress)
             }

src/ui/interface.rs

@@ -45,10 +45,10 @@ pub async fn draw(state: &mut ui::State, window: &mut Window, params: Params) ->
     };
 
     let controls = components::controls(state.width);
-    let menu = match (params.minimalist, &state.current) {
-        (true, _) => vec![action, middle],
-        // (false, Some(x)) => vec![x.path.clone(), action, middle, controls],
-        _ => vec![action, middle, controls],
+    let menu = if params.minimalist {
+        vec![action, middle]
+    } else {
+        vec![action, middle, controls]
     };
 
     window.draw(menu, false)?;

src/ui/window.rs

@@ -20,7 +20,7 @@ pub struct Window {
     /// prerendered, as they don't change from window to window.
     ///
     /// If the option to not include borders is set, these will just be empty [String]s.
-    borders: [String; 2],
+    pub(crate) borders: [String; 2],
 
     /// The width of the window.
     width: usize,

src/volume.rs

@@ -20,7 +20,7 @@ pub enum Error {
 #[derive(Clone, Copy)]
 pub struct PersistentVolume {
     /// The volume, as a percentage.
-    inner: u16,
+    pub(crate) inner: u16,
 }
 
 impl PersistentVolume {