refactor: general small refactors to simplify code

- Remove `expect` in favor of `unwrap` when the `Result`'s error variant
  contains the info in the `expect` anyway (eg. when locking things).
  The line number/context are given by the backtrace.
- Remove over-specification of types (`&T` instead of
  `&RWReadLockGuard`)
- Put reused values into constants
- `try_from` instead of manual function
- Change `if let Some(()) = ...` to `if T.is_some()`

src/commands.rs

@@ -113,8 +113,8 @@ impl CommandManager {
         match cmd {
             Command::Noop => Ok(None),
             Command::Quit => {
-                let queue = self.queue.queue.read().expect("can't readlock queue");
-                self.config.with_state_mut(move |mut s| {
+                let queue = self.queue.queue.read().unwrap();
+                self.config.with_state_mut(move |s| {
                     debug!(
                         "saving state, {} items, current track: {:?}",
                         queue.len(),

src/config.rs

@@ -1,12 +1,12 @@
 use std::collections::HashMap;
 use std::error::Error;
 use std::path::PathBuf;
-use std::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard};
+use std::sync::{RwLock, RwLockReadGuard};
 use std::{fs, process};
 
 use cursive::theme::Theme;
 use log::{debug, error};
-use ncspot::CONFIGURATION_FILE_NAME;
+use ncspot::{CONFIGURATION_FILE_NAME, USER_STATE_FILE_NAME};
 use platform_dirs::AppDirs;
 
 use crate::command::{SortDirection, SortKey};
@@ -209,7 +209,7 @@ impl Config {
         });
 
         let mut userstate = {
-            let path = config_path("userstate.cbor");
+            let path = config_path(USER_STATE_FILE_NAME);
             CBOR.load_or_generate_default(path, || Ok(UserState::default()), true)
                 .expect("could not load user state")
         };
@@ -234,28 +234,28 @@ impl Config {
     }
 
     pub fn values(&self) -> RwLockReadGuard<ConfigValues> {
-        self.values.read().expect("can't readlock config values")
+        self.values.read().unwrap()
     }
 
     pub fn state(&self) -> RwLockReadGuard<UserState> {
-        self.state.read().expect("can't readlock user state")
+        self.state.read().unwrap()
     }
 
     pub fn with_state_mut<F>(&self, cb: F)
     where
-        F: Fn(RwLockWriteGuard<UserState>),
+        F: Fn(&mut UserState),
     {
-        let state_guard = self.state.write().expect("can't writelock user state");
-        cb(state_guard);
+        let mut state_guard = self.state.write().unwrap();
+        cb(&mut state_guard);
     }
 
     pub fn save_state(&self) {
         // update cache version number
-        self.with_state_mut(|mut state| state.cache_version = CACHE_VERSION);
+        self.with_state_mut(|state| state.cache_version = CACHE_VERSION);
 
-        let path = config_path("userstate.cbor");
+        let path = config_path(USER_STATE_FILE_NAME);
         debug!("saving user state to {}", path.display());
-        if let Err(e) = CBOR.write(path, self.state().clone()) {
+        if let Err(e) = CBOR.write(path, &*self.state()) {
             error!("Could not save user state: {}", e);
         }
     }

src/events.rs

@@ -36,14 +36,12 @@ impl EventManager {
     }
 
     pub fn send(&self, event: Event) {
-        self.tx.send(event).expect("could not send event");
+        self.tx.send(event).unwrap();
         self.trigger();
     }
 
     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");
+        self.cursive_sink.send(Box::new(Cursive::noop)).unwrap();
     }
 }

src/ipc.rs

@@ -70,7 +70,7 @@ impl IpcSocket {
             mode: event.clone(),
             playable,
         };
-        self.tx.send(status).expect("Error publishing IPC update");
+        self.tx.send(status).unwrap();
     }
 
     async fn worker(listener: UnixListener, ev: EventManager, tx: Receiver<Status>) {

src/lib.rs

@@ -4,6 +4,7 @@ use librespot_playback::audio_backend;
 pub const AUTHOR: &str = "Henrik Friedrichsen <[email protected]> and contributors";
 pub const BIN_NAME: &str = "ncspot";
 pub const CONFIGURATION_FILE_NAME: &str = "config.toml";
+pub const USER_STATE_FILE_NAME: &str = "userstate.cbor";
 
 /// Return the [Command](clap::Command) that models the program's command line arguments. The
 /// command can be used to parse the actual arguments passed to the program, or to automatically

src/library.rs

@@ -66,7 +66,7 @@ impl Library {
     }
 
     pub fn playlists(&self) -> RwLockReadGuard<Vec<Playlist>> {
-        self.playlists.read().expect("can't readlock playlists")
+        self.playlists.read().unwrap()
     }
 
     fn load_cache<T: DeserializeOwned>(&self, cache_path: PathBuf, store: Arc<RwLock<Vec<T>>>) {
@@ -89,7 +89,7 @@ impl Library {
                         cache_path.display(),
                         cache.len()
                     );
-                    let mut store = store.write().expect("can't writelock store");
+                    let mut store = store.write().unwrap();
                     store.clear();
                     store.extend(cache);
 
@@ -119,7 +119,7 @@ impl Library {
     }
 
     fn append_or_update(&self, updated: &Playlist) -> usize {
-        let mut store = self.playlists.write().expect("can't writelock playlists");
+        let mut store = self.playlists.write().unwrap();
         for (index, local) in store.iter_mut().enumerate() {
             if local.id == updated.id {
                 *local = updated.clone();
@@ -136,14 +136,14 @@ impl Library {
         }
 
         let pos = {
-            let store = self.playlists.read().expect("can't readlock playlists");
+            let store = self.playlists.read().unwrap();
             store.iter().position(|i| i.id == id)
         };
 
         if let Some(position) = pos {
             if self.spotify.api.delete_playlist(id) {
                 {
-                    let mut store = self.playlists.write().expect("can't writelock playlists");
+                    let mut store = self.playlists.write().unwrap();
                     store.remove(position);
                 }
                 self.save_cache(config::cache_path(CACHE_PLAYLISTS), self.playlists.clone());
@@ -530,7 +530,7 @@ impl Library {
 
     pub fn playlist_update(&self, updated: &Playlist) {
         {
-            let mut playlists = self.playlists.write().expect("can't writelock playlists");
+            let mut playlists = self.playlists.write().unwrap();
             if let Some(playlist) = playlists.iter_mut().find(|p| p.id == updated.id) {
                 *playlist = updated.clone();
             }

src/mpris.rs

@@ -371,9 +371,8 @@ impl MprisPlayer {
     fn open_uri(&self, uri: &str) {
         let spotify_url = if uri.contains("open.spotify.com") {
             SpotifyUrl::from_url(uri)
-        } else if UriType::from_uri(uri).is_some() {
+        } else if let Ok(uri_type) = UriType::try_from(uri) {
             let id = &uri[uri.rfind(':').unwrap_or(0) + 1..uri.len()];
-            let uri_type = UriType::from_uri(uri).unwrap();
             Some(SpotifyUrl::new(id, uri_type))
         } else {
             None

src/queue.rs

@@ -424,7 +424,7 @@ impl Queue {
 
     /// Set the current repeat behavior and save it to the configuration.
     pub fn set_repeat(&self, new: RepeatSetting) {
-        self.cfg.with_state_mut(|mut s| s.repeat = new);
+        self.cfg.with_state_mut(|s| s.repeat = new);
     }
 
     /// Get the current shuffle behavior.
@@ -458,7 +458,7 @@ impl Queue {
 
     /// Set the current shuffle behavior.
     pub fn set_shuffle(&self, new: bool) {
-        self.cfg.with_state_mut(|mut s| s.shuffle = new);
+        self.cfg.with_state_mut(|s| s.shuffle = new);
         if new {
             self.generate_random_order();
         } else {

src/spotify.rs

@@ -52,7 +52,6 @@ pub struct Spotify {
     elapsed: Arc<RwLock<Option<Duration>>>,
     since: Arc<RwLock<Option<SystemTime>>>,
     channel: Arc<RwLock<Option<mpsc::UnboundedSender<WorkerCommand>>>>,
-    user: Option<String>,
 }
 
 impl Spotify {
@@ -66,29 +65,25 @@ impl Spotify {
             elapsed: Arc::new(RwLock::new(None)),
             since: Arc::new(RwLock::new(None)),
             channel: Arc::new(RwLock::new(None)),
-            user: None,
         };
 
         let (user_tx, user_rx) = oneshot::channel();
         spotify.start_worker(Some(user_tx));
-        spotify.user = ASYNC_RUNTIME.get().unwrap().block_on(user_rx).ok();
+        let user = ASYNC_RUNTIME.get().unwrap().block_on(user_rx).ok();
         let volume = cfg.state().volume;
         spotify.set_volume(volume);
 
         spotify.api.set_worker_channel(spotify.channel.clone());
         spotify.api.update_token();
 
-        spotify.api.set_user(spotify.user.clone());
+        spotify.api.set_user(user);
 
         spotify
     }
 
     pub fn start_worker(&self, user_tx: Option<oneshot::Sender<String>>) {
         let (tx, rx) = mpsc::unbounded_channel();
-        *self
-            .channel
-            .write()
-            .expect("can't writelock worker channel") = Some(tx);
+        *self.channel.write().unwrap() = Some(tx);
         {
             let worker_channel = self.channel.clone();
             let cfg = self.cfg.clone();
@@ -133,9 +128,10 @@ impl Spotify {
         credentials: Credentials,
     ) -> Result<Session, SessionError> {
         let librespot_cache_path = config::cache_path("librespot");
-        let audio_cache_path = match cfg.values().audio_cache.unwrap_or(true) {
-            true => Some(librespot_cache_path.join("files")),
-            false => None,
+        let audio_cache_path = if let Some(false) = cfg.values().audio_cache {
+            None
+        } else {
+            Some(librespot_cache_path.join("files"))
         };
         let cache = Cache::new(
             Some(librespot_cache_path.clone()),
@@ -230,17 +226,12 @@ impl Spotify {
         worker.run_loop().await;
 
         error!("worker thread died, requesting restart");
-        *worker_channel
-            .write()
-            .expect("can't writelock worker channel") = None;
+        *worker_channel.write().unwrap() = None;
         events.send(Event::SessionDied)
     }
 
     pub fn get_current_status(&self) -> PlayerEvent {
-        let status = self
-            .status
-            .read()
-            .expect("could not acquire read lock on playback status");
+        let status = self.status.read().unwrap();
         (*status).clone()
     }
 
@@ -253,34 +244,22 @@ impl Spotify {
     }
 
     fn set_elapsed(&self, new_elapsed: Option<Duration>) {
-        let mut elapsed = self
-            .elapsed
-            .write()
-            .expect("could not acquire write lock on elapsed time");
+        let mut elapsed = self.elapsed.write().unwrap();
         *elapsed = new_elapsed;
     }
 
     fn get_elapsed(&self) -> Option<Duration> {
-        let elapsed = self
-            .elapsed
-            .read()
-            .expect("could not acquire read lock on elapsed time");
+        let elapsed = self.elapsed.read().unwrap();
         *elapsed
     }
 
     fn set_since(&self, new_since: Option<SystemTime>) {
-        let mut since = self
-            .since
-            .write()
-            .expect("could not acquire write lock on since time");
+        let mut since = self.since.write().unwrap();
         *since = new_since;
     }
 
     fn get_since(&self) -> Option<SystemTime> {
-        let since = self
-            .since
-            .read()
-            .expect("could not acquire read lock on since time");
+        let since = self.since.read().unwrap();
         *since
     }
 
@@ -309,10 +288,7 @@ impl Spotify {
             }
         }
 
-        let mut status = self
-            .status
-            .write()
-            .expect("could not acquire write lock on player status");
+        let mut status = self.status.write().unwrap();
         *status = new_status;
     }
 
@@ -336,7 +312,7 @@ impl Spotify {
 
     fn send_worker(&self, cmd: WorkerCommand) {
         info!("sending command to worker: {:?}", cmd);
-        let channel = self.channel.read().expect("can't readlock worker channel");
+        let channel = self.channel.read().unwrap();
         match channel.as_ref() {
             Some(channel) => {
                 if let Err(e) = channel.send(cmd) {
@@ -376,7 +352,7 @@ impl Spotify {
 
     pub fn set_volume(&self, volume: u16) {
         info!("setting volume to {}", volume);
-        self.cfg.with_state_mut(|mut s| s.volume = volume);
+        self.cfg.with_state_mut(|s| s.volume = volume);
         self.send_worker(WorkerCommand::SetVolume(volume));
     }
 
@@ -399,22 +375,24 @@ pub enum UriType {
     Episode,
 }
 
-impl UriType {
-    pub fn from_uri(s: &str) -> Option<Self> {
+impl TryFrom<&str> for UriType {
+    type Error = ();
+
+    fn try_from(s: &str) -> Result<Self, Self::Error> {
         if s.starts_with("spotify:album:") {
-            Some(Self::Album)
+            Ok(Self::Album)
         } else if s.starts_with("spotify:artist:") {
-            Some(Self::Artist)
+            Ok(Self::Artist)
         } else if s.starts_with("spotify:track:") {
-            Some(Self::Track)
+            Ok(Self::Track)
         } else if s.starts_with("spotify:") && s.contains(":playlist:") {
-            Some(Self::Playlist)
+            Ok(Self::Playlist)
         } else if s.starts_with("spotify:show:") {
-            Some(Self::Show)
+            Ok(Self::Show)
         } else if s.starts_with("spotify:episode:") {
-            Some(Self::Episode)
+            Ok(Self::Episode)
         } else {
-            None
+            Err(())
         }
     }
 }