dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Stop the export folder forgetting itself, and let Android reach one

Browse Source 
Three faults, compounding into an export that could not be made to work on a
tablet at all and a destination that appeared to reset on its own.

**Switching target destroyed the destination.** One field held both a
filesystem path and a remote folder, so changing the target had to clear it —
`/home/x/Exports` carried to the server would have offered to create a folder
called `home` at the library root. The consequence was that merely looking at
the other option threw away the destination already chosen, which reads,
correctly, as a setting that will not stick. There are two fields now. Each
target remembers where it was pointed and switching is free; `active_destination`
picks between them so no caller can reach for the wrong one.

**The library root read as "unset".** The picker opens at the root, so
confirming it where it opens stored an empty string — indistinguishable from
"ask each time" and looking exactly like the picker had done nothing. Empty
now means the library root for a server destination, which is a real folder
and the one the photographs are already in; it means "ask" only for a device
folder, where no path is worth assuming. The page labels it so.

**Android defaulted to a target it cannot use.** A device folder there means
the Storage Access Framework, which provides no filesystem path (ARCH §6.9)
and is not implemented — so the default target could never succeed however the
destination was filled in. The export button said "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. The device target is now
absent from `ExportTarget::available()` on Android and the default there is the
server, which needs no platform work at all. A settings file carrying an
unreachable target — copied from a desktop, say — is corrected on read rather
than left to fail at the last step.

Seven tests, each named for the fault it prevents returning. The compatibility
one matters most: a file written before `remote_destination` existed keeps its
device path and gains an empty remote one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-17 09:25:15 +02:00
co-authored by Claude Opus 5
parent 2330ed25e9
commit 8ea92545df
3 changed files with 239 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/dr-types/src/settings.rs
+217 -13
View File
@@ -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<String>) {
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();