From a01b00da8f3969f8e5aa859837bf5a43752c856c Mon Sep 17 00:00:00 2001 From: jahala Date: Sat, 10 Oct 2026 08:12:48 +0100 Subject: [PATCH] UI: closing a document with unsaved changes asks first (#141) (#146) 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) Co-authored-by: Brandon Thomas --- crates/ui-egui/src/chrome.rs | 2 +- crates/ui-egui/src/dialogs.rs | 31 ++++++++++++++- crates/ui-egui/src/lib.rs | 72 +++++++++++++++++++++++++++++++++++ crates/ui-egui/src/menus.rs | 19 +++++++++ 4 files changed, 122 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/chrome.rs b/crates/ui-egui/src/chrome.rs index 84a3ccb..1abcf41 100644 --- a/crates/ui-egui/src/chrome.rs +++ b/crates/ui-egui/src/chrome.rs @@ -514,7 +514,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 c443555..05f72bc 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -1007,6 +1007,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"), @@ -1027,6 +1028,7 @@ pub fn show(app: &mut DesignApp, ctx: &egui::Context) { ui.set_min_width(380.0); ui.set_max_width(match d.id.as_str() { "alert" => 440.0, + "closeDocument" => 380.0, "newDocument" if crate::i18n::is_rtl(&app.ui.language) => 380.0, _ => 640.0, }); @@ -1395,6 +1397,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:")); @@ -1555,11 +1561,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() @@ -1572,6 +1579,10 @@ pub fn show(app: &mut DesignApp, ctx: &egui::Context) { if cancel || 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); + } }); }); }); @@ -1871,6 +1882,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.open_file( "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 704a52e..69c2443 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1001,6 +1001,78 @@ pub fn drop_key_name_text(raw: &mut egui::RawInput, text_field_focused: bool) { 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()); + } + /// The Control panel's two-row groups were centred as if they were one row tall, so the second /// row hung below the panel and was cut off. #[test] diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 2449224..22b2076 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -1772,9 +1772,28 @@ pub fn menu_enabled(app: &DesignApp, id: &str) -> bool { enabled(app, id) } +/// 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})); + } + } +} + /// 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`]). 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;