docs: small overall documentation improvements (#1381)

* docs: small overall documentation improvements

- Add documentation comments to various items
- Change web API return types from bool/Option to Result
- Create helper functions with descriptive names instead of comments
- Remove redundant/confusing types
- Fix some documentation comments as instructed by `cargo doc`
- Rename variables to clear names

* docs: small fixes to the documentation update

src/commands.rs

@@ -238,8 +238,8 @@ impl CommandManager {
             }
             Command::NewPlaylist(name) => {
                 match self.spotify.api.create_playlist(name, None, None) {
-                    Some(_) => self.library.update_library(),
-                    None => error!("could not create playlist {}", name),
+                    Ok(_) => self.library.update_library(),
+                    Err(_) => error!("could not create playlist {}", name),
                 }
                 Ok(None)
             }

src/config.rs

@@ -233,14 +233,17 @@ impl Config {
         }
     }
 
+    /// Get the user configuration values.
     pub fn values(&self) -> RwLockReadGuard<ConfigValues> {
         self.values.read().expect("can't readlock config values")
     }
 
+    /// Get the runtime user state values.
     pub fn state(&self) -> RwLockReadGuard<UserState> {
         self.state.read().expect("can't readlock user state")
     }
 
+    /// Modify the internal user state through a shared reference using a closure.
     pub fn with_state_mut<F>(&self, cb: F)
     where
         F: Fn(RwLockWriteGuard<UserState>),
@@ -249,9 +252,15 @@ impl Config {
         cb(state_guard);
     }
 
-    pub fn save_state(&self) {
-        // update cache version number
+    /// Update the version number of the runtime user state. This should be done before saving it to
+    /// disk.
+    fn update_state_cache_version(&self) {
         self.with_state_mut(|mut state| state.cache_version = CACHE_VERSION);
+    }
+
+    /// Save runtime state to the user configuration directory.
+    pub fn save_state(&self) {
+        self.update_state_cache_version();
 
         let path = config_path("userstate.cbor");
         debug!("saving user state to {}", path.display());
@@ -260,9 +269,9 @@ impl Config {
         }
     }
 
+    /// Create a [Theme] from the user supplied theme in the configuration file.
     pub fn build_theme(&self) -> Theme {
-        let theme = &self.values().theme;
-        crate::theme::load(theme)
+        crate::theme::load(&self.values().theme)
     }
 
     /// Attempt to reload the configuration from the configuration file.

src/events.rs

@@ -4,18 +4,18 @@ use cursive::{CbSink, Cursive};
 use crate::queue::QueueEvent;
 use crate::spotify::PlayerEvent;
 
+/// Events that can be sent to and handled by the main event loop (the one drawing the TUI).
 pub enum Event {
     Player(PlayerEvent),
     Queue(QueueEvent),
     SessionDied,
     IpcInput(String),
 }
 
-pub type EventSender = Sender<Event>;
-
+/// Manager that can be used to send and receive messages across threads.
 #[derive(Clone)]
 pub struct EventManager {
-    tx: EventSender,
+    tx: Sender<Event>,
     rx: Receiver<Event>,
     cursive_sink: CbSink,
 }
@@ -31,17 +31,20 @@ impl EventManager {
         }
     }
 
+    /// Return a non-blocking iterator over the messages awaiting handling. Calling `next()` on the
+    /// iterator never blocks.
     pub fn msg_iter(&self) -> TryIter<Event> {
         self.rx.try_iter()
     }
 
+    /// Send a new event to be handled.
     pub fn send(&self, event: Event) {
         self.tx.send(event).expect("could not send event");
         self.trigger();
     }
 
+    /// Send a no-op to the Cursive event loop to trigger immediate processing of events.
     pub fn trigger(&self) {
-        // send a no-op to trigger event loop processing
         self.cursive_sink
             .send(Box::new(Cursive::noop))
             .expect("could not send no-op event to cursive");

src/library.rs

@@ -21,11 +21,20 @@ use crate::model::show::Show;
 use crate::model::track::Track;
 use crate::spotify::Spotify;
 
+/// Cached tracks database filename.
 const CACHE_TRACKS: &str = "tracks.db";
+
+/// Cached albums database filename.
 const CACHE_ALBUMS: &str = "albums.db";
+
+/// Cached artists database filename.
 const CACHE_ARTISTS: &str = "artists.db";
+
+/// Cached playlists database filename.
 const CACHE_PLAYLISTS: &str = "playlists.db";
 
+/// The user library with all their saved tracks, albums, playlists... High level interface to the
+/// Spotify API used to manage items in the user library.
 #[derive(Clone)]
 pub struct Library {
     pub tracks: Arc<RwLock<Vec<Track>>>,
@@ -43,7 +52,7 @@ pub struct Library {
 
 impl Library {
     pub fn new(ev: EventManager, spotify: Spotify, cfg: Arc<Config>) -> Self {
-        let current_user = spotify.api.current_user();
+        let current_user = spotify.api.current_user().ok();
         let user_id = current_user.as_ref().map(|u| u.id.id().to_string());
         let display_name = current_user.as_ref().and_then(|u| u.display_name.clone());
 
@@ -149,7 +158,7 @@ impl Library {
             .position(|i| i.id == id);
 
         if let Some(position) = position {
-            if self.spotify.api.delete_playlist(id) {
+            if self.spotify.api.delete_playlist(id).is_ok() {
                 self.playlists
                     .write()
                     .expect("can't writelock playlists")
@@ -163,7 +172,7 @@ impl Library {
     }
 
     /// Set the playlist with `id` to contain only `tracks`. If the playlist already contains
-    /// tracks, they will be removed.
+    /// tracks, they will be removed. Update the cache to match the new state.
     pub fn overwrite_playlist(&self, id: &str, tracks: &[Playable]) {
         debug!("saving {} tracks to list {}", tracks.len(), id);
         self.spotify.api.overwrite_playlist(id, tracks);
@@ -179,12 +188,12 @@ impl Library {
     pub fn save_playlist(&self, name: &str, tracks: &[Playable]) {
         debug!("saving {} tracks to new list {}", tracks.len(), name);
         match self.spotify.api.create_playlist(name, None, None) {
-            Some(id) => self.overwrite_playlist(&id, tracks),
-            None => error!("could not create new playlist.."),
+            Ok(id) => self.overwrite_playlist(&id, tracks),
+            Err(_) => error!("could not create new playlist.."),
         }
     }
 
-    /// Update the local copy and cache of the library with the remote data.
+    /// Update the local library and its cache on disk.
     pub fn update_library(&self) {
         *self.is_done.write().unwrap() = false;
 
@@ -278,7 +287,7 @@ impl Library {
         debug!("loading shows");
 
         let mut saved_shows: Vec<Show> = Vec::new();
-        let mut shows_result = self.spotify.api.get_saved_shows(0);
+        let mut shows_result = self.spotify.api.get_saved_shows(0).ok();
 
         while let Some(shows) = shows_result {
             saved_shows.extend(shows.items.iter().map(|show| (&show.show).into()));
@@ -290,6 +299,7 @@ impl Library {
                     self.spotify
                         .api
                         .get_saved_shows(shows.offset + shows.items.len() as u32)
+                        .ok()
                 }
                 None => None,
             }
@@ -364,7 +374,7 @@ impl Library {
             let page = self.spotify.api.current_user_followed_artists(last);
             debug!("artists page: {}", i);
             i += 1;
-            if page.is_none() {
+            if page.is_err() {
                 error!("Failed to fetch artists.");
                 return;
             }
@@ -422,7 +432,7 @@ impl Library {
 
             i += 1;
 
-            if page.is_none() {
+            if page.is_err() {
                 error!("Failed to fetch albums.");
                 return;
             }
@@ -465,7 +475,7 @@ impl Library {
             debug!("tracks page: {}", i);
             i += 1;
 
-            if page.is_none() {
+            if page.is_err() {
                 error!("Failed to fetch tracks.");
                 return;
             }
@@ -604,7 +614,7 @@ impl Library {
             .api
             .current_user_saved_tracks_add(tracks.iter().filter_map(|t| t.id.as_deref()).collect());
 
-        if save_tracks_result.is_none() {
+        if save_tracks_result.is_err() {
             return;
         }
 
@@ -645,7 +655,7 @@ impl Library {
             .current_user_saved_tracks_delete(
                 tracks.iter().filter_map(|t| t.id.as_deref()).collect(),
             )
-            .is_none()
+            .is_err()
         {
             return;
         }
@@ -692,7 +702,7 @@ impl Library {
                 .spotify
                 .api
                 .current_user_saved_albums_add(vec![album_id.as_str()])
-                .is_none()
+                .is_err()
             {
                 return;
             }
@@ -725,7 +735,7 @@ impl Library {
                 .spotify
                 .api
                 .current_user_saved_albums_delete(vec![album_id.as_str()])
-                .is_none()
+                .is_err()
             {
                 return;
             }
@@ -763,7 +773,7 @@ impl Library {
                 .spotify
                 .api
                 .user_follow_artists(vec![artist_id.as_str()])
-                .is_none()
+                .is_err()
             {
                 return;
             }
@@ -799,7 +809,7 @@ impl Library {
                 .spotify
                 .api
                 .user_unfollow_artists(vec![artist_id.as_str()])
-                .is_none()
+                .is_err()
             {
                 return;
             }
@@ -846,7 +856,7 @@ impl Library {
 
         let follow_playlist_result = self.spotify.api.user_playlist_follow_playlist(&playlist.id);
 
-        if follow_playlist_result.is_none() {
+        if follow_playlist_result.is_err() {
             return;
         }
 
@@ -881,7 +891,7 @@ impl Library {
             return;
         }
 
-        if self.spotify.api.save_shows(&[show.id.as_str()]) {
+        if self.spotify.api.save_shows(&[show.id.as_str()]).is_ok() {
             {
                 let mut store = self.shows.write().unwrap();
                 if !store.iter().any(|s| s.id == show.id) {
@@ -897,7 +907,7 @@ impl Library {
             return;
         }
 
-        if self.spotify.api.unsave_shows(&[show.id.as_str()]) {
+        if self.spotify.api.unsave_shows(&[show.id.as_str()]).is_ok() {
             let mut store = self.shows.write().unwrap();
             *store = store.iter().filter(|s| s.id != show.id).cloned().collect();
         }

src/model/album.rs

@@ -38,7 +38,7 @@ impl Album {
 
         if let Some(ref album_id) = self.id {
             let mut collected_tracks = Vec::new();
-            if let Some(full_album) = spotify.api.album(album_id) {
+            if let Ok(full_album) = spotify.api.album(album_id) {
                 let mut tracks_result = Some(full_album.tracks.clone());
                 while let Some(ref tracks) = tracks_result {
                     for t in &tracks.items {
@@ -51,11 +51,14 @@ impl Album {
                     tracks_result = match tracks.next {
                         Some(_) => {
                             debug!("requesting tracks again..");
-                            spotify.api.album_tracks(
-                                album_id,
-                                50,
-                                tracks.offset + tracks.items.len() as u32,
-                            )
+                            spotify
+                                .api
+                                .album_tracks(
+                                    album_id,
+                                    50,
+                                    tracks.offset + tracks.items.len() as u32,
+                                )
+                                .ok()
                         }
                         None => None,
                     }
@@ -273,6 +276,7 @@ impl ListItem for Album {
                 None,
                 Some(track_ids),
             )
+            .ok()
             .map(|r| r.tracks)
             .map(|tracks| tracks.iter().map(Track::from).collect());
         recommendations.map(|tracks| {

src/model/artist.rs

@@ -35,7 +35,7 @@ impl Artist {
     fn load_top_tracks(&mut self, spotify: Spotify) {
         if let Some(artist_id) = &self.id {
             if self.tracks.is_none() {
-                self.tracks = spotify.api.artist_top_tracks(artist_id);
+                self.tracks = spotify.api.artist_top_tracks(artist_id).ok();
             }
         }
     }
@@ -182,6 +182,7 @@ impl ListItem for Artist {
         let recommendations: Option<Vec<Track>> = spotify
             .api
             .recommendations(Some(vec![&id]), None, None)
+            .ok()
             .map(|r| r.tracks)
             .map(|tracks| tracks.iter().map(Track::from).collect());
 

src/model/playlist.rs

@@ -67,6 +67,7 @@ impl Playlist {
         match spotify
             .api
             .delete_tracks(&self.id, &self.snapshot_id, &[playable])
+            .is_ok()
         {
             false => false,
             true => {
@@ -83,7 +84,11 @@ impl Playlist {
     pub fn append_tracks(&mut self, new_tracks: &[Playable], spotify: &Spotify, library: &Library) {
         let mut has_modified = false;
 
-        if spotify.api.append_tracks(&self.id, new_tracks, None) {
+        if spotify
+            .api
+            .append_tracks(&self.id, new_tracks, None)
+            .is_ok()
+        {
             if let Some(tracks) = &mut self.tracks {
                 tracks.append(&mut new_tracks.to_vec());
                 has_modified = true;
@@ -304,6 +309,7 @@ impl ListItem for Playlist {
                 None,
                 Some(track_ids.iter().map(|t| t.as_ref()).collect()),
             )
+            .ok()
             .map(|r| r.tracks)
             .map(|tracks| tracks.iter().map(Track::from).collect());
 

src/model/track.rs

@@ -278,6 +278,7 @@ impl ListItem for Track {
             spotify
                 .api
                 .recommendations(None, None, Some(vec![id]))
+                .ok()
                 .map(|r| r.tracks)
                 .map(|tracks| tracks.iter().map(Self::from).collect())
         } else {
@@ -309,7 +310,7 @@ impl ListItem for Track {
         let spotify = queue.get_spotify();
 
         match self.album_id {
-            Some(ref album_id) => spotify.api.album(album_id).map(|ref fa| fa.into()),
+            Some(ref album_id) => spotify.api.album(album_id).map(|ref fa| fa.into()).ok(),
             None => None,
         }
     }