Merge pull request #582 from Johannesd3/refactor_cache_remove_volume

audio/src/fetch.rs

@@ -429,8 +429,8 @@ impl AudioFile {
             complete_rx
                 .map(move |mut file| {
                     if let Some(cache) = session_.cache() {
-                        cache.save_file(file_id, &mut file);
                         debug!("File {} complete, saving to cache", file_id);
+                        cache.save_file(file_id, &mut file);
                     } else {
                         debug!("File {} complete", file_id);
                     }

connect/src/spirc.rs

@@ -21,7 +21,6 @@ use librespot_core::spotify_id::{SpotifyAudioType, SpotifyId, SpotifyIdError};
 use librespot_core::util::url_encode;
 use librespot_core::util::SeqGenerator;
 use librespot_core::version;
-use librespot_core::volume::Volume;
 
 enum SpircPlayStatus {
     Stopped,
@@ -1297,7 +1296,7 @@ impl SpircTask {
         self.mixer
             .set_volume(volume_to_mixer(volume, &self.config.volume_ctrl));
         if let Some(cache) = self.session.cache() {
-            cache.save_volume(Volume { volume })
+            cache.save_volume(volume)
         }
         self.player.emit_volume_set_event(volume);
     }

core/src/authentication.rs

@@ -1,16 +1,10 @@
 use aes::Aes192;
-use base64;
 use byteorder::{BigEndian, ByteOrder};
 use hmac::Hmac;
 use pbkdf2::pbkdf2;
 use protobuf::ProtobufEnum;
-use serde;
-use serde_json;
 use sha1::{Digest, Sha1};
-use std::fs::File;
+use std::io::{self, Read};
-use std::io::{self, Read, Write};
-use std::ops::FnOnce;
-use std::path::Path;
 
 use crate::protocol::authentication::AuthenticationType;
 
@@ -112,27 +106,6 @@ impl Credentials {
             auth_data: auth_data,
         }
     }
-
-    fn from_reader<R: Read>(mut reader: R) -> Credentials {
-        let mut contents = String::new();
-        reader.read_to_string(&mut contents).unwrap();
-
-        serde_json::from_str(&contents).unwrap()
-    }
-
-    pub(crate) fn from_file<P: AsRef<Path>>(path: P) -> Option<Credentials> {
-        File::open(path).ok().map(Credentials::from_reader)
-    }
-
-    fn save_to_writer<W: Write>(&self, writer: &mut W) {
-        let contents = serde_json::to_string(&self.clone()).unwrap();
-        writer.write_all(contents.as_bytes()).unwrap();
-    }
-
-    pub(crate) fn save_to_file<P: AsRef<Path>>(&self, path: P) {
-        let mut file = File::create(path).unwrap();
-        self.save_to_writer(&mut file)
-    }
 }
 
 fn serialize_protobuf_enum<T, S>(v: &T, ser: S) -> Result<S::Ok, S::Error>

core/src/cache.rs

@@ -1,115 +1,165 @@
 use std::fs;
 use std::fs::File;
-use std::io;
+use std::io::{self, Error, ErrorKind, Read, Write};
-use std::io::Read;
+use std::path::{Path, PathBuf};
-use std::path::Path;
-use std::path::PathBuf;
 
 use crate::authentication::Credentials;
 use crate::spotify_id::FileId;
-use crate::volume::Volume;
 
+/// A cache for volume, credentials and audio files.
 #[derive(Clone)]
 pub struct Cache {
-    audio_root: PathBuf,
+    credentials_location: Option<PathBuf>,
-    system_root: PathBuf,
+    volume_location: Option<PathBuf>,
-    use_audio_cache: bool,
+    audio_location: Option<PathBuf>,
-}
-
-fn mkdir_existing(path: &Path) -> io::Result<()> {
-    fs::create_dir(path).or_else(|err| {
-        if err.kind() == io::ErrorKind::AlreadyExists {
-            Ok(())
-        } else {
-            Err(err)
-        }
-    })
 }
 
 impl Cache {
-    pub fn new(audio_location: PathBuf, system_location: PathBuf, use_audio_cache: bool) -> Cache {
+    pub fn new<P: AsRef<Path>>(
-        if use_audio_cache == true {
+        system_location: Option<P>,
-            mkdir_existing(&audio_location).unwrap();
+        audio_location: Option<P>,
-            mkdir_existing(&audio_location.join("files")).unwrap();
+    ) -> io::Result<Self> {
+        if let Some(location) = &system_location {
+            fs::create_dir_all(location)?;
         }
-        mkdir_existing(&system_location).unwrap();
 
-        Cache {
+        if let Some(location) = &audio_location {
-            audio_root: audio_location,
+            fs::create_dir_all(location)?;
-            system_root: system_location,
-            use_audio_cache: use_audio_cache,
         }
-    }
-}
 
-impl Cache {
+        let audio_location = audio_location.map(|p| p.as_ref().to_owned());
-    fn credentials_path(&self) -> PathBuf {
+        let volume_location = system_location.as_ref().map(|p| p.as_ref().join("volume"));
-        self.system_root.join("credentials.json")
+        let credentials_location = system_location
+            .as_ref()
+            .map(|p| p.as_ref().join("credentials.json"));
+
+        let cache = Cache {
+            credentials_location,
+            volume_location,
+            audio_location,
+        };
+
+        Ok(cache)
     }
 
     pub fn credentials(&self) -> Option<Credentials> {
-        let path = self.credentials_path();
+        let location = self.credentials_location.as_ref()?;
-        Credentials::from_file(path)
+
+        // This closure is just convencience to enable the question mark operator
+        let read = || {
+            let mut file = File::open(location)?;
+            let mut contents = String::new();
+            file.read_to_string(&mut contents)?;
+            serde_json::from_str(&contents).map_err(|e| Error::new(ErrorKind::InvalidData, e))
+        };
+
+        match read() {
+            Ok(c) => Some(c),
+            Err(e) => {
+                // If the file did not exist, the file was probably not written
+                // before. Otherwise, log the error.
+                if e.kind() != ErrorKind::NotFound {
+                    warn!("Error reading credentials from cache: {}", e);
+                }
+                None
+            }
+        }
     }
 
     pub fn save_credentials(&self, cred: &Credentials) {
-        let path = self.credentials_path();
+        if let Some(location) = &self.credentials_location {
-        cred.save_to_file(&path);
+            let result = File::create(location).and_then(|mut file| {
-    }
+                let data = serde_json::to_string(cred)?;
-}
+                write!(file, "{}", data)
+            });
 
-// cache volume to system_root/volume
+            if let Err(e) = result {
-impl Cache {
+                warn!("Cannot save credentials to cache: {}", e)
-    fn volume_path(&self) -> PathBuf {
+            }
-        self.system_root.join("volume")
+        }
     }
 
     pub fn volume(&self) -> Option<u16> {
-        let path = self.volume_path();
+        let location = self.volume_location.as_ref()?;
-        Volume::from_file(path)
+
+        let read = || {
+            let mut file = File::open(location)?;
+            let mut contents = String::new();
+            file.read_to_string(&mut contents)?;
+            contents
+                .parse()
+                .map_err(|e| Error::new(ErrorKind::InvalidData, e))
+        };
+
+        match read() {
+            Ok(v) => Some(v),
+            Err(e) => {
+                if e.kind() != ErrorKind::NotFound {
+                    warn!("Error reading volume from cache: {}", e);
+                }
+                None
+            }
+        }
     }
 
-    pub fn save_volume(&self, volume: Volume) {
+    pub fn save_volume(&self, volume: u16) {
-        let path = self.volume_path();
+        if let Some(ref location) = self.volume_location {
-        volume.save_to_file(&path);
+            let result = File::create(location).and_then(|mut file| write!(file, "{}", volume));
+            if let Err(e) = result {
+                warn!("Cannot save volume to cache: {}", e);
+            }
+        }
     }
-}
 
-impl Cache {
+    fn file_path(&self, file: FileId) -> Option<PathBuf> {
-    fn file_path(&self, file: FileId) -> PathBuf {
+        self.audio_location.as_ref().map(|location| {
-        let name = file.to_base16();
+            let name = file.to_base16();
-        self.audio_root
+            let mut path = location.join(&name[0..2]);
-            .join("files")
+            path.push(&name[2..]);
-            .join(&name[0..2])
+            path
-            .join(&name[2..])
+        })
     }
 
     pub fn file(&self, file: FileId) -> Option<File> {
-        File::open(self.file_path(file)).ok()
+        File::open(self.file_path(file)?)
+            .map_err(|e| {
+                if e.kind() != ErrorKind::NotFound {
+                    warn!("Error reading file from cache: {}", e)
+                }
+            })
+            .ok()
     }
 
-    pub fn save_file(&self, file: FileId, contents: &mut dyn Read) {
+    pub fn save_file<F: Read>(&self, file: FileId, contents: &mut F) {
-        if self.use_audio_cache {
+        let path = if let Some(path) = self.file_path(file) {
-            let path = self.file_path(file);
+            path
-            mkdir_existing(path.parent().unwrap()).unwrap();
+        } else {
+            return;
+        };
+        let parent = path.parent().unwrap();
 
-            let mut cache_file = File::create(path).unwrap_or_else(|_e| {
+        let result = fs::create_dir_all(parent)
-                ::std::fs::remove_dir_all(&self.audio_root.join("files")).unwrap();
+            .and_then(|_| File::create(&path))
-                mkdir_existing(&self.audio_root.join("files")).unwrap();
+            .and_then(|mut file| io::copy(contents, &mut file));
 
-                let path = self.file_path(file);
+        if let Err(e) = result {
-                mkdir_existing(path.parent().unwrap()).unwrap();
+            if e.kind() == ErrorKind::Other {
-                File::create(path).unwrap()
+                // Perhaps there's no space left in the cache
-            });
+                // TODO: try to narrow down the error (platform-dependently)
-            ::std::io::copy(contents, &mut cache_file).unwrap_or_else(|_e| {
+                info!("An error occured while writing to cache, trying to flush the cache");
-                ::std::fs::remove_dir_all(&self.audio_root.join("files")).unwrap();
-                mkdir_existing(&self.audio_root.join("files")).unwrap();
 
-                let path = self.file_path(file);
+                if fs::remove_dir_all(self.audio_location.as_ref().unwrap())
-                mkdir_existing(path.parent().unwrap()).unwrap();
+                    .and_then(|_| fs::create_dir_all(parent))
-                let mut file = File::create(path).unwrap();
+                    .and_then(|_| File::create(&path))
-                ::std::io::copy(contents, &mut file).unwrap()
+                    .and_then(|mut file| io::copy(contents, &mut file))
-            });
+                    .is_ok()
+                {
+                    // It worked, there's no need to print a warning
+                    return;
+                }
+            }
+
+            warn!("Cannot save file to cache: {}", e)
         }
     }
 }

core/src/lib.rs

@@ -54,4 +54,3 @@ pub mod session;
 pub mod spotify_id;
 pub mod util;
 pub mod version;
-pub mod volume;

core/src/volume.rs

@@ -1,33 +0,0 @@
-use std::fs::File;
-use std::io::{Read, Write};
-use std::path::Path;
-
-#[derive(Clone, Copy, Debug)]
-pub struct Volume {
-    pub volume: u16,
-}
-
-impl Volume {
-    // read volume from file
-    fn from_reader<R: Read>(mut reader: R) -> u16 {
-        let mut contents = String::new();
-        reader.read_to_string(&mut contents).unwrap();
-        contents.trim().parse::<u16>().unwrap()
-    }
-
-    pub(crate) fn from_file<P: AsRef<Path>>(path: P) -> Option<u16> {
-        File::open(path).ok().map(Volume::from_reader)
-    }
-
-    // write volume to file
-    fn save_to_writer<W: Write>(&self, writer: &mut W) {
-        writer
-            .write_all(self.volume.to_string().as_bytes())
-            .unwrap();
-    }
-
-    pub(crate) fn save_to_file<P: AsRef<Path>>(&self, path: P) {
-        let mut file = File::create(path).unwrap();
-        self.save_to_writer(&mut file)
-    }
-}

src/main.rs

@@ -5,7 +5,7 @@ use sha1::{Digest, Sha1};
 use std::env;
 use std::io::{self, stderr, Write};
 use std::mem;
-use std::path::PathBuf;
+use std::path::Path;
 use std::process::exit;
 use std::str::FromStr;
 use std::time::Instant;
@@ -254,18 +254,34 @@ fn setup(args: &[String]) -> Setup {
         mapped_volume: !matches.opt_present("mixer-linear-volume"),
     };
 
-    let cache = matches.opt_str("c").map(|cache_path| {
+    let cache = {
-        let use_audio_cache = !matches.opt_present("disable-audio-cache");
+        let audio_dir;
-        let system_cache_directory = matches
+        let system_dir;
-            .opt_str("system-cache")
+        if matches.opt_present("disable-audio-cache") {
-            .unwrap_or(String::from(cache_path.clone()));
+            audio_dir = None;
+            system_dir = matches
+                .opt_str("system-cache")
+                .or_else(|| matches.opt_str("c"))
+                .map(|p| p.into());
+        } else {
+            let cache_dir = matches.opt_str("c");
+            audio_dir = cache_dir
+                .as_ref()
+                .map(|p| AsRef::<Path>::as_ref(p).join("files"));
+            system_dir = matches
+                .opt_str("system-cache")
+                .or_else(|| cache_dir)
+                .map(|p| p.into());
+        }
 
-        Cache::new(
+        match Cache::new(system_dir, audio_dir) {
-            PathBuf::from(cache_path),
+            Ok(cache) => Some(cache),
-            PathBuf::from(system_cache_directory),
+            Err(e) => {
-            use_audio_cache,
+                warn!("Cannot create cache: {}", e);
-        )
+                None
-    });
+            }
+        }
+    };
 
     let initial_volume = matches
         .opt_str("initial-volume")