feat(daemon): make the download folder a live setting in both modes
The full-surface audit found the settings story half-true: `config set download_dir` and the TUI's settings form edited a file a running daemon never rereads, while the attached TUI displayed the laptop's path and discarded the edit entirely. - `download.set_dir`: a session-wide move, published in the contract; the per-transfer destination stays refused. `daemon.status` reports it and the next transfer lands there. - `config set download_dir` and `config set shared_dirs` push to a live daemon the way `shares add` does, through one shared helper; the socket probe is paid only by the keys a daemon owns. - The attached TUI seeds its settings form from the daemon's status, pushes both folders over the socket as typed, reports a partial refusal as exactly that, and creates no client-side directories. It reads the daemon token from the resolved config — with the token only in config.toml it could not attach over TCP — and settings persistence honours --config instead of hard-coding the default path. - `search.wishlist` records a start time, so an attached window renders it as running rather than instantly finished. The rest is the coverage the audit demanded: CLI-level token auth (wrong, missing, and valid over TCP), `daemon status|stop` with nothing running, --sort, --limit, --pick first, --password-cmd, --log-file, --upload-slots, the config-named download folder in both modes, and headless settings-form tests attached and local. Every guard was checked by disabling what it protects. Both e2e modes run green: 94 CLI tests each way, 16 protocol, 285 unit, 34 lib.
This commit is contained in:
Michel de Graaf
@@ -58,8 +58,9 @@ So: **one daemon, then as many commands as you like.** Reach for `--no-daemon`
|
||||
only when you deliberately want an isolated session, and expect to justify it.
|
||||
|
||||
Two things change while a daemon is running: files land in the *daemon's*
|
||||
download directory (`daemon status` reports which), and `shares add` updates it
|
||||
straight away rather than at its next start.
|
||||
download directory (`daemon status` reports which), and `shares add` or
|
||||
`config set download_dir` update it straight away rather than at its next
|
||||
start.
|
||||
|
||||
## The contract
|
||||
|
||||
|
||||
@@ -152,6 +152,13 @@ pub trait SessionApi: Send + Sync {
|
||||
fn shared_counts(&self) -> (u32, u32);
|
||||
fn shared_directories(&self) -> Vec<String>;
|
||||
fn set_shared_directories(&self, directories: Vec<String>) -> Result<()>;
|
||||
|
||||
/// Point future transfers at `directory`.
|
||||
///
|
||||
/// Only a daemon holds a download folder of its own — locally every
|
||||
/// download call names its directory, so the local session has nothing to
|
||||
/// store and accepts silently.
|
||||
fn set_download_directory(&self, directory: String) -> Result<()>;
|
||||
}
|
||||
|
||||
/// The local session: the library client itself, so `Arc<Client>` is already an
|
||||
@@ -353,4 +360,8 @@ impl SessionApi for Client {
|
||||
fn set_shared_directories(&self, directories: Vec<String>) -> Result<()> {
|
||||
Self::set_shared_directories(self, directories)
|
||||
}
|
||||
|
||||
fn set_download_directory(&self, _directory: String) -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
@@ -55,29 +55,45 @@ pub struct LiveDaemon {
|
||||
pub token: Option<String>,
|
||||
}
|
||||
|
||||
/// Tell a running daemon what this machine now shares.
|
||||
/// Tell a running daemon about a change it should apply now.
|
||||
///
|
||||
/// Locally, `shares add` means "shared from the next run on", because there is
|
||||
/// no session yet. A daemon *is* that next run and it is already going, so
|
||||
/// leaving it on its startup list would make the command look like it had
|
||||
/// done nothing — the folder configured, and still invisible to the network.
|
||||
fn push_to_daemon(out: &Out, daemon: &LiveDaemon, directories: Vec<String>) {
|
||||
/// Locally a config change means "from the next run on", because there is no
|
||||
/// session yet. A daemon *is* that next run and it is already going, so
|
||||
/// leaving it on its startup settings would make the command look like it
|
||||
/// had done nothing — the folder configured, and the network none the wiser.
|
||||
fn push_to_daemon(
|
||||
out: &Out,
|
||||
daemon: &LiveDaemon,
|
||||
applied: &str,
|
||||
kept: &str,
|
||||
apply: impl FnOnce(&RemoteSession) -> soulseek_rs::Result<()>,
|
||||
) {
|
||||
let Some(endpoint) = daemon.endpoint.as_ref() else {
|
||||
return;
|
||||
};
|
||||
let token = daemon.token.clone().or_else(crate::daemon::stored_token);
|
||||
let outcome = RemoteSession::connect(endpoint, token)
|
||||
.and_then(|session| session.set_shared_directories(directories));
|
||||
.and_then(|session| apply(&session));
|
||||
match outcome {
|
||||
Ok(()) => out.status("the running daemon is re-indexing"),
|
||||
Ok(()) => out.status(applied),
|
||||
// Not fatal: the config file is written either way, so the change
|
||||
// still takes effect when the daemon next starts.
|
||||
Err(e) => out.warn(&format!(
|
||||
"the daemon kept its own shares ({e}); restart it to pick this up"
|
||||
"the daemon kept {kept} ({e}); restart it to pick this up"
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
||||
fn push_shares(out: &Out, daemon: &LiveDaemon, directories: Vec<String>) {
|
||||
push_to_daemon(
|
||||
out,
|
||||
daemon,
|
||||
"the running daemon is re-indexing",
|
||||
"its own shares",
|
||||
move |session| session.set_shared_directories(directories),
|
||||
);
|
||||
}
|
||||
|
||||
pub fn shares_add(
|
||||
out: &Out,
|
||||
store: &Store,
|
||||
@@ -104,7 +120,7 @@ pub fn shares_add(
|
||||
updated.shared_dir = None;
|
||||
store.save(&updated)?;
|
||||
out.status(&format!("sharing {directory}"));
|
||||
push_to_daemon(out, daemon, dirs);
|
||||
push_shares(out, daemon, dirs);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -128,7 +144,7 @@ pub fn shares_remove(
|
||||
updated.shared_dir = None;
|
||||
store.save(&updated)?;
|
||||
out.status(&format!("no longer sharing {directory}"));
|
||||
push_to_daemon(out, daemon, dirs);
|
||||
push_shares(out, daemon, dirs);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -228,14 +244,36 @@ pub fn config_set(
|
||||
store: &Store,
|
||||
key: &str,
|
||||
value: &str,
|
||||
daemon: impl FnOnce() -> LiveDaemon,
|
||||
) -> CliResult {
|
||||
let mut updated = store.config.clone();
|
||||
updated.set(key, value).map_err(CliError::usage)?;
|
||||
store.save(&updated)?;
|
||||
let current = updated.get(key);
|
||||
out.emit(&ConfigRecord {
|
||||
key: key.to_string(),
|
||||
value: updated.get(key),
|
||||
value: current.clone(),
|
||||
});
|
||||
// A running daemon holds the live settings, so for the keys it owns a
|
||||
// config write alone would look like it did nothing until its next
|
||||
// restart — the same trap `shares add` closes by pushing to the session.
|
||||
// `daemon` is resolved lazily: finding one means connecting to a socket,
|
||||
// which the other keys should not pay for.
|
||||
match key {
|
||||
"download_dir" => {
|
||||
if let Some(directory) = current {
|
||||
push_to_daemon(
|
||||
out,
|
||||
&daemon(),
|
||||
"the running daemon will download there now",
|
||||
"its own download folder",
|
||||
move |session| session.set_download_directory(directory),
|
||||
);
|
||||
}
|
||||
}
|
||||
"shared_dirs" => push_shares(out, &daemon(), share_list(&updated)),
|
||||
_ => {}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -221,7 +221,7 @@ pub fn run(
|
||||
hub: Arc::clone(&hub),
|
||||
browses: Arc::clone(&browses),
|
||||
downloads: Arc::clone(&downloads),
|
||||
download_dir: ctx.download_dir.clone(),
|
||||
download_dir: Mutex::new(ctx.download_dir.clone()),
|
||||
server: ctx.settings.server_address.to_string(),
|
||||
token: token.clone(),
|
||||
started: Instant::now(),
|
||||
|
||||
@@ -10,11 +10,11 @@
|
||||
|
||||
use super::proto::{
|
||||
Ack, AuthParams, AuthResult, BrowseEvent, DaemonStatus, DirectoriesParams,
|
||||
DownloadStartParams, DownloadStarted, DownloadStatusEvent, Downloads,
|
||||
Event, IntervalSeconds, Members, MessageParams, Messages, Method,
|
||||
QueryParams, RoomEventDto, RoomRef, SayParams, SearchResults, Searches,
|
||||
Seconds, SessionLossEvent, SharesStatus, SlotsParams, TransferRef,
|
||||
UploadInfoDto, Uploads, UserMessageDto, UserRef, UserResult,
|
||||
DirectoryParams, DownloadStartParams, DownloadStarted, DownloadStatusEvent,
|
||||
Downloads, Event, IntervalSeconds, Members, MessageParams, Messages,
|
||||
Method, QueryParams, RoomEventDto, RoomRef, SayParams, SearchResults,
|
||||
Searches, Seconds, SessionLossEvent, SharesStatus, SlotsParams,
|
||||
TransferRef, UploadInfoDto, Uploads, UserMessageDto, UserRef, UserResult,
|
||||
};
|
||||
use schemars::{JsonSchema, SchemaGenerator, generate::SchemaSettings};
|
||||
use serde_json::{Value, json};
|
||||
@@ -49,6 +49,7 @@ fn params_schema(method: Method, generator: &mut SchemaGenerator) -> Value {
|
||||
| Method::DownloadRemove
|
||||
| Method::DownloadRemoveQueued
|
||||
| Method::UploadCancel => schema_of::<TransferRef>(generator),
|
||||
Method::DownloadSetDir => schema_of::<DirectoryParams>(generator),
|
||||
Method::UploadSlots => schema_of::<SlotsParams>(generator),
|
||||
Method::RoomJoin | Method::RoomLeave | Method::RoomMembers => {
|
||||
schema_of::<RoomRef>(generator)
|
||||
@@ -104,6 +105,7 @@ fn result_schema(method: Method, generator: &mut SchemaGenerator) -> Value {
|
||||
| Method::DownloadResume
|
||||
| Method::DownloadRemove
|
||||
| Method::DownloadRemoveQueued
|
||||
| Method::DownloadSetDir
|
||||
| Method::UploadCancel
|
||||
| Method::UploadSlots
|
||||
| Method::PrivilegesCheck
|
||||
@@ -155,6 +157,9 @@ fn summary(method: Method) -> &'static str {
|
||||
Method::DownloadResume => "Resume a paused transfer.",
|
||||
Method::DownloadRemove => "Forget a transfer entirely.",
|
||||
Method::DownloadRemoveQueued => "Drop a transfer that has not started.",
|
||||
Method::DownloadSetDir => {
|
||||
"Land future transfers in a different folder on the daemon's host."
|
||||
}
|
||||
Method::UploadList => "Uploads being served, active first.",
|
||||
Method::UploadCancel => "Stop serving an upload in progress.",
|
||||
Method::UploadSlots => "How many uploads may run at once.",
|
||||
|
||||
@@ -81,6 +81,14 @@ pub struct DirectoriesParams {
|
||||
pub directories: Vec<String>,
|
||||
}
|
||||
|
||||
/// Where the daemon should land transfers from now on, on *its* filesystem.
|
||||
/// The same trust as `shares.set`: an authenticated caller is the owner.
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)]
|
||||
#[cfg_attr(test, derive(schemars::JsonSchema))]
|
||||
pub struct DirectoryParams {
|
||||
pub directory: String,
|
||||
}
|
||||
|
||||
/// Every search this session has run, newest last. Shared by all clients, so
|
||||
/// a second one attaching sees what the first has been looking for.
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)]
|
||||
|
||||
@@ -90,6 +90,7 @@ wire_vocabulary! {
|
||||
DownloadResume => "download.resume",
|
||||
DownloadRemove => "download.remove",
|
||||
DownloadRemoveQueued => "download.remove_queued",
|
||||
DownloadSetDir => "download.set_dir",
|
||||
UploadList => "upload.list",
|
||||
UploadCancel => "upload.cancel",
|
||||
UploadSlots => "upload.slots",
|
||||
|
||||
@@ -8,12 +8,12 @@
|
||||
use super::hub::{Hub, PendingBrowses, PendingDownload};
|
||||
use super::proto::{
|
||||
Ack, AuthResult, CODE_APPLICATION, CODE_INVALID_PARAMS, ChatMessageDto,
|
||||
DaemonStatus, DirectoriesParams, DownloadDto, DownloadStartParams,
|
||||
DownloadStarted, Downloads, IntervalSeconds, Members, MessageParams,
|
||||
Messages, Method, OPENRPC, PROTOCOL_VERSION, QueryParams, RoomRef,
|
||||
RpcError, SayParams, SearchResultDto, SearchResults, SearchSummary,
|
||||
Searches, Seconds, SharesStatus, SlotsParams, TransferRef, UploadInfoDto,
|
||||
Uploads, UserInfoDto, UserRef, UserResult,
|
||||
DaemonStatus, DirectoriesParams, DirectoryParams, DownloadDto,
|
||||
DownloadStartParams, DownloadStarted, Downloads, IntervalSeconds, Members,
|
||||
MessageParams, Messages, Method, OPENRPC, PROTOCOL_VERSION, QueryParams,
|
||||
RoomRef, RpcError, SayParams, SearchResultDto, SearchResults,
|
||||
SearchSummary, Searches, Seconds, SharesStatus, SlotsParams, TransferRef,
|
||||
UploadInfoDto, Uploads, UserInfoDto, UserRef, UserResult,
|
||||
};
|
||||
use crate::api::SessionApi;
|
||||
use crate::output::Exit;
|
||||
@@ -29,9 +29,9 @@ pub struct Daemon {
|
||||
pub hub: Arc<Hub>,
|
||||
pub browses: Arc<PendingBrowses>,
|
||||
pub downloads: Arc<Mutex<Vec<PendingDownload>>>,
|
||||
/// Where transfers land, on *this* machine. A remote caller cannot see
|
||||
/// this filesystem, so it does not get to choose.
|
||||
pub download_dir: String,
|
||||
/// Where transfers land, on *this* machine. A caller may point it at a
|
||||
/// different folder with `download.set_dir`, but never per download.
|
||||
pub download_dir: Mutex<String>,
|
||||
pub server: String,
|
||||
pub token: String,
|
||||
pub started: Instant,
|
||||
@@ -126,6 +126,9 @@ impl Daemon {
|
||||
self.session.start_wishlist_search(&query.query).map_err(
|
||||
|e| RpcError::application(Exit::Connection, e.to_string()),
|
||||
)?;
|
||||
// Without a start time every other window would render this
|
||||
// search as finished the moment it goes out.
|
||||
self.record_search_start(&query.query);
|
||||
ok(Ack::OK)
|
||||
}
|
||||
Method::SearchWishlistInterval => ok(IntervalSeconds {
|
||||
@@ -133,6 +136,11 @@ impl Daemon {
|
||||
}),
|
||||
|
||||
Method::DownloadStart => self.start_download(parse(params)?),
|
||||
Method::DownloadSetDir => {
|
||||
let dir: DirectoryParams = parse(params)?;
|
||||
self.set_download_dir(dir.directory)?;
|
||||
ok(Ack::OK)
|
||||
}
|
||||
Method::DownloadList => ok(Downloads {
|
||||
downloads: self
|
||||
.session
|
||||
@@ -344,10 +352,10 @@ impl Daemon {
|
||||
params: DownloadStartParams,
|
||||
) -> Result<Value, RpcError> {
|
||||
// The daemon's own directory, always. Letting a caller name a path
|
||||
// would hand any authenticated client an arbitrary write on this
|
||||
// host's filesystem, and a remote one cannot see that filesystem to
|
||||
// choose sensibly anyway.
|
||||
let directory = self.download_dir.clone();
|
||||
// per download would scatter transfers in ways `daemon.status` cannot
|
||||
// report; changing the folder is a deliberate session-wide act,
|
||||
// `download.set_dir`.
|
||||
let directory = self.download_dir();
|
||||
let (download, updates) = self
|
||||
.session
|
||||
.download_with_metadata(
|
||||
@@ -376,6 +384,35 @@ impl Daemon {
|
||||
})
|
||||
}
|
||||
|
||||
fn download_dir(&self) -> String {
|
||||
self.download_dir
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.clone()
|
||||
}
|
||||
|
||||
/// Point future transfers at `directory`, creating it on *this* host.
|
||||
/// The same trust boundary as `shares.set`: authenticated means owner.
|
||||
fn set_download_dir(&self, directory: String) -> Result<(), RpcError> {
|
||||
if directory.trim().is_empty() {
|
||||
return Err(RpcError::new(
|
||||
CODE_INVALID_PARAMS,
|
||||
"the download directory cannot be empty".to_string(),
|
||||
));
|
||||
}
|
||||
std::fs::create_dir_all(&directory).map_err(|e| {
|
||||
RpcError::application(
|
||||
Exit::Usage,
|
||||
format!("cannot create {directory}: {e}"),
|
||||
)
|
||||
})?;
|
||||
*self
|
||||
.download_dir
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner) = directory;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn shares(&self) -> SharesStatus {
|
||||
let (folders, files) = self.session.shared_counts();
|
||||
SharesStatus {
|
||||
@@ -427,7 +464,7 @@ impl Daemon {
|
||||
listen_port: self.session.listen_port(),
|
||||
shared_folders: folders,
|
||||
shared_files: files,
|
||||
download_dir: self.download_dir.clone(),
|
||||
download_dir: self.download_dir(),
|
||||
session_loss: self.session.session_loss().map(Into::into),
|
||||
clients: self.hub.subscribers(),
|
||||
uptime_secs: self.started.elapsed().as_secs(),
|
||||
@@ -577,7 +614,7 @@ mod tests {
|
||||
hub: Arc::new(Hub::new()),
|
||||
browses: Arc::new(PendingBrowses::default()),
|
||||
downloads: Arc::new(Mutex::new(Vec::new())),
|
||||
download_dir: "/tmp".to_string(),
|
||||
download_dir: Mutex::new("/tmp".to_string()),
|
||||
server: "server.invalid:2416".to_string(),
|
||||
token: token.to_string(),
|
||||
started: Instant::now(),
|
||||
@@ -771,5 +808,11 @@ mod tests {
|
||||
) -> soulseek_rs::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
fn set_download_directory(
|
||||
&self,
|
||||
_directory: String,
|
||||
) -> soulseek_rs::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -10,11 +10,12 @@ mod mirror;
|
||||
use crate::api::{SessionApi, SessionSearch};
|
||||
use crate::daemon::proto::{
|
||||
Ack, AuthParams, AuthResult, CODE_APPLICATION, ChatMessageDto,
|
||||
DaemonStatus, DirectoriesParams, DownloadDto, DownloadStartParams,
|
||||
DownloadStarted, Downloads, Event, IntervalSeconds, Members, MessageParams,
|
||||
Messages, Method, PROTOCOL_VERSION, QueryParams, Request, Response,
|
||||
RoomRef, RpcError, SayParams, SearchResults, Searches, Seconds,
|
||||
SharesStatus, SlotsParams, TransferRef, Uploads, UserRef, UserResult,
|
||||
DaemonStatus, DirectoriesParams, DirectoryParams, DownloadDto,
|
||||
DownloadStartParams, DownloadStarted, Downloads, Event, IntervalSeconds,
|
||||
Members, MessageParams, Messages, Method, PROTOCOL_VERSION, QueryParams,
|
||||
Request, Response, RoomRef, RpcError, SayParams, SearchResults, Searches,
|
||||
Seconds, SharesStatus, SlotsParams, TransferRef, Uploads, UserRef,
|
||||
UserResult,
|
||||
};
|
||||
use mirror::Mirror;
|
||||
use serde::de::DeserializeOwned;
|
||||
@@ -781,6 +782,10 @@ impl SessionApi for RemoteSession {
|
||||
fn set_shared_directories(&self, directories: Vec<String>) -> Result<()> {
|
||||
self.tell(Method::SharesSet, DirectoriesParams { directories })
|
||||
}
|
||||
|
||||
fn set_download_directory(&self, directory: String) -> Result<()> {
|
||||
self.tell(Method::DownloadSetDir, DirectoryParams { directory })
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
+37
-9
@@ -10,7 +10,7 @@ use crate::output::{CliError, CliResult, Exit, Out};
|
||||
use crate::ui::launch_main_tui;
|
||||
use soulseek_rs::{ClientSettings, ClientVersion, PeerAddress};
|
||||
use std::io::{BufRead, IsTerminal};
|
||||
use std::path::PathBuf;
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
use std::{env, io};
|
||||
@@ -38,7 +38,7 @@ pub fn run(mut cli: Cli, out: &Out) -> CliResult {
|
||||
// rather than logging in a second time, which the server would answer
|
||||
// by cutting the daemon off.
|
||||
if let Some(endpoint) = daemon_endpoint(&cli, &resolved) {
|
||||
return run_attached_tui(&cli, &resolved, &endpoint);
|
||||
return run_attached_tui(&resolved, &endpoint, config_path);
|
||||
}
|
||||
return run_default_tui(&cli, &resolved, config_path, &file_config);
|
||||
};
|
||||
@@ -72,7 +72,11 @@ pub fn run(mut cli: Cli, out: &Out) -> CliResult {
|
||||
}
|
||||
Commands::Config(ConfigCommand::Set { ref key, ref value }) => {
|
||||
return crate::commands::settings::config_set(
|
||||
out, &store, key, value,
|
||||
out,
|
||||
&store,
|
||||
key,
|
||||
value,
|
||||
|| live_daemon(&cli, &resolved),
|
||||
);
|
||||
}
|
||||
Commands::Shares(SharesCommand::List) => {
|
||||
@@ -442,7 +446,12 @@ fn run_default_tui(
|
||||
}
|
||||
};
|
||||
|
||||
persist_credentials(&outcome, config_path, file_config, &secret_store);
|
||||
persist_credentials(
|
||||
&outcome,
|
||||
config_path.as_deref(),
|
||||
file_config,
|
||||
&secret_store,
|
||||
);
|
||||
|
||||
// Best-effort: make ourselves reachable behind a home router so firewalled
|
||||
// peers can connect back. Mapped only once the listener is up, and for the
|
||||
@@ -462,6 +471,7 @@ fn run_default_tui(
|
||||
resolved.max_concurrent_downloads,
|
||||
Duration::from_secs(resolved.search_timeout),
|
||||
store,
|
||||
config_path,
|
||||
)
|
||||
.map_err(|e| CliError::new(Exit::Failure, e.to_string()))
|
||||
}
|
||||
@@ -472,9 +482,9 @@ fn run_default_tui(
|
||||
/// ask for and nothing to store. Everything the UI shows comes back over the
|
||||
/// socket, and what it persists stays daemon-side.
|
||||
fn run_attached_tui(
|
||||
cli: &Cli,
|
||||
resolved: &crate::persist::config::Resolved,
|
||||
endpoint: &crate::remote::Endpoint,
|
||||
config_path: Option<PathBuf>,
|
||||
) -> CliResult {
|
||||
use ratatui::crossterm::{
|
||||
event::EnableMouseCapture,
|
||||
@@ -482,7 +492,9 @@ fn run_attached_tui(
|
||||
terminal::{Clear, ClearType},
|
||||
};
|
||||
|
||||
let token = cli
|
||||
// The resolved token, not the flag alone: `daemon_token` in config.toml
|
||||
// is the documented remote setup, and the flag/env are layered into it.
|
||||
let token = resolved
|
||||
.daemon_token
|
||||
.clone()
|
||||
.or_else(crate::daemon::stored_token);
|
||||
@@ -493,6 +505,21 @@ fn run_attached_tui(
|
||||
))
|
||||
})?;
|
||||
|
||||
// The settings form shows and edits the *daemon's* download folder — the
|
||||
// one transfers actually land in — not this machine's configured default.
|
||||
// The connect above just succeeded, so a daemon that cannot answer this
|
||||
// is a real problem to report, not one to paper over with a local path.
|
||||
let download_dir = session
|
||||
.ask_for::<crate::daemon::proto::DaemonStatus>(
|
||||
crate::daemon::proto::Method::DaemonStatus,
|
||||
)
|
||||
.map(|status| status.download_dir)
|
||||
.map_err(|e| {
|
||||
CliError::connection(format!(
|
||||
"the daemon at {endpoint} did not report its status: {e}"
|
||||
))
|
||||
})?;
|
||||
|
||||
soulseek_rs::utils::logger::enable_buffering();
|
||||
let _ =
|
||||
execute!(std::io::stdout(), Clear(ClearType::All), EnableMouseCapture);
|
||||
@@ -503,10 +530,11 @@ fn run_attached_tui(
|
||||
launch_main_tui(
|
||||
terminal,
|
||||
Arc::new(session),
|
||||
resolved.download_dir.clone(),
|
||||
download_dir,
|
||||
resolved.max_concurrent_downloads,
|
||||
Duration::from_secs(resolved.search_timeout),
|
||||
None,
|
||||
config_path,
|
||||
)
|
||||
.map_err(|e| CliError::new(Exit::Failure, e.to_string()))
|
||||
}
|
||||
@@ -516,7 +544,7 @@ fn run_attached_tui(
|
||||
/// best-effort: failing to persist must not break a working session.
|
||||
fn persist_credentials(
|
||||
outcome: &crate::ui::login::LoginOutcome,
|
||||
config_path: Option<PathBuf>,
|
||||
config_path: Option<&Path>,
|
||||
file_config: &crate::persist::config::FileConfig,
|
||||
secret_store: &dyn crate::persist::secret::SecretStore,
|
||||
) {
|
||||
@@ -525,7 +553,7 @@ fn persist_credentials(
|
||||
{
|
||||
let mut updated = file_config.clone();
|
||||
updated.username = Some(outcome.username.clone());
|
||||
if let Err(e) = updated.save(&path) {
|
||||
if let Err(e) = updated.save(path) {
|
||||
eprintln!("⚠️ Could not save config: {e}");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -28,6 +28,9 @@ pub struct MainTui {
|
||||
search_timeout: Duration,
|
||||
spinner_state: usize,
|
||||
store: Option<StateStore>,
|
||||
/// Where settings changes are persisted — the config file this run
|
||||
/// resolved, so `--config` is honoured.
|
||||
config_path: Option<std::path::PathBuf>,
|
||||
/// Last snapshot written to disk, to skip no-op saves.
|
||||
saved_snapshot: Snapshot,
|
||||
}
|
||||
@@ -39,6 +42,7 @@ impl MainTui {
|
||||
max_concurrent_downloads: usize,
|
||||
search_timeout: Duration,
|
||||
store: Option<StateStore>,
|
||||
config_path: Option<std::path::PathBuf>,
|
||||
) -> Self {
|
||||
let mut tui = Self {
|
||||
client,
|
||||
@@ -48,6 +52,7 @@ impl MainTui {
|
||||
search_timeout,
|
||||
spinner_state: 0,
|
||||
store,
|
||||
config_path,
|
||||
saved_snapshot: Snapshot::default(),
|
||||
};
|
||||
tui.restore_persisted_state();
|
||||
@@ -254,6 +259,7 @@ pub fn launch_main_tui(
|
||||
max_concurrent_downloads: usize,
|
||||
search_timeout: Duration,
|
||||
store: Option<StateStore>,
|
||||
config_path: Option<std::path::PathBuf>,
|
||||
) -> Result<()> {
|
||||
let tui = MainTui::new(
|
||||
client,
|
||||
@@ -261,6 +267,7 @@ pub fn launch_main_tui(
|
||||
max_concurrent_downloads,
|
||||
search_timeout,
|
||||
store,
|
||||
config_path,
|
||||
);
|
||||
tui.run(terminal)
|
||||
}
|
||||
@@ -280,6 +287,10 @@ mod tests {
|
||||
searches: std::sync::Mutex<Vec<crate::api::SessionSearch>>,
|
||||
/// Whether this session is a daemon's. Local sessions do not sync.
|
||||
shared: bool,
|
||||
/// What `set_download_directory` was last asked for, if anything.
|
||||
download_dir_set: std::sync::Mutex<Option<String>>,
|
||||
/// What `set_shared_directories` was last asked for, if anything.
|
||||
shares_set: std::sync::Mutex<Option<Vec<String>>>,
|
||||
}
|
||||
|
||||
fn queued(username: &str, filename: &str) -> soulseek_rs::types::Download {
|
||||
@@ -468,8 +479,17 @@ mod tests {
|
||||
}
|
||||
fn set_shared_directories(
|
||||
&self,
|
||||
_directories: Vec<String>,
|
||||
directories: Vec<String>,
|
||||
) -> soulseek_rs::Result<()> {
|
||||
*self.shares_set.lock().expect("not poisoned") = Some(directories);
|
||||
Ok(())
|
||||
}
|
||||
fn set_download_directory(
|
||||
&self,
|
||||
directory: String,
|
||||
) -> soulseek_rs::Result<()> {
|
||||
*self.download_dir_set.lock().expect("not poisoned") =
|
||||
Some(directory);
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
@@ -509,6 +529,7 @@ mod tests {
|
||||
1,
|
||||
Duration::from_secs(1),
|
||||
None,
|
||||
None,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -710,4 +731,109 @@ mod tests {
|
||||
// record, so nothing is invented.
|
||||
assert!(tui(Vec::new()).state.messages.is_empty());
|
||||
}
|
||||
|
||||
/// Attached, both folders in the settings form name paths on the
|
||||
/// *daemon's* machine: they go over the socket exactly as typed, and
|
||||
/// nothing is created or validated against this machine's filesystem.
|
||||
#[test]
|
||||
fn applying_settings_attached_pushes_both_folders_to_the_daemon() {
|
||||
let session = Arc::new(TalkativeSession {
|
||||
shared: true,
|
||||
..TalkativeSession::default()
|
||||
});
|
||||
let scratch = tempfile::tempdir().expect("a temp dir");
|
||||
let config_path = scratch.path().join("config.toml");
|
||||
let mut tui = MainTui::new(
|
||||
session.clone(),
|
||||
"/daemon/old".to_string(),
|
||||
1,
|
||||
Duration::from_secs(1),
|
||||
None,
|
||||
Some(config_path.clone()),
|
||||
);
|
||||
|
||||
let daemon_only = scratch.path().join("only-on-the-daemon");
|
||||
let daemon_dir = daemon_only.to_str().expect("utf-8").to_string();
|
||||
tui.open_settings();
|
||||
{
|
||||
let settings = tui.state.settings.as_mut().expect("open");
|
||||
settings.download_dir.clone_from(&daemon_dir);
|
||||
settings.share_dirs = vec!["/not-here-either".to_string()];
|
||||
}
|
||||
tui.apply_settings();
|
||||
|
||||
assert_eq!(
|
||||
*session.download_dir_set.lock().expect("not poisoned"),
|
||||
Some(daemon_dir.clone()),
|
||||
"the folder is pushed to the daemon"
|
||||
);
|
||||
assert_eq!(
|
||||
*session.shares_set.lock().expect("not poisoned"),
|
||||
Some(vec!["/not-here-either".to_string()]),
|
||||
"shares go over as typed — the daemon's filesystem judges them"
|
||||
);
|
||||
assert!(
|
||||
!daemon_only.exists(),
|
||||
"no directory is created on this machine for a daemon's path"
|
||||
);
|
||||
assert_eq!(tui.download_dir, daemon_dir);
|
||||
|
||||
let saved = crate::persist::config::FileConfig::load(&config_path)
|
||||
.expect("config readable");
|
||||
assert_eq!(
|
||||
saved.download_dir.as_deref(),
|
||||
Some(daemon_dir.as_str()),
|
||||
"the change still lands in config.toml for the next restart"
|
||||
);
|
||||
}
|
||||
|
||||
/// Locally the same form works on this machine: the folder is created
|
||||
/// here and share paths that do not exist here are dropped before the
|
||||
/// session hears about them.
|
||||
#[test]
|
||||
fn applying_settings_locally_stays_on_this_machine() {
|
||||
let session = Arc::new(TalkativeSession {
|
||||
shared: false,
|
||||
..TalkativeSession::default()
|
||||
});
|
||||
let scratch = tempfile::tempdir().expect("a temp dir");
|
||||
let config_path = scratch.path().join("config.toml");
|
||||
let share = scratch.path().join("music");
|
||||
std::fs::create_dir_all(&share).expect("share dir");
|
||||
let mut tui = MainTui::new(
|
||||
session.clone(),
|
||||
"/tmp".to_string(),
|
||||
1,
|
||||
Duration::from_secs(1),
|
||||
None,
|
||||
Some(config_path),
|
||||
);
|
||||
|
||||
let downloads = scratch.path().join("downloads");
|
||||
let downloads_dir = downloads.to_str().expect("utf-8").to_string();
|
||||
tui.open_settings();
|
||||
{
|
||||
let settings = tui.state.settings.as_mut().expect("open");
|
||||
settings.download_dir.clone_from(&downloads_dir);
|
||||
settings.share_dirs = vec![
|
||||
share.to_str().expect("utf-8").to_string(),
|
||||
"/definitely-not-a-real-share".to_string(),
|
||||
];
|
||||
}
|
||||
tui.apply_settings();
|
||||
|
||||
// The seam is told in both modes; a local session accepts and takes
|
||||
// the directory per download instead of storing it.
|
||||
assert_eq!(
|
||||
*session.download_dir_set.lock().expect("not poisoned"),
|
||||
Some(downloads_dir.clone())
|
||||
);
|
||||
assert_eq!(
|
||||
*session.shares_set.lock().expect("not poisoned"),
|
||||
Some(vec![share.to_str().expect("utf-8").to_string()]),
|
||||
"paths missing on this machine are dropped before applying"
|
||||
);
|
||||
assert!(downloads.is_dir(), "the folder is created here");
|
||||
assert_eq!(tui.download_dir, downloads_dir);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
//! Settings popup behavior: open, key handling, and applying changes
|
||||
//! (persist to config.toml + live share rescan on the client).
|
||||
//! Settings popup behavior: open, key handling, and applying changes —
|
||||
//! persisted to config.toml, applied live to the session (pushed over the
|
||||
//! socket when the session is a daemon's).
|
||||
|
||||
use super::MainTui;
|
||||
use crate::models::{SettingsAction, SettingsState};
|
||||
@@ -25,38 +26,68 @@ impl MainTui {
|
||||
}
|
||||
}
|
||||
|
||||
/// Persist the edited settings and apply the share set live.
|
||||
fn apply_settings(&mut self) {
|
||||
/// Persist the edited settings and apply them live.
|
||||
///
|
||||
/// Attached to a daemon, both folders name paths on the *daemon's*
|
||||
/// machine: they are pushed over the socket and validated there, and no
|
||||
/// directory is created on this one. Locally they are this machine's and
|
||||
/// are validated here.
|
||||
pub(super) fn apply_settings(&mut self) {
|
||||
let Some(settings) = self.state.settings.as_ref() else {
|
||||
return;
|
||||
};
|
||||
let download_dir = settings.download_dir.clone();
|
||||
let share_dirs = settings.share_dirs.clone();
|
||||
|
||||
if let Err(e) = std::fs::create_dir_all(&download_dir) {
|
||||
self.set_settings_status(format!(
|
||||
"Cannot create {download_dir}: {e}"
|
||||
));
|
||||
return;
|
||||
}
|
||||
self.download_dir.clone_from(&download_dir);
|
||||
|
||||
// Validate the share paths (tilde-expand, must exist) and apply.
|
||||
let valid = crate::directories::resolve_shared_directories(&share_dirs);
|
||||
let dropped = share_dirs.len() - valid.len();
|
||||
let mut status = match self.client.set_shared_directories(valid) {
|
||||
Ok(()) if dropped > 0 => {
|
||||
format!("Applied ({dropped} invalid path(s) ignored)")
|
||||
let shares = if self.client.daemon_endpoint().is_some() {
|
||||
share_dirs.clone()
|
||||
} else {
|
||||
if let Err(e) = std::fs::create_dir_all(&download_dir) {
|
||||
self.set_settings_status(format!(
|
||||
"Cannot create {download_dir}: {e}"
|
||||
));
|
||||
return;
|
||||
}
|
||||
Ok(()) => format!("Applied · sharing {}", self.share_counts()),
|
||||
Err(e) => format!("Could not apply shares: {e}"),
|
||||
// Validate the share paths (tilde-expand, must exist).
|
||||
crate::directories::resolve_shared_directories(&share_dirs)
|
||||
};
|
||||
let dropped = share_dirs.len() - shares.len();
|
||||
|
||||
// Persist to config.toml so the change survives a restart.
|
||||
if let Some(path) = crate::persist::paths::config_file() {
|
||||
// Two independent applies: a failure of one must not misreport or
|
||||
// silently discard the other, so both run and both are told.
|
||||
let mut refused = Vec::new();
|
||||
let dir_applied =
|
||||
match self.client.set_download_directory(download_dir.clone()) {
|
||||
Ok(()) => true,
|
||||
Err(e) => {
|
||||
refused.push(format!("download folder: {e}"));
|
||||
false
|
||||
}
|
||||
};
|
||||
if let Err(e) = self.client.set_shared_directories(shares) {
|
||||
refused.push(format!("shares: {e}"));
|
||||
}
|
||||
|
||||
let mut status = if !refused.is_empty() {
|
||||
format!("Could not apply {}", refused.join("; "))
|
||||
} else if dropped > 0 {
|
||||
format!("Applied ({dropped} invalid path(s) ignored)")
|
||||
} else {
|
||||
format!("Applied · sharing {}", self.share_counts())
|
||||
};
|
||||
if dir_applied {
|
||||
self.download_dir.clone_from(&download_dir);
|
||||
}
|
||||
|
||||
// Persist to config.toml so the change survives a restart. What was
|
||||
// refused live is not written either: the file must not promise a
|
||||
// folder the session never accepted.
|
||||
if let Some(path) = self.config_path.clone() {
|
||||
let result = crate::persist::config::FileConfig::load(&path)
|
||||
.and_then(|mut config| {
|
||||
config.download_dir = Some(download_dir);
|
||||
if dir_applied {
|
||||
config.download_dir = Some(download_dir);
|
||||
}
|
||||
// The singular `shared_dir` is merged with `shared_dirs` on
|
||||
// load, so leaving it would resurrect a removed folder.
|
||||
config.shared_dir = None;
|
||||
|
||||
Reference in New Issue
Block a user