diff --git a/core/dr-types/src/settings.rs b/core/dr-types/src/settings.rs index 110265f..dd622e8 100644 --- a/core/dr-types/src/settings.rs +++ b/core/dr-types/src/settings.rs @@ -208,12 +208,27 @@ pub struct ExportSettings { /// is how exports end up somewhere the user never looks, and this is the /// one field where the app genuinely does not know the answer. /// - /// What the string *is* depends on [`Self::target`], and on the platform: - /// a filesystem path on Linux, a SAF tree URI on Android, or a path under - /// the library root when the target is the server. The same widening - /// `SourceRef` performs for reads (ARCH §3.1) — no core API takes a - /// `Path`, because Android has none to give. + /// A filesystem path on Linux, a SAF tree URI on Android. The server has + /// its own field below. pub destination: String, + + /// Destination folder on the server, relative to the library root. + /// + /// **A second field rather than one reinterpreted by [`Self::target`].** + /// It was one field, and switching between targets had to clear it — + /// `/home/x/Exports` carried over to the server would have offered to + /// create a folder called `home` at the library root. Clearing it meant + /// that flipping the choice to look at the other option destroyed the + /// destination already set, which read, correctly, as the setting + /// refusing to stick. + /// + /// Two fields cost one line of JSON and remove the whole problem: each + /// target remembers where it was pointed, and switching is free. + /// + /// Empty means the library root, which is a real destination — unlike + /// [`Self::destination`], where empty means "ask each time" because there + /// is no sensible folder to assume on a filesystem. + pub remote_destination: String, } impl Default for ExportSettings { @@ -231,8 +246,9 @@ impl Default for ExportSettings { filename_template: "{name}".to_string(), collision: CollisionPolicy::Increment, strip_location: true, - target: ExportTarget::Device, + target: ExportTarget::default(), destination: String::new(), + remote_destination: String::new(), } } } @@ -254,24 +270,58 @@ impl Default for ExportSettings { /// Nextcloud and exports to a phone's local storage has put the output /// somewhere the user's other devices cannot see, which is rarely what was /// meant. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum ExportTarget { - /// A folder on this device — a path on Linux, a SAF tree grant on Android. + /// A folder on this device. /// - /// The default, and deliberately so despite the above: an export the user - /// cannot immediately open is a surprise, and on a desktop the file - /// manager is the obvious next step. The server is a choice, not an - /// assumption about where someone wants their pictures. - #[default] + /// The right default on a desktop: an export the user cannot immediately + /// open is a surprise, and the file manager is the obvious next step. + /// + /// **Not available on Android.** Writing to a folder there means the + /// Storage Access Framework, which provides no filesystem path (ARCH §6.9) + /// and is not implemented — so this target could not succeed however the + /// destination was filled in. It is excluded from [`Self::available`] + /// rather than offered and then failing. Device, /// A folder on the connected account, created if absent. Remote, } +impl Default for ExportTarget { + /// Device on a desktop, the server on Android. + /// + /// Not a preference dressed as a default. On Android the device target + /// cannot work at all, and defaulting to it produced the worst possible + /// first experience: the export button reported "no export folder is set", + /// the settings page offered no way to choose one, and the only way out + /// was to guess that the *other* target was the working one. + fn default() -> Self { + if cfg!(target_os = "android") { + Self::Remote + } else { + Self::Device + } + } +} + impl ExportTarget { pub const ALL: [Self; 2] = [Self::Device, Self::Remote]; + /// The targets this platform can actually reach. + /// + /// The settings page builds its choices from this rather than from + /// [`Self::ALL`], so Android does not offer a destination it cannot write + /// to. Offering it and failing at the last step is the shape of bug that + /// makes a feature look broken rather than absent. + pub fn available() -> &'static [Self] { + if cfg!(target_os = "android") { + &[Self::Remote] + } else { + &Self::ALL + } + } + pub fn label(self) -> &'static str { match self { Self::Device => "This device", @@ -290,6 +340,57 @@ impl ExportTarget { } } +impl ExportSettings { + /// The destination for the current target. + /// + /// An accessor rather than two fields the caller chooses between, because + /// choosing wrongly is silent: exporting to the server using the device + /// path would create a folder named after the first path segment at the + /// library root, and the pictures would be somewhere nobody looks. + pub fn active_destination(&self) -> &str { + match self.target { + ExportTarget::Device => &self.destination, + ExportTarget::Remote => &self.remote_destination, + } + } + + /// Set the destination for the current target, leaving the other alone. + pub fn set_active_destination(&mut self, value: impl Into) { + let value = value.into(); + match self.target { + ExportTarget::Device => self.destination = value, + ExportTarget::Remote => self.remote_destination = value, + } + } + + /// Whether an export can proceed without asking where to put it. + /// + /// The two targets differ, and the difference is not an oversight. A + /// filesystem has no folder worth assuming, so an empty device destination + /// means "ask each time". The server does have one — the library root, the + /// folder the photographs are already in — so an empty remote destination + /// is a real answer rather than a missing one. + pub fn destination_is_set(&self) -> bool { + match self.target { + ExportTarget::Device => !self.destination.trim().is_empty(), + ExportTarget::Remote => true, + } + } + + /// How to name the destination in the interface. + /// + /// The library root has to read as a place rather than as a blank field, + /// or confirming the picker where it opens looks like it did nothing. + pub fn destination_label(&self) -> &str { + match self.target { + ExportTarget::Device if self.destination.trim().is_empty() => "Ask each time", + ExportTarget::Device => &self.destination, + ExportTarget::Remote if self.remote_destination.trim().is_empty() => "Library root", + ExportTarget::Remote => &self.remote_destination, + } + } +} + /// Output container and codec (FR-EXP-1). #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] @@ -540,6 +641,21 @@ impl Settings { if self.export.destination.trim().is_empty() { self.export.destination.clear(); } + // Trimmed rather than merely emptied: a remote folder is a path + // segment sent to the server, and " Exports" would be created with the + // space in its name. + let trimmed = self.export.remote_destination.trim(); + if trimmed.len() != self.export.remote_destination.len() { + self.export.remote_destination = trimmed.to_string(); + } + + // A target this platform cannot reach, from a settings file copied + // between devices or written by an older build. Corrected rather than + // rejected — the alternative is an export button that cannot work and + // a page that cannot say why. + if !ExportTarget::available().contains(&self.export.target) { + self.export.target = ExportTarget::default(); + } } } @@ -680,6 +796,94 @@ mod tests { ); } + #[test] + fn switching_target_does_not_destroy_the_other_destination() { + // The bug this pair of fields exists to remove. With one field, + // switching had to clear it — so glancing at the other option threw + // away the destination already chosen, which reads as the setting + // refusing to stick. + let mut s = Settings::default(); + s.export.target = ExportTarget::Device; + s.export.set_active_destination("/home/x/Exports"); + s.export.target = ExportTarget::Remote; + s.export.set_active_destination("Exports/2026"); + + s.export.target = ExportTarget::Device; + assert_eq!(s.export.active_destination(), "/home/x/Exports"); + s.export.target = ExportTarget::Remote; + assert_eq!(s.export.active_destination(), "Exports/2026"); + } + + #[test] + fn the_library_root_is_a_destination_not_a_missing_one() { + // Confirming the folder picker where it opens stores an empty remote + // path. That is the library root — a real folder, the one the + // photographs are already in — and it must not read as "unset", or + // the picker appears to have done nothing. + let mut s = Settings::default(); + s.export.target = ExportTarget::Remote; + s.export.remote_destination = String::new(); + assert!(s.export.destination_is_set()); + assert_eq!(s.export.destination_label(), "Library root"); + } + + #[test] + fn an_empty_device_destination_still_means_ask() { + // The asymmetry is deliberate: a filesystem has no folder worth + // assuming, so empty there is a question rather than an answer. + let mut s = Settings::default(); + s.export.target = ExportTarget::Device; + s.export.destination = String::new(); + assert!(!s.export.destination_is_set()); + assert_eq!(s.export.destination_label(), "Ask each time"); + } + + #[test] + fn android_does_not_offer_a_destination_it_cannot_write_to() { + // The reason export could never succeed on a tablet: the default + // target was the device, whose implementation is SAF and does not + // exist, and the page offered no way to choose anything else that + // worked. + let available = ExportTarget::available(); + assert!(available.contains(&ExportTarget::Remote)); + if cfg!(target_os = "android") { + assert!(!available.contains(&ExportTarget::Device)); + assert_eq!(ExportTarget::default(), ExportTarget::Remote); + } else { + assert!(available.contains(&ExportTarget::Device)); + assert_eq!(ExportTarget::default(), ExportTarget::Device); + } + } + + #[test] + fn a_target_this_platform_cannot_reach_is_corrected_on_read() { + // A settings file copied from a desktop to a tablet. Left alone it + // would produce an export button that cannot work and a page with no + // explanation. + let mut s = Settings::default(); + s.export.target = ExportTarget::Device; + s.sanitise(); + assert!(ExportTarget::available().contains(&s.export.target)); + } + + #[test] + fn a_remote_folder_is_trimmed_rather_than_sent_with_its_spaces() { + // It becomes a path segment on the server; " Exports" would be + // created with the space in its name. + let mut s = Settings::default(); + s.export.remote_destination = " Exports/2026 ".to_string(); + s.sanitise(); + assert_eq!(s.export.remote_destination, "Exports/2026"); + } + + #[test] + fn a_file_written_before_the_remote_destination_existed_still_loads() { + let json = r#"{"export": {"destination": "/home/x/Exports"}}"#; + let parsed: Settings = serde_json::from_str(json).unwrap(); + assert_eq!(parsed.export.destination, "/home/x/Exports"); + assert_eq!(parsed.export.remote_destination, ""); + } + #[test] fn a_remote_target_round_trips_through_json() { let mut s = Settings::default(); diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index d7acf76..3442130 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -351,7 +351,7 @@ fn export_now( // since the server cannot be reached from here and may not be reachable // at all — see `export::stage` for why two queued exports of one name // both survive regardless. - let target_dir = std::path::PathBuf::from(&stored.export.destination); + let target_dir = std::path::PathBuf::from(stored.export.active_destination()); let taken = |name: &str| -> bool { match stored.export.target { dr_types::ExportTarget::Device => target_dir.join(name).exists(), @@ -373,7 +373,7 @@ fn export_now( export::place( &encoded, stored.export.target, - &stored.export.destination, + stored.export.active_destination(), &outbox, ) } diff --git a/ui/dr-ui/src/settings_ui.rs b/ui/dr-ui/src/settings_ui.rs index a197186..6602f08 100644 --- a/ui/dr-ui/src/settings_ui.rs +++ b/ui/dr-ui/src/settings_ui.rs @@ -90,7 +90,7 @@ impl SettingsController { /// survived only until the window closed would be the one setting that /// behaved differently from all the others. pub fn set_destination(&self, path: String) { - self.edit(|s| s.export.destination = path); + self.edit(|s| s.export.set_active_destination(path)); } /// Report a failure onto the page's error line. @@ -194,18 +194,19 @@ pub fn render(window: &AppWindow, controller: &SettingsController) { window.set_settings_collision_selected(index_of(&CollisionPolicy::ALL, &s.export.collision)); window.set_settings_strip_location(s.export.strip_location); - window.set_settings_target_labels(labels(ExportTarget::ALL.iter().map(|t| t.label()))); - window.set_settings_target_selected(index_of(&ExportTarget::ALL, &s.export.target)); - window.set_settings_destination(s.export.destination.clone().into()); + // `available()`, not `ALL`: Android cannot write to a device folder, and + // offering a target that fails at the last step is what made export look + // broken there rather than absent. + let targets = ExportTarget::available(); + window.set_settings_target_labels(labels(targets.iter().map(|t| t.label()))); + window.set_settings_target_selected(index_of(targets, &s.export.target)); + window.set_settings_destination(s.export.active_destination().into()); // The field means different things either side of the choice, and a // placeholder saying which is cheaper than a paragraph under it. - window.set_settings_destination_hint( - match s.export.target { - ExportTarget::Device => "Choose a folder…", - ExportTarget::Remote => "A folder under the library root, created if absent", - } - .into(), - ); + // The placeholder names what an empty field *means*, which differs by + // target: on a filesystem it is a question, on the server it is the + // library root. + window.set_settings_destination_hint(s.export.destination_label().into()); // --- the remote folder picker -------------------------------------- { @@ -518,7 +519,7 @@ where let ctl = controller.clone(); window.on_settings_destination_changed(move |text| { let Some(w) = weak.upgrade() else { return }; - ctl.edit(|s| s.export.destination = text.to_string()); + ctl.edit(|s| s.export.set_active_destination(text.to_string())); render(&w, &ctl); }); } @@ -528,17 +529,13 @@ where let ctl = controller.clone(); window.on_settings_target_changed(move |i| { let Some(w) = weak.upgrade() else { return }; - if let Some(&t) = ExportTarget::ALL.get(i.max(0) as usize) { - // The destination is cleared with the switch. A path and a - // remote folder are not the same kind of string, and carrying - // `/home/x/Exports` across to the server would offer to create - // a folder called `home` at the library root. - ctl.edit(|s| { - if s.export.target != t { - s.export.target = t; - s.export.destination.clear(); - } - }); + if let Some(&t) = ExportTarget::available().get(i.max(0) as usize) { + // Nothing is cleared. Each target keeps its own destination + // (see `ExportSettings::remote_destination`), so switching to + // look at the other option no longer throws away the one + // already set — which is what made this setting appear not to + // stick. + ctl.edit(|s| s.export.target = t); } render(&w, &ctl); });