fix(tui): apply username folders everywhere and resume in place
Move the sanitizer next to the directory helpers so the CLI download command and the daemon honour use_username_dirs too, not only the TUI. Resumed downloads keep the folder they were saved with instead of nesting the username a second time, and toggling the setting applies to the next download without a restart. Document the config key.
This commit is contained in:
@@ -606,6 +606,7 @@ file, in that order of precedence. Flags work before or after the subcommand.
|
||||
| `--listener-port` | `SOULSEEK_LISTENER_PORT` | `listener_port` | `2234` |
|
||||
| `--no-listener` / `--listener` | `SOULSEEK_NO_LISTENER` | `disable_listener` | listener on |
|
||||
| — | — | `accept_children` | off |
|
||||
| — | — | `use_username_dirs` | off |
|
||||
| `--max-concurrent-downloads` | `SOULSEEK_MAX_CONCURRENT_DOWNLOADS` | `max_concurrent_downloads` | `20` |
|
||||
| `--search-timeout` | `SOULSEEK_SEARCH_TIMEOUT` | `search_timeout` | `10` |
|
||||
| `--daemon ADDR` | `SOULSEEK_DAEMON` | `daemon` | local socket if one is up |
|
||||
@@ -627,10 +628,10 @@ to them. It is off by default — each child costs a socket and a copy of the
|
||||
network's whole search stream — and it needs the listener, since a child has to
|
||||
be able to dial in.
|
||||
|
||||
`config get` and `config set` cover the twelve settings the file holds
|
||||
`config get` and `config set` cover the thirteen settings the file holds
|
||||
(`username`, `server`, `listener_port`, `disable_listener`, `accept_children`,
|
||||
`download_dir`, `shared_dirs`, `max_concurrent_downloads`, `search_timeout`,
|
||||
`password_cmd`, `daemon`, `daemon_token`), and list them back at you when you
|
||||
`download_dir`, `use_username_dirs`, `shared_dirs`, `max_concurrent_downloads`,
|
||||
`search_timeout`, `password_cmd`, `daemon`, `daemon_token`), and list them back at you when you
|
||||
name something
|
||||
else. An empty string clears a key, and `shared_dirs` takes a comma-separated
|
||||
list. Waits expressed in seconds (`--search-timeout`, `--timeout`,
|
||||
|
||||
@@ -34,6 +34,8 @@ pub struct Ctx {
|
||||
pub out: Out,
|
||||
pub settings: ClientSettings,
|
||||
pub download_dir: String,
|
||||
/// Save each peer's files under `download_dir/<username>`.
|
||||
pub use_username_dirs: bool,
|
||||
pub max_concurrent_downloads: usize,
|
||||
pub search_timeout: Duration,
|
||||
/// Where to find a daemon, when this run should use one rather than open
|
||||
@@ -47,6 +49,16 @@ pub struct Ctx {
|
||||
}
|
||||
|
||||
impl Ctx {
|
||||
/// Where a download from `username` lands.
|
||||
#[must_use]
|
||||
pub fn dir_for(&self, username: &str) -> String {
|
||||
crate::directories::download_dir_for(
|
||||
&self.download_dir,
|
||||
username,
|
||||
self.use_username_dirs,
|
||||
)
|
||||
}
|
||||
|
||||
/// Re-read a "found nothing", "timed out" or "transfer failed" verdict
|
||||
/// against the session that produced it.
|
||||
///
|
||||
|
||||
@@ -458,7 +458,7 @@ fn download_all(
|
||||
client.as_ref(),
|
||||
&ctx.out,
|
||||
&request,
|
||||
&ctx.download_dir,
|
||||
&ctx.dir_for(&request.user),
|
||||
timeout,
|
||||
);
|
||||
if sender.send((request, outcome)).is_err() {
|
||||
|
||||
@@ -202,6 +202,7 @@ pub fn run(
|
||||
browses: Arc::clone(&browses),
|
||||
downloads: Arc::clone(&downloads),
|
||||
download_dir: Mutex::new(ctx.download_dir.clone()),
|
||||
use_username_dirs: ctx.use_username_dirs,
|
||||
server: ctx.settings.server_address.to_string(),
|
||||
token: token.clone(),
|
||||
started: Instant::now(),
|
||||
|
||||
@@ -38,6 +38,7 @@ pub struct Daemon {
|
||||
/// 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 use_username_dirs: bool,
|
||||
pub server: String,
|
||||
pub token: String,
|
||||
pub started: Instant,
|
||||
@@ -428,7 +429,11 @@ impl Daemon {
|
||||
// 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 directory = crate::directories::download_dir_for(
|
||||
&self.download_dir(),
|
||||
¶ms.username,
|
||||
self.use_username_dirs,
|
||||
);
|
||||
let (download, updates) = self
|
||||
.session
|
||||
.download_with_metadata(
|
||||
@@ -800,6 +805,7 @@ mod tests {
|
||||
browses: Arc::new(PendingBrowses::default()),
|
||||
downloads: Arc::new(Mutex::new(Vec::new())),
|
||||
download_dir: Mutex::new("/tmp".to_string()),
|
||||
use_username_dirs: false,
|
||||
server: "server.invalid:2416".to_string(),
|
||||
token: token.to_string(),
|
||||
started: Instant::now(),
|
||||
|
||||
@@ -50,9 +50,54 @@ pub fn resolve_shared_directories(dirs: &[String]) -> Vec<String> {
|
||||
resolved
|
||||
}
|
||||
|
||||
/// The folder a download from `username` lands in: `download_dir` itself, or
|
||||
/// a per-user subfolder of it when `per_user` is set, as SoulseekQt does.
|
||||
#[must_use]
|
||||
pub fn download_dir_for(
|
||||
download_dir: &str,
|
||||
username: &str,
|
||||
per_user: bool,
|
||||
) -> String {
|
||||
if !per_user {
|
||||
return download_dir.to_string();
|
||||
}
|
||||
std::path::Path::new(download_dir)
|
||||
.join(sanitize_path_component(username))
|
||||
.display()
|
||||
.to_string()
|
||||
}
|
||||
|
||||
/// A peer-chosen name made safe as one path component: separators and
|
||||
/// characters Windows refuses become `_`, so `../..` cannot climb out.
|
||||
fn sanitize_path_component(name: &str) -> String {
|
||||
let mut out: String = name
|
||||
.chars()
|
||||
.map(|c| match c {
|
||||
'/' | '\\' | ':' | '*' | '?' | '"' | '<' | '>' | '|' => '_',
|
||||
c if c.is_control() => '_',
|
||||
c => c,
|
||||
})
|
||||
.collect();
|
||||
if out.is_empty() || out.ends_with(['.', ' ']) {
|
||||
out.push('_');
|
||||
}
|
||||
if is_reserved(&out) {
|
||||
out.insert(0, '_');
|
||||
}
|
||||
out
|
||||
}
|
||||
|
||||
/// Windows device names (`CON`, `COM1`, …) that cannot be a folder name.
|
||||
fn is_reserved(name: &str) -> bool {
|
||||
let stem = name.split('.').next().unwrap_or(name).to_ascii_uppercase();
|
||||
matches!(stem.as_str(), "CON" | "PRN" | "AUX" | "NUL")
|
||||
|| (matches!(stem.get(..3), Some("COM" | "LPT"))
|
||||
&& matches!(stem.as_bytes().get(3..), Some([b'1'..=b'9'])))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::resolve_shared_directory;
|
||||
use super::{download_dir_for, resolve_shared_directory};
|
||||
use std::sync::atomic::{AtomicU32, Ordering};
|
||||
|
||||
fn unique_temp_dir() -> std::path::PathBuf {
|
||||
@@ -97,4 +142,23 @@ mod tests {
|
||||
assert!(result.is_err());
|
||||
let _ = std::fs::remove_dir_all(dir);
|
||||
}
|
||||
|
||||
fn dl(name: &str) -> String {
|
||||
std::path::Path::new("/dl").join(name).display().to_string()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn per_user_folder_is_a_child_of_the_download_dir() {
|
||||
assert_eq!(download_dir_for("/dl", "alice", false), "/dl");
|
||||
assert_eq!(download_dir_for("/dl", "alice", true), dl("alice"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_hostile_username_cannot_leave_the_download_dir() {
|
||||
assert_eq!(download_dir_for("/dl", "../..", true), dl(".._.._"));
|
||||
assert_eq!(download_dir_for("/dl", "/tmp/x", true), dl("_tmp_x"));
|
||||
assert_eq!(download_dir_for("/dl", "..", true), dl(".._"));
|
||||
assert_eq!(download_dir_for("/dl", "", true), dl("_"));
|
||||
assert_eq!(download_dir_for("/dl", "com1", true), dl("_com1"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -176,7 +176,7 @@ impl FileConfig {
|
||||
"use_username_dirs wants true or false, got '{other}'"
|
||||
));
|
||||
}
|
||||
})
|
||||
});
|
||||
}
|
||||
other => {
|
||||
return Err(format!(
|
||||
|
||||
@@ -276,6 +276,7 @@ fn context(
|
||||
version: ClientVersion::REFERENCE_CLIENT,
|
||||
},
|
||||
download_dir,
|
||||
use_username_dirs: resolved.use_username_dirs,
|
||||
max_concurrent_downloads: resolved.max_concurrent_downloads,
|
||||
search_timeout: Duration::from_secs(resolved.search_timeout),
|
||||
daemon,
|
||||
|
||||
@@ -1,10 +1,9 @@
|
||||
use super::MainTui;
|
||||
use crate::directories::download_dir_for;
|
||||
use crate::models::{DownloadEntry, FileDisplayData};
|
||||
use crate::ui::panes::{InfoSubject, selected_transfer};
|
||||
use crate::ui::sanitize_path_component;
|
||||
use soulseek_rs::types::UploadStatus;
|
||||
use soulseek_rs::{DownloadStatus, types::Download};
|
||||
use std::path::Path;
|
||||
use std::{sync::mpsc, thread};
|
||||
|
||||
impl MainTui {
|
||||
@@ -209,21 +208,15 @@ impl MainTui {
|
||||
peer_upload_speed: Some(file.speed),
|
||||
peer_free_slots: Some(file.slots),
|
||||
};
|
||||
let download_dir = if use_username_dirs {
|
||||
let sanitized_username =
|
||||
sanitize_path_component(file.username.as_str());
|
||||
Path::new(download_dir.as_str())
|
||||
.join(sanitized_username.as_str())
|
||||
.display()
|
||||
.to_string()
|
||||
} else {
|
||||
download_dir.clone()
|
||||
};
|
||||
match client.download_with_metadata(
|
||||
file.filename.clone(),
|
||||
file.username.clone(),
|
||||
file.size,
|
||||
download_dir,
|
||||
download_dir_for(
|
||||
&download_dir,
|
||||
&file.username,
|
||||
use_username_dirs,
|
||||
),
|
||||
metadata,
|
||||
) {
|
||||
Ok((download, rx)) => {
|
||||
|
||||
@@ -12,13 +12,11 @@ use crate::persist::{
|
||||
snapshot::{Snapshot, restore_messages, restore_searches},
|
||||
state::{PersistedMessage, StateStore},
|
||||
};
|
||||
use crate::ui::sanitize_path_component;
|
||||
use color_eyre::Result;
|
||||
use ratatui::{
|
||||
DefaultTerminal,
|
||||
crossterm::event::{self, Event, KeyEventKind, poll},
|
||||
};
|
||||
use std::path::Path;
|
||||
use std::{sync::Arc, time::Duration};
|
||||
|
||||
pub struct MainTui {
|
||||
@@ -134,23 +132,12 @@ impl MainTui {
|
||||
} else {
|
||||
let client = self.client.clone();
|
||||
let sender = sender.clone();
|
||||
let use_username_dirs = self.use_username_dirs;
|
||||
std::thread::spawn(move || {
|
||||
let sanitized_username =
|
||||
sanitize_path_component(entry.username.as_str());
|
||||
let download_dir = if use_username_dirs {
|
||||
Path::new(entry.download_directory.as_str())
|
||||
.join(sanitized_username.as_str())
|
||||
.display()
|
||||
.to_string()
|
||||
} else {
|
||||
entry.download_directory
|
||||
};
|
||||
match client.download(
|
||||
entry.filename.clone(),
|
||||
entry.username,
|
||||
entry.size,
|
||||
download_dir,
|
||||
entry.download_directory,
|
||||
) {
|
||||
Ok((download, rx)) => {
|
||||
let _ = sender.send((download, rx));
|
||||
@@ -268,6 +255,7 @@ impl MainTui {
|
||||
}
|
||||
}
|
||||
|
||||
#[expect(clippy::too_many_arguments, reason = "one call site per mode")]
|
||||
pub fn launch_main_tui(
|
||||
terminal: DefaultTerminal,
|
||||
client: Arc<dyn SessionApi>,
|
||||
|
||||
@@ -109,6 +109,7 @@ impl MainTui {
|
||||
if dir_applied {
|
||||
self.download_dir.clone_from(&download_dir);
|
||||
}
|
||||
self.use_username_dirs = use_username_dirs;
|
||||
|
||||
// Persist to config.toml so the change survives a restart. What was
|
||||
// refused live is not written either: the file must not promise a
|
||||
|
||||
@@ -108,6 +108,30 @@ fn applying_settings_locally_stays_on_this_machine() {
|
||||
assert_eq!(tui.download_dir, downloads_dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn toggling_username_folders_applies_live_and_persists() {
|
||||
let session = Arc::new(TalkativeSession::default());
|
||||
let scratch = tempfile::tempdir().expect("a temp dir");
|
||||
let config_path = scratch.path().join("config.toml");
|
||||
let mut tui = MainTui::new(
|
||||
session,
|
||||
scratch.path().to_str().expect("utf-8").to_string(),
|
||||
false,
|
||||
Duration::from_secs(1),
|
||||
None,
|
||||
Some(config_path.clone()),
|
||||
);
|
||||
|
||||
tui.open_settings();
|
||||
tui.state.settings.as_mut().expect("open").use_username_dirs = true;
|
||||
tui.apply_settings();
|
||||
|
||||
assert!(tui.use_username_dirs, "the next download uses it");
|
||||
let saved = crate::persist::config::FileConfig::load(&config_path)
|
||||
.expect("config saved");
|
||||
assert_eq!(saved.use_username_dirs, Some(true));
|
||||
}
|
||||
|
||||
/// The account block answers "who is this session, and what is it
|
||||
/// offering" without a trip to another screen.
|
||||
#[test]
|
||||
|
||||
@@ -72,41 +72,6 @@ pub const fn get_spinner_char(state: usize) -> &'static str {
|
||||
SPINNER_CHARS[state % SPINNER_CHARS.len()]
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn sanitize_path_component(name: &str) -> String {
|
||||
let mut out = name
|
||||
.chars()
|
||||
.map(|c| match c {
|
||||
'/' | '\\' | ':' | '*' | '?' | '"' | '<' | '>' | '|' => '_',
|
||||
c if c.is_control() => '_',
|
||||
c => c,
|
||||
})
|
||||
.collect::<String>();
|
||||
if out.is_empty() || out.ends_with(['.', ' ']) {
|
||||
out.push('_');
|
||||
}
|
||||
if is_reserved(out.as_str()) {
|
||||
out.insert(0, '_');
|
||||
}
|
||||
out
|
||||
}
|
||||
|
||||
#[cfg(target_os = "windows")]
|
||||
fn is_reserved(name: &str) -> bool {
|
||||
let stem = name.split('.').next().unwrap_or(name).to_ascii_uppercase();
|
||||
if matches!(stem.as_str(), "CON" | "PRN" | "AUX" | "NUL") {
|
||||
true
|
||||
} else {
|
||||
matches!(stem.get(..3), Some("COM" | "LPT"))
|
||||
&& matches!(stem.as_bytes().get(3..), Some([b'1'..=b'9']))
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(not(target_os = "windows"))]
|
||||
fn is_reserved(name: &str) -> bool {
|
||||
false
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
@@ -129,6 +129,11 @@ const SETTINGS: readonly Setting[] = [
|
||||
</>
|
||||
),
|
||||
},
|
||||
{
|
||||
key: 'use_username_dirs',
|
||||
fallback: 'false',
|
||||
body: <>Save each peer's files in a subfolder named after them.</>,
|
||||
},
|
||||
{
|
||||
key: 'max_concurrent_downloads',
|
||||
fallback: '20',
|
||||
|
||||
Reference in New Issue
Block a user