From c84eb56258af9908957f38ee2f2cb67ba709e1aa Mon Sep 17 00:00:00 2001 From: Jan Hallvard Larsen Date: Thu, 8 Oct 2026 16:20:51 +0100 Subject: [PATCH] UI: closing a document with unsaved changes asks first (#141) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit File ▸ Close, its shortcut (Ctrl/Cmd+W) and the tab's × closed a document with unsaved changes at once. Closing also discards the document's recovery data, so the edits were gone for good. The three user paths now go through menus::close_document. A document with unsaved changes opens a dialog with Save, Don't Save and Cancel: - Save runs the same Save the menu does, then closes. If the document is still unsaved afterwards (the file picker was cancelled), it stays open. - Don't Save closes it. - Cancel (or Escape) does nothing. A document with nothing to save closes as before. The engine command file.close is unchanged and never asks, so scripts, MCP and the control channel's engine.execute keep working unattended. ui.menu.invoke without params acts like the menu item, as it does for other dialogs. Quitting the app is not covered here. Recovery data survives a quit, so unsaved documents come back on the next start. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/ui-egui/src/chrome.rs | 2 +- crates/ui-egui/src/dialogs.rs | 32 +++++++++++++++- crates/ui-egui/src/lib.rs | 72 +++++++++++++++++++++++++++++++++++ crates/ui-egui/src/menus.rs | 19 +++++++++ 4 files changed, 122 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/chrome.rs b/crates/ui-egui/src/chrome.rs index dfb5e699b..78431e14d 100644 --- a/crates/ui-egui/src/chrome.rs +++ b/crates/ui-egui/src/chrome.rs @@ -491,7 +491,7 @@ pub fn doc_tabs(app: &mut DesignApp, ui: &mut egui::Ui) { x += w; } if let Some(i) = close { - let _ = app.run("file.close", json!({"index": i})); + crate::menus::close_document(app, Some(i)); } else if let Some(i) = activate { let _ = app.run("file.activate", json!({"index": i})); } diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 34832db14..147df7e8c 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -819,6 +819,7 @@ pub fn show(app: &mut DesignApp, ctx: &egui::Context) { "polygonSettings" => crate::i18n::tr(&app.ui.language, "Polygon Settings"), "userDictionary" => crate::i18n::tr(&app.ui.language, "User Dictionary"), "newWorkspace" => crate::i18n::tr(&app.ui.language, "New Workspace"), + "closeDocument" => crate::i18n::tr(&app.ui.language, "Unsaved Changes"), "menus" => crate::i18n::tr(&app.ui.language, "Menu Customization"), "importOptions" => crate::i18n::tr(&app.ui.language, "Import Options"), "fittingOptions" => crate::i18n::tr(&app.ui.language, "Frame Fitting Options"), @@ -837,7 +838,7 @@ pub fn show(app: &mut DesignApp, ctx: &egui::Context) { }; egui::Modal::new(egui::Id::new("dialog")).show(ctx, |ui| { ui.set_min_width(380.0); - ui.set_max_width(if d.id == "newDocument" && crate::i18n::is_rtl(&app.ui.language) { 380.0 } else { 640.0 }); + ui.set_max_width(if d.id == "closeDocument" || (d.id == "newDocument" && crate::i18n::is_rtl(&app.ui.language)) { 380.0 } else { 640.0 }); if crate::i18n::is_rtl(&app.ui.language) { ui.horizontal(|ui| { ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { @@ -1198,6 +1199,10 @@ pub fn show(app: &mut DesignApp, ctx: &egui::Context) { }); } } + "closeDocument" => { + let text = crate::i18n::tr(&app.ui.language, "“{}” has changes that are not saved. Save them before closing?").replace("{}", &d.s("title")); + crate::rtl::label(ui, text); + } "newWorkspace" => { ui.horizontal(|ui| { crate::rtl::label(ui, crate::i18n::tr(&app.ui.language, "Name:")); @@ -1369,11 +1374,12 @@ pub fn show(app: &mut DesignApp, ctx: &egui::Context) { _ => {} } ui.add_space(12.0); + let ok_label = if d.id == "closeDocument" { " Save " } else { " OK " }; ui.horizontal(|ui| { ui.with_layout(egui::Layout::right_to_left(egui::Align::Center), |ui| { if ui .add( - egui::Button::new(crate::rtl::widget(ui, egui::RichText::new(crate::i18n::tr(&app.ui.language, " OK ")).color(egui::Color32::WHITE))) + egui::Button::new(crate::rtl::widget(ui, egui::RichText::new(crate::i18n::tr(&app.ui.language, ok_label)).color(egui::Color32::WHITE))) .fill(crate::theme::Tokens::get(ui.ctx()).accent_strong), ) .clicked() @@ -1384,6 +1390,10 @@ pub fn show(app: &mut DesignApp, ctx: &egui::Context) { if ui.button(crate::rtl::widget(ui, crate::i18n::tr(&app.ui.language, "Cancel"))).clicked() || ui.input(|i| i.key_pressed(egui::Key::Escape)) { result = Some(false); } + if d.id == "closeDocument" && ui.button(crate::rtl::widget(ui, crate::i18n::tr(&app.ui.language, "Don't Save"))).clicked() { + d.fields.insert("discard".into(), json!(true)); + result = Some(true); + } }); }); }); @@ -1601,6 +1611,24 @@ pub fn confirm(app: &mut DesignApp) -> Result { app.run("document.preferences", doc) } "newWorkspace" => app.run("window.newWorkspace", json!({"name": d.s("name")})), + "closeDocument" => { + // The document may have moved in the tab row (or gone) while the dialog was open. + let uid = d.fields.get("uid").and_then(Value::as_u64); + let index = |app: &DesignApp| app.session.documents().iter().position(|doc| Some(doc.uid) == uid); + let Some(i) = index(app) else { return Ok(Value::Null) }; + if !d.b("discard") { + app.run("file.activate", json!({"index": i}))?; + app.run("app.save", json!({}))?; + // Still unsaved: the save was cancelled (no file chosen). Keep the document open. + if app.session.documents().get(i).is_none_or(|doc| doc.is_dirty()) { + return Ok(Value::Null); + } + } + match index(app) { + Some(i) => app.run("file.close", json!({"index": i})), + None => Ok(Value::Null), + } + } "importOptions" => app.run( "file.place", json!({"path": d.s("path"), "removeStyles": d.b("removeStyles"), "styleConflicts": d.s("styleConflicts"), "styleMap": d.fields.get("map").cloned().unwrap_or(json!({}))}), diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 7b474fa21..d81b0699c 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -872,6 +872,78 @@ pub fn now_ms() -> f64 { mod tests { use super::*; + /// An app with one document that has an unsaved edit. + fn app_with_unsaved_document() -> DesignApp { + let mut app = DesignApp::new(designcraft_engine::Session::new(), Services::default()); + app.run("file.new", json!({})).unwrap(); + assert!(!app.session.documents()[0].is_dirty(), "a new document has nothing to save"); + app.run("frame.create", json!({"rect": [36, 36, 136, 136]})).unwrap(); + assert!(app.session.documents()[0].is_dirty()); + app + } + + /// File ▸ Close (and its shortcut) closed a document with unsaved changes without asking, and + /// closing discards the document's recovery data too. + #[test] + fn closing_an_unsaved_document_asks_first() { + let mut app = app_with_unsaved_document(); + menus::activate(&mut app, "file.close", &Value::Null); + assert_eq!(app.session.documents().len(), 1, "the document stays open until the user answers"); + assert_eq!(app.ui.dialog.as_ref().map(|d| d.id.as_str()), Some("closeDocument")); + // Cancel: nothing happens. + app.ui.dialog = None; + assert!(app.session.documents()[0].is_dirty()); + // The tab's × asks too. + menus::close_document(&mut app, Some(0)); + assert_eq!(app.ui.dialog.as_ref().map(|d| d.id.as_str()), Some("closeDocument")); + assert_eq!(app.session.documents().len(), 1); + } + + #[test] + fn closing_without_saving_discards_the_document() { + let mut app = app_with_unsaved_document(); + menus::close_document(&mut app, None); + app.ui.dialog.as_mut().unwrap().fields.insert("discard".into(), json!(true)); + dialogs::confirm(&mut app).unwrap(); + assert!(app.session.documents().is_empty()); + assert!(app.ui.dialog.is_none()); + } + + #[test] + fn closing_with_save_writes_the_file_then_closes() { + let mut app = app_with_unsaved_document(); + let path = std::env::temp_dir().join(format!("designcraft-close-test-{}.designcraft", std::process::id())); + app.run("file.saveAs", json!({"path": path})).unwrap(); + let saved = std::fs::metadata(&path).unwrap().len(); + app.run("frame.create", json!({"rect": [200, 200, 300, 300]})).unwrap(); + menus::close_document(&mut app, None); + dialogs::confirm(&mut app).unwrap(); + assert!(app.session.documents().is_empty()); + let written = std::fs::metadata(&path).unwrap().len(); + std::fs::remove_file(&path).unwrap(); + assert!(written > saved, "the second frame was saved ({saved} → {written} bytes)"); + } + + /// Save on a document that has no file opens the file picker; cancelling it must not close the + /// document. (No picker service here: the same as cancelling.) + #[test] + fn a_cancelled_save_keeps_the_document_open() { + let mut app = app_with_unsaved_document(); + menus::close_document(&mut app, None); + dialogs::confirm(&mut app).unwrap(); + assert_eq!(app.session.documents().len(), 1); + assert!(app.session.documents()[0].is_dirty()); + } + + #[test] + fn closing_a_saved_document_does_not_ask() { + let mut app = DesignApp::new(designcraft_engine::Session::new(), Services::default()); + app.run("file.new", json!({})).unwrap(); + menus::activate(&mut app, "file.close", &Value::Null); + assert!(app.session.documents().is_empty()); + assert!(app.ui.dialog.is_none()); + } + #[test] fn snap_view_uses_the_saved_switches() { let mut ui = UiState::default(); diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index abff58c62..0d0043c65 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -1717,7 +1717,26 @@ pub fn menu_enabled(app: &DesignApp, id: &str) -> bool { /// A menu item was chosen. An engine command whose label ends in "…" and that takes parameters /// opens a dialog built from its parameter documentation (see [`crate::dialogs::command_fields`]). +/// Close document `index` (the active one when `None`) the way the user asks for it: with the +/// tab's ×, File ▸ Close or its shortcut. A document with unsaved changes asks first, because +/// closing also discards its recovery data. Scripts and agents use `file.close`, which never asks. +pub fn close_document(app: &mut DesignApp, index: Option) { + let Some(i) = index.or(app.session.active_index()) else { return }; + match app.session.documents().get(i) { + Some(d) if d.is_dirty() => { + app.ui.dialog = Some(crate::dialogs::Dialog::new("closeDocument", json!({"uid": d.uid, "title": d.title(), "discard": false}))); + } + _ => { + let _ = app.run("file.close", json!({"index": i})); + } + } +} + pub fn activate(app: &mut DesignApp, id: &str, params: &Value) { + if params.is_null() && id == "file.close" { + close_document(app, None); + return; + } if params.is_null() && id == "layout.documentSetup" { crate::dialogs::open_document_setup(app); return;