From 6bb15d227859d64aee6545c7769d514c3a623432 Mon Sep 17 00:00:00 2001 From: David Mann Date: Thu, 22 Oct 2026 21:39:04 +0200 Subject: [PATCH] Integration: keep another instance's shift tool switch choice (#5878) Two instances migrate the same version 1 file (useShiftKeyForToolSwitch false -> true), so both save bases hold false. When B turns the switch off and saves version 2, A's next unrelated save wrote its migrated true over that choice. write_prefs now drops the migration delta from the base when the file on disk is already version 2 or later. Test: another_instances_shift_tool_switch_choice_beats_this_ones_migration (fails without the fix: true written over false). Co-authored-by: Higgs32584 <55243596+Higgs32584@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/ui-egui/src/prefs_ui.rs | 36 ++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/crates/ui-egui/src/prefs_ui.rs b/crates/ui-egui/src/prefs_ui.rs index 28e613504c..cb21fde285 100644 --- a/crates/ui-egui/src/prefs_ui.rs +++ b/crates/ui-egui/src/prefs_ui.rs @@ -618,6 +618,7 @@ fn write_prefs(app: &mut PhotocraftApp, ours: &Value) -> Result<(), String> { let stored = app.services.load_prefs.as_mut().and_then(|load| load()).and_then(|t| serde_json::from_str::(&t).ok()); let text = match (stored, &app.prefs_rt.saved_value) { (Some(mut theirs @ Value::Object(_)), Some(base)) => { + let base = &without_stale_migration(base, ours, &theirs); merge_changes(base, ours, &mut theirs); serde_json::to_string_pretty(&theirs) } @@ -627,6 +628,21 @@ fn write_prefs(app: &mut PhotocraftApp, ours: &Value) -> Result<(), String> { save(&text) } +/// The save base without the shift tool switch migration (#5647) once storage already holds a +/// version 2 file: another instance wrote it, so its Use Shift Key for Tool Switch is a choice, +/// not the old default this instance migrated, and must not be overwritten by the migration. +fn without_stale_migration(base: &Value, ours: &Value, theirs: &Value) -> Value { + let mut base = base.clone(); + let new_file = |v: &Value| v.get("version").and_then(Value::as_u64).is_some_and(|v| v >= 2); + if new_file(theirs) && !new_file(&base) { + let key = "useShiftKeyForToolSwitch"; + if let (Some(o), Some(tools)) = (ours.pointer("/tools").and_then(|t| t.get(key)), base.get_mut("tools").and_then(Value::as_object_mut)) { + tools.insert(key.into(), o.clone()); + } + } + base +} + /// Three-way merge of preference trees: whatever `ours` changed since `base` goes over `theirs` /// (what storage holds now) and the rest of `theirs` is kept. Objects merge key by key; any /// other value, arrays included, is replaced whole. @@ -3276,6 +3292,26 @@ mod tests { assert_eq!((v["interface"]["theme"].as_str(), v["performance"]["historyStates"].as_u64()), (Some("studio"), Some(12))); } + #[test] + fn another_instances_shift_tool_switch_choice_beats_this_ones_migration() { + // Two instances migrate the same version 1 file. One turns the switch off and saves + // version 2; the other's unrelated save must not write its migrated "on" over that. + let saved = json!({"version": 1, "tools": {"useShiftKeyForToolSwitch": false}}); + let store = Arc::new(Mutex::new(Some(saved.to_string()))); + let ctx = egui::Context::default(); + let (mut a, mut b) = (app_on(&store, Arc::default()), app_on(&store, Arc::default())); + tick(&mut a, &ctx); + tick(&mut b, &ctx); + b.run("prefs.set", json!({"values": {"tools.useShiftKeyForToolSwitch": false}})).unwrap(); + tick(&mut b, &ctx); + assert_eq!(stored(&store)["tools"]["useShiftKeyForToolSwitch"], false); + a.run("prefs.set", json!({"values": {"performance.historyStates": 12}})).unwrap(); + tick(&mut a, &ctx); + let v = stored(&store); + assert_eq!((v["tools"]["useShiftKeyForToolSwitch"].as_bool(), v["performance"]["historyStates"].as_u64()), (Some(false), Some(12))); + assert_eq!(v["version"], json!(prefs::PREFS_VERSION)); + } + #[test] fn a_newer_files_version_is_kept_on_save() { let saved = json!({"version": prefs::PREFS_VERSION + 1, "tools": {"useShiftKeyForToolSwitch": false}});