lostecho/ncspot
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

refactor: general small refactors to simplify code

Browse Source 
- 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
- `FromStr` instead of manual function
- Change `if let Some(()) = ...` to `if T.is_some()`
This commit is contained in:
Thomas Frans
2024-02-01 22:05:43 +01:00
committed by Henrik Friedrichsen
parent c5d666f35c
commit 3c8e546445
12 changed files with 97 additions and 96 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
src/commands.rs
View File

@@ -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(),

22
src/config.rs
View File

@@ -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")
};
@@ -235,36 +235,36 @@ impl Config {
/// Get the user configuration values.
pub fn values(&self) -> RwLockReadGuard<ConfigValues> {
self.values.read().expect("can't readlock config values")
self.values.read().unwrap()
}
/// Get the runtime user state values.
pub fn state(&self) -> RwLockReadGuard<UserState> {
self.state.read().expect("can't readlock user state")
self.state.read().unwrap()
}
/// 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>),
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);
}
/// 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);
self.with_state_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");
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);
}
}

6
src/events.rs
View File

@@ -39,14 +39,12 @@ impl EventManager {
/// Send a new event to be handled.
pub fn send(&self, event: Event) {
self.tx.send(event).expect("could not send event");
self.tx.send(event).unwrap();
self.trigger();
}
/// Send a no-op to the Cursive event loop to trigger immediate processing of events.
pub fn trigger(&self) {
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();
}
}

2
src/ipc.rs
View File

@@ -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>) {

1
src/lib.rs
View File

@@ -4,6 +4,7 @@ use librespot_playback::audio_backend;
pub const AUTHOR: &str = "Henrik Friedrichsen <henrik@affekt.org> 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

9
src/library.rs
View File

@@ -133,7 +133,7 @@ impl Library {
/// Append `updated` to the local playlists or update the local version if it exists. Return the
/// index of the appended/updated playlist.
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;
@@ -159,10 +159,7 @@ impl Library {
if let Some(position) = position {
if self.spotify.api.delete_playlist(id).is_ok() {
self.playlists
.write()
.expect("can't writelock playlists")
.remove(position);
self.playlists.write().unwrap().remove(position);
self.save_cache(
&config::cache_path(CACHE_PLAYLISTS),
&self.playlists.read().unwrap(),
@@ -581,7 +578,7 @@ impl Library {
/// If there is a local version of the playlist, update it and rewrite the cache.
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();
}

3
src/mpris.rs
View File

@@ -372,9 +372,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) = uri.parse() {
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

4
src/queue.rs
View File

@@ -423,7 +423,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.
@@ -457,7 +457,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 {

116
src/spotify.rs
View File

@@ -1,7 +1,8 @@
use std::env;
use std::error::Error;
use std::str::FromStr;
use std::sync::{Arc, RwLock};
use std::time::{Duration, SystemTime};
use std::{env, fmt};
use futures::channel::oneshot;
use librespot_core::authentication::Credentials;
@@ -56,8 +57,6 @@ pub struct Spotify {
since: Arc<RwLock<Option<SystemTime>>>,
/// Channel to send commands to the worker thread.
channel: Arc<RwLock<Option<mpsc::UnboundedSender<WorkerCommand>>>>,
/// The username of the logged in user.
user: Option<String>,
}
impl Spotify {
@@ -71,19 +70,18 @@ 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
}
@@ -92,10 +90,7 @@ impl Spotify {
/// in user.
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();
@@ -144,9 +139,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()),
@@ -243,18 +239,13 @@ 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)
}
/// Get the current playback status of the [Player].
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()
}
@@ -268,34 +259,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
}
@@ -329,10 +308,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;
}
@@ -361,7 +337,7 @@ impl Spotify {
/// Send a [WorkerCommand] to the worker thread.
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) {
@@ -407,7 +383,7 @@ impl Spotify {
/// Set the current volume of the [Player].
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));
}
@@ -434,23 +410,59 @@ pub enum UriType {
Episode,
}
impl UriType {
/// Try to create a [UriType] from the given string.
pub fn from_uri(s: &str) -> Option<Self> {
#[derive(Debug)]
pub struct UriParseError;
impl fmt::Display for UriParseError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str("invalid Spotify URI")
}
}
impl Error for UriParseError {}
impl FromStr for UriType {
type Err = UriParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
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(UriParseError)
}
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn parses_album_uri() {
let uri_type = "spotify:album:29F5MF6Q9VYlryDsYEQz6a".parse();
assert!(matches!(uri_type, Ok(UriType::Album)));
}
#[test]
fn parse_invalid_uri() {
let uri_type: Result<UriType, _> = "kayava".parse();
assert!(matches!(uri_type, Err(UriParseError)));
}
#[test]
fn parse_playlist_uri() {
let uri_type = "spotify:playlist:37i9dQZF1DX36Xw4IJIVKA".parse();
assert!(matches!(uri_type, Ok(UriType::Playlist)));
}
}

22
src/spotify_api.rs
View File

@@ -95,26 +95,18 @@ impl WebApi {
let (token_tx, token_rx) = std::sync::mpsc::channel();
let cmd = WorkerCommand::RequestToken(token_tx);
if let Some(channel) = self
.worker_channel
.read()
.expect("can't readlock worker channel")
.as_ref()
{
channel.send(cmd).expect("can't send message to worker");
if let Some(channel) = self.worker_channel.read().unwrap().as_ref() {
channel.send(cmd).unwrap();
let token_option = token_rx.recv().unwrap();
if let Some(token) = token_option {
*self.api.token.lock().expect("can't writelock api token") = Some(Token {
*self.api.token.lock().unwrap() = Some(Token {
access_token: token.access_token,
expires_in: chrono::Duration::seconds(token.expires_in.into()),
scopes: HashSet::from_iter(token.scope),
expires_at: None,
refresh_token: None,
});
*self
.token_expiration
.write()
.expect("could not writelock token") =
*self.token_expiration.write().unwrap() =
Utc::now() + ChronoDuration::seconds(token.expires_in.into());
} else {
error!("Failed to update token");
@@ -231,7 +223,7 @@ impl WebApi {
None
};
if let Some(()) = self.api_with_retry(|api| {
let replace_items = self.api_with_retry(|api| {
let playable_ids: Vec<PlayableId> = tracks
.iter()
.filter_map(|playable| playable.into())
@@ -240,7 +232,9 @@ impl WebApi {
PlaylistId::from_id(id).unwrap(),
playable_ids.iter().map(|p| p.as_ref()),
)
}) {
});
if replace_items.is_some() {
debug!("saved {} tracks to playlist {}", tracks.len(), id);
while let Some(ref mut tracks) = remainder.clone() {
// grab the next set of 100 tracks

2
src/ui/playlist.rs
View File

@@ -93,7 +93,7 @@ impl ViewExt for PlaylistView {
}
if let Command::Sort(key, direction) = cmd {
self.library.cfg.with_state_mut(|mut state| {
self.library.cfg.with_state_mut(|state| {
let order = crate::config::SortingOrder {
key: key.clone(),
direction: direction.clone(),

2
src/ui/search_results.rs
View File

@@ -394,7 +394,7 @@ impl SearchResultsView {
self.spotify.api.update_token();
// is the query a Spotify URI?
if let Some(uritype) = UriType::from_uri(&query) {
if let Ok(uritype) = query.parse() {
match uritype {
UriType::Track => {
self.perform_search(