diff --git a/README.md b/README.md index fe01120..f030387 100644 --- a/README.md +++ b/README.md @@ -185,8 +185,10 @@ config directory: A value Collector does not understand falls back to its default and the rest of the file is kept. A file that is not valid JSON is renamed to -`preferences.json.invalid`, never overwritten, and Collector says so at the -bottom of the screen and starts from the defaults. +`preferences.json.invalid` (or `.invalid.1`, `.invalid.2`... so an earlier copy +is never overwritten), and Collector says so at the bottom of the screen and +starts from the defaults. A file Collector cannot read at all stays where it +is, and nothing is saved until the next start. ### Language diff --git a/src/i18n/en.rs b/src/i18n/en.rs index 3ea915d..bd4e84d 100644 --- a/src/i18n/en.rs +++ b/src/i18n/en.rs @@ -137,7 +137,7 @@ Keys: p opens and closes Preferences, q or Ctrl+C quits. ("preferences_file", "Preferences file:"), ( "preferences_set_aside", - "Invalid preferences set aside as preferences.json.invalid", + "Invalid preferences set aside as {file}", ), ( "preferences_not_saved", diff --git a/src/i18n/fr.rs b/src/i18n/fr.rs index bd217e8..8b6e5b5 100644 --- a/src/i18n/fr.rs +++ b/src/i18n/fr.rs @@ -134,7 +134,7 @@ Touches : p ouvre et ferme les Préférences, q ou Ctrl+C quitte. ("preferences_file", "Fichier des préférences :"), ( "preferences_set_aside", - "Préférences invalides mises de côté : preferences.json.invalid", + "Préférences invalides mises de côté : {file}", ), ( "preferences_not_saved", diff --git a/src/preferences.rs b/src/preferences.rs index f2a9880..11c6e51 100644 --- a/src/preferences.rs +++ b/src/preferences.rs @@ -14,7 +14,7 @@ use std::path::{Path, PathBuf}; use serde_json::{Value, json}; -use crate::i18n::{Locale, t, tf}; +use crate::i18n::{Locale, tf}; use crate::{paths, theme}; /// The theme a fresh install starts on, and the one an unknown theme falls @@ -227,11 +227,12 @@ pub fn path() -> io::Result { } /// The preferences at `path`, or the defaults when there is no file yet. An -/// error means the file exists but cannot be read or is not JSON; it is left -/// untouched. +/// error means the file exists but cannot be read, or is not JSON (kind +/// `InvalidData`); it is left untouched. pub fn load(path: &Path) -> io::Result { match fs::read_to_string(path) { - Ok(text) => Preferences::from_json(&text).map_err(io::Error::other), + Ok(text) => Preferences::from_json(&text) + .map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err)), Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(Preferences::default()), Err(err) => Err(err), } @@ -239,11 +240,12 @@ pub fn load(path: &Path) -> io::Result { /// Writes through a temporary file in the same directory, then renames it /// over the old one: a crash or a full disk mid-write leaves the previous file -/// whole rather than half of a new one. +/// whole rather than half of a new one. The temporary name carries the process +/// id, so two Collectors saving at once never write into the same file. pub fn save(path: &Path, preferences: &Preferences) -> io::Result<()> { let dir = path.parent().unwrap_or(Path::new(".")); fs::create_dir_all(dir)?; - let temporary = with_suffix(path, ".tmp"); + let temporary = with_suffix(path, &format!(".{}.tmp", std::process::id())); let written = File::create(&temporary).and_then(|mut file| { file.write_all(preferences.to_json().as_bytes())?; file.sync_all() @@ -256,28 +258,50 @@ pub fn save(path: &Path, preferences: &Preferences) -> io::Result<()> { } /// Where the monitor saves, and what the status line tells the user about the -/// file, given what [`path`] and [`load`] returned. A file that failed to load -/// is renamed to `preferences.json.invalid` first, so that saving never -/// overwrites something the user may want back. If even that fails, nothing -/// is saved this session. +/// file, given what [`path`] and [`load`] returned. A file that is not valid +/// preferences is renamed to `preferences.json.invalid` first (`.invalid.1`, +/// `.invalid.2`... when an earlier one is there), so that saving never +/// overwrites something the user may want back. A file that cannot be read at +/// all stays where it is, and nothing is saved this session; nor is it when +/// the rename fails. pub fn save_target( path: io::Result, loaded: &io::Result, ) -> (Option, Option) { - let not_saved = |err: io::Error| { + let not_saved = |err: &io::Error| { let message = tf("preferences_not_saved", &[("error", &err.to_string())]); (None, Some(message)) }; match (path, loaded) { - (Err(err), _) => not_saved(err), + (Err(err), _) => not_saved(&err), (Ok(path), Ok(_)) => (Some(path), None), - (Ok(path), Err(_)) => match fs::rename(&path, with_suffix(&path, ".invalid")) { - Ok(()) => (Some(path), Some(t("preferences_set_aside").to_owned())), - Err(err) => not_saved(err), - }, + (Ok(path), Err(err)) if err.kind() == io::ErrorKind::InvalidData => { + let aside = set_aside_name(&path); + match fs::rename(&path, &aside) { + Ok(()) => { + let file = aside.file_name().unwrap_or_default().to_string_lossy(); + let message = tf("preferences_set_aside", &[("file", &file)]); + (Some(path), Some(message)) + } + Err(err) => not_saved(&err), + } + } + (Ok(_), Err(err)) => not_saved(err), } } +/// The first of `.invalid`, `.invalid.1`, `.invalid.2`... +/// that does not exist yet. +fn set_aside_name(path: &Path) -> PathBuf { + let mut aside = with_suffix(path, ".invalid"); + let mut n = 0; + while aside.exists() { + n += 1; + aside = with_suffix(path, &format!(".invalid.{n}")); + } + aside +} + fn with_suffix(path: &Path, suffix: &str) -> PathBuf { let mut name = path.as_os_str().to_owned(); name.push(suffix); @@ -429,7 +453,8 @@ mod tests { }; save(&path, &preferences).unwrap(); assert_eq!(load(&path).unwrap(), preferences); - assert!(!with_suffix(&path, ".tmp").exists(), "temporary file left"); + let temporary = with_suffix(&path, &format!(".{}.tmp", std::process::id())); + assert!(!temporary.exists(), "temporary file left"); // No accent: left out, as colony-ui writes it, and read back as none. let preferences = Preferences::default(); @@ -472,6 +497,39 @@ mod tests { save(&path, &Preferences::default()).unwrap(); assert_eq!(load(&path).unwrap(), Preferences::default()); assert!(with_suffix(&path, ".invalid").exists()); + + // A second bad file goes next to the first, which stays as it was. + fs::write(&path, "second").unwrap(); + let loaded = load(&path); + let (target, status) = save_target(Ok(path.clone()), &loaded); + assert_eq!(target.as_deref(), Some(path.as_path())); + assert!(status.unwrap().contains("preferences.json.invalid.1")); + assert_eq!( + fs::read_to_string(with_suffix(&path, ".invalid")).unwrap(), + "{ not json" + ); + assert_eq!( + fs::read_to_string(with_suffix(&path, ".invalid.1")).unwrap(), + "second" + ); + } + + #[test] + fn an_unreadable_file_stays_where_it_is() { + let dir = TempDir::new("unreadable"); + fs::create_dir_all(&dir.0).unwrap(); + // A directory where the file should be: it cannot be read, but it is + // not a bad preferences file either. + let path = dir.0.join("preferences.json"); + fs::create_dir(&path).unwrap(); + + let loaded = load(&path); + assert!(loaded.is_err()); + let (target, status) = save_target(Ok(path.clone()), &loaded); + assert_eq!(target, None, "nothing is saved"); + assert!(status.is_some(), "the user is told"); + assert!(path.is_dir(), "left where it is"); + assert!(!with_suffix(&path, ".invalid").exists()); } #[test] diff --git a/src/ui/preferences.rs b/src/ui/preferences.rs index 91acede..b70fa6a 100644 --- a/src/ui/preferences.rs +++ b/src/ui/preferences.rs @@ -337,7 +337,8 @@ pub fn render(frame: &mut Frame, app: &App, page: &Page, area: Rect, hits: &mut height: 1, ..inner }; - let style = style::item(index == page.category, hovered(app, item)); + // Muted unless selected, which the accent's on_accent text overrides. + let style = style::muted().patch(style::item(index == page.category, hovered(app, item))); frame.render_widget(Paragraph::new(format!(" {label}")).style(style), item); hits.push(Hit { area: item,