From 5ecd0451565381eee0a1025305ebba3aba44d4b4 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 14 Oct 2026 08:41:13 +0700 Subject: [PATCH] Fix #1693: a browser download that can't be started fails the save it is for The web app's download callback returned nothing, so a save whose Blob, URL or anchor couldn't be made was reported as written: the document counted as saved, its recovery copy was dropped, and Save Changes closed it. The callback now returns the error, which fails the save, export or file command like a refused write on the desktop. A timer that can't be set after the download started is not a failure. --- apps/vectorcraft-web/src/web.rs | 10 +++--- crates/ui-egui/src/dialogs/save_options.rs | 5 ++- crates/ui-egui/src/dialogs/tests_package.rs | 7 ++-- .../ui-egui/src/dialogs/tests_save_for_web.rs | 5 ++- .../src/dialogs/tests_screen_assets.rs | 5 ++- crates/ui-egui/src/dialogs/tests_screens.rs | 5 ++- crates/ui-egui/src/io.rs | 34 +++++++++++++++++-- crates/ui-egui/src/lib.rs | 4 ++- crates/ui-egui/src/panels/library_panel.rs | 9 +++-- crates/ui-egui/src/panels/links.rs | 5 +-- .../ui-egui/src/panels/tests_asset_export.rs | 5 ++- crates/ui-egui/src/shortcut_editor.rs | 5 +-- crates/ui-egui/src/tests_nativeoptions.rs | 5 ++- 13 files changed, 78 insertions(+), 26 deletions(-) diff --git a/apps/vectorcraft-web/src/web.rs b/apps/vectorcraft-web/src/web.rs index 50028187..e4e3bbd7 100644 --- a/apps/vectorcraft-web/src/web.rs +++ b/apps/vectorcraft-web/src/web.rs @@ -434,11 +434,8 @@ fn services(inbox: Inbox, place_inbox: PlaceInbox, ctx: egui::Context, locks: Op ctx.request_repaint(); }); })), - download: Some(Box::new(|name: &str, bytes: &[u8]| { - if let Err(e) = download(name, bytes) { - log::error!("download of {name} failed: {e}"); - } - })), + // A download that couldn't be started fails the save or export it is for (#1693). + download: Some(Box::new(|name: &str, bytes: &[u8]| download(name, bytes).inspect_err(|e| log::error!("download of {name} failed: {e}")))), inbox: Some(inbox), recovery_store: Some(Arc::new(BrowserStore { locks })), // File → Print: the browser's print dialog. @@ -517,7 +514,8 @@ fn download(path: &str, bytes: &[u8]) -> Result<(), String> { let revoke = wasm_bindgen::closure::Closure::once_into_js(move || { web_sys::Url::revoke_object_url(&url).ok(); }); - window.set_timeout_with_callback_and_timeout_and_arguments_0(revoke.unchecked_ref(), 10_000).map_err(js)?; + // The download has started: a timer that can't be set leaves the URL allocated, no more. + let _ = window.set_timeout_with_callback_and_timeout_and_arguments_0(revoke.unchecked_ref(), 10_000); Ok(()) } diff --git a/crates/ui-egui/src/dialogs/save_options.rs b/crates/ui-egui/src/dialogs/save_options.rs index 92958d8a..0d5b5a06 100644 --- a/crates/ui-egui/src/dialogs/save_options.rs +++ b/crates/ui-egui/src/dialogs/save_options.rs @@ -268,7 +268,10 @@ mod tests { let written = Written::default(); let w = written.clone(); let services = Services { - download: Some(Box::new(move |name: &str, b: &[u8]| w.borrow_mut().push((name.to_string(), b.to_vec())))), + download: Some(Box::new(move |name: &str, b: &[u8]| { + w.borrow_mut().push((name.to_string(), b.to_vec())); + Ok(()) + })), ..Default::default() }; (with_art(services), written) diff --git a/crates/ui-egui/src/dialogs/tests_package.rs b/crates/ui-egui/src/dialogs/tests_package.rs index f9607395..ecd26301 100644 --- a/crates/ui-egui/src/dialogs/tests_package.rs +++ b/crates/ui-egui/src/dialogs/tests_package.rs @@ -95,7 +95,10 @@ fn the_web_downloads_a_zip() { let mut app = saved(&dir); let got: Rc>> = Rc::default(); let g = got.clone(); - app.services.download = Some(Box::new(move |name: &str, b: &[u8]| g.borrow_mut().push((name.to_string(), b.len())))); + app.services.download = Some(Box::new(move |name: &str, b: &[u8]| { + g.borrow_mut().push((name.to_string(), b.len())); + Ok(()) + })); app.run("ui.packageDialog", json!({})).unwrap(); assert!(!frame(&mut app).contains("Location:"), "no folder to choose"); confirm(&mut app).unwrap(); @@ -124,7 +127,7 @@ fn fonts_left_out_of_the_package_are_named() { assert!(detail.contains(gone) && !detail.contains("Source Sans 3"), "{detail}"); // The web's download says so too. app.ui.dialog = None; - app.services.download = Some(Box::new(|_: &str, _: &[u8]| {})); + app.services.download = Some(Box::new(|_: &str, _: &[u8]| Ok(()))); app.run("ui.packageDialog", json!({})).unwrap(); confirm(&mut app).unwrap(); assert!(app.ui.status.contains(gone), "{}", app.ui.status); diff --git a/crates/ui-egui/src/dialogs/tests_save_for_web.rs b/crates/ui-egui/src/dialogs/tests_save_for_web.rs index 164087a9..b13d36f2 100644 --- a/crates/ui-egui/src/dialogs/tests_save_for_web.rs +++ b/crates/ui-egui/src/dialogs/tests_save_for_web.rs @@ -24,7 +24,10 @@ fn app(dir: &std::path::Path, web: bool) -> (VectorcraftApp, Log) { ..Default::default() }; if web { - services.download = Some(Box::new(move |name: &str, b: &[u8]| l2.borrow_mut().push(format!("download {name} {}", b.len())))); + services.download = Some(Box::new(move |name: &str, b: &[u8]| { + l2.borrow_mut().push(format!("download {name} {}", b.len())); + Ok(()) + })); } let mut app = VectorcraftApp::new(Session::new(), services); app.run("file.new", json!({"width": 60, "height": 40})).unwrap(); diff --git a/crates/ui-egui/src/dialogs/tests_screen_assets.rs b/crates/ui-egui/src/dialogs/tests_screen_assets.rs index b052d165..9274b522 100644 --- a/crates/ui-egui/src/dialogs/tests_screen_assets.rs +++ b/crates/ui-egui/src/dialogs/tests_screen_assets.rs @@ -21,7 +21,10 @@ fn app(web: bool) -> (VectorcraftApp, Log, [u64; 2]) { let l = log.clone(); let mut services = Services::default(); if web { - services.download = Some(Box::new(move |name: &str, bytes: &[u8]| l.borrow_mut().push((name.to_string(), bytes.to_vec())))); + services.download = Some(Box::new(move |name: &str, bytes: &[u8]| { + l.borrow_mut().push((name.to_string(), bytes.to_vec())); + Ok(()) + })); } else { services.reveal = Some(Box::new(move |path: &str| { l.borrow_mut().push((path.to_string(), vec![])); diff --git a/crates/ui-egui/src/dialogs/tests_screens.rs b/crates/ui-egui/src/dialogs/tests_screens.rs index 506001ab..4341415c 100644 --- a/crates/ui-egui/src/dialogs/tests_screens.rs +++ b/crates/ui-egui/src/dialogs/tests_screens.rs @@ -21,7 +21,10 @@ fn app(artboards: usize, web: bool) -> (VectorcraftApp, Log) { let l = log.clone(); let mut services = Services::default(); if web { - services.download = Some(Box::new(move |name: &str, bytes: &[u8]| l.borrow_mut().push((name.to_string(), bytes.to_vec())))); + services.download = Some(Box::new(move |name: &str, bytes: &[u8]| { + l.borrow_mut().push((name.to_string(), bytes.to_vec())); + Ok(()) + })); } else { services.reveal = Some(Box::new(move |path: &str| { l.borrow_mut().push((path.to_string(), vec![])); diff --git a/crates/ui-egui/src/io.rs b/crates/ui-egui/src/io.rs index 314462c3..66198372 100644 --- a/crates/ui-egui/src/io.rs +++ b/crates/ui-egui/src/io.rs @@ -239,8 +239,7 @@ pub fn new_from_template(app: &mut VectorcraftApp, path: Option) -> Resu /// Web: download `bytes` under `path`'s file name; desktop: write them to `path`. pub(crate) fn write_to(services: &mut Services, path: &str, bytes: &[u8]) -> Result<(), String> { if let Some(dl) = services.download.as_mut() { - dl(&fileio::file_name(path), bytes); - return Ok(()); + return dl(&fileio::file_name(path), bytes); } services.write.as_mut().ok_or("no file writer")?(path, bytes) } @@ -768,7 +767,7 @@ pub(crate) fn export_files(app: &mut VectorcraftApp, id: &str, params: Value) -> } else if let Some(download) = app.services.download.as_mut() { for (name, data) in &returned { let bytes = vectorcraft_format::base64_decode(data).ok_or("the export returned unreadable data")?; - download(name, &bytes); + download(name, &bytes)?; } let what = match returned.as_slice() { [(name, _)] => (*name).to_string(), @@ -830,6 +829,35 @@ mod tests { assert!(app.ui.dialog.is_none()); } + /// #1693: a browser download that can't be started is a failed save: the document stays + /// modified and without a file, and Save Changes doesn't close it. + #[test] + fn a_download_that_cannot_start_leaves_the_document_unsaved_and_open() { + let started = Rc::new(RefCell::new(false)); + let ok = started.clone(); + let download: crate::DownloadFn = Box::new(move |_: &str, _: &[u8]| if *ok.borrow() { Ok(()) } else { Err("the browser refused the download".into()) }); + let mut app = VectorcraftApp::new(Session::new(), Services { download: Some(download), ..Default::default() }); + app.run("file.new", json!({"width": 100, "height": 80})).unwrap(); + app.run("shape.rectangle", json!({"x": 10, "y": 10, "width": 30, "height": 20})).unwrap(); + let unsaved = |app: &VectorcraftApp| { + let st = app.session.doc().unwrap(); + (st.is_dirty(), st.path.clone()) + }; + let e = save(&mut app, SaveMode::Save, &json!({"path": "Poster.vectorcraft"}), false).unwrap_err(); + assert!(e.contains("the browser refused the download"), "{e}"); + assert_eq!(unsaved(&app), (true, None)); + // Save Changes › Save: not saved, so not closed. + let (i, uid) = (app.session.active_index().unwrap(), app.session.doc().unwrap().uid); + app.ui.dialog = Some(crate::state::Dialog::new(crate::unsaved::KIND, json!({"index": i, "uid": uid, "name": "Poster", "then": "close"}))); + app.session.doc_mut().unwrap().path = Some("Poster.vectorcraft".into()); + assert!(crate::unsaved::confirm(&mut app).is_err()); + assert_eq!((app.session.documents().len(), app.session.doc().unwrap().is_dirty()), (1, true)); + // Once the download starts, the save counts. + *started.borrow_mut() = true; + save(&mut app, SaveMode::Save, &json!({}), false).unwrap(); + assert!(!app.session.doc().unwrap().is_dirty()); + } + fn bytes_of(app: &mut VectorcraftApp, cmd: &str, p: Value) -> Vec { let v = app.session.execute(cmd, &p).unwrap(); vectorcraft_format::base64_decode(v["dataBase64"].as_str().unwrap()).unwrap() diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index e3e953cc..aae24fbe 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -188,7 +188,9 @@ pub type PickSave = Box Option>; pub type ReadFn = Box Result, String>>; pub type WriteFn = Box Result<(), String>>; pub type Inbox = Arc)>>>; -pub type DownloadFn = Box; +/// Starts a browser download of the bytes under a file name; an error when it couldn't be started +/// (the save or export then fails as a refused write does: #1693). +pub type DownloadFn = Box Result<(), String>>; /// Opens a URL in the system browser. pub type OpenUrlFn = Box; diff --git a/crates/ui-egui/src/panels/library_panel.rs b/crates/ui-egui/src/panels/library_panel.rs index dc2f1aab..00ca76fb 100644 --- a/crates/ui-egui/src/panels/library_panel.rs +++ b/crates/ui-egui/src/panels/library_panel.rs @@ -646,8 +646,13 @@ mod tests { // The web downloads the file under that name. let downloads = std::rc::Rc::new(std::cell::RefCell::new(vec![])); let dl = downloads.clone(); - let services = - crate::Services { download: Some(Box::new(move |name: &str, _: &[u8]| dl.borrow_mut().push(name.to_string()))), ..Default::default() }; + let services = crate::Services { + download: Some(Box::new(move |name: &str, _: &[u8]| { + dl.borrow_mut().push(name.to_string()); + Ok(()) + })), + ..Default::default() + }; let mut web = VectorcraftApp::new(Session::new(), services); web.run("file.new", json!({"width": 100, "height": 100})).unwrap(); web.run("ui.saveSwatchLibrary", json!({})).unwrap(); diff --git a/crates/ui-egui/src/panels/links.rs b/crates/ui-egui/src/panels/links.rs index 5250c854..6031e6ef 100644 --- a/crates/ui-egui/src/panels/links.rs +++ b/crates/ui-egui/src/panels/links.rs @@ -155,11 +155,12 @@ pub(crate) fn go_to(app: &mut VectorcraftApp, id: u64) { /// Write embedded image `id` to a picked file (the web downloads it) and link it there. pub(crate) fn unembed(app: &mut VectorcraftApp, id: u64, name: &str) { - if let Some(dl) = app.services.download.as_mut() { + if app.services.download.is_some() { if let Ok(v) = app.session.execute("links.unembed", &json!({ "id": id })) && let (Some(name), Some(bytes)) = (v["name"].as_str(), v["dataBase64"].as_str().and_then(vectorcraft_format::base64_decode)) + && let Err(e) = crate::io::write_to(&mut app.services, name, &bytes) { - dl(name, &bytes); + app.status(e); } return; } diff --git a/crates/ui-egui/src/panels/tests_asset_export.rs b/crates/ui-egui/src/panels/tests_asset_export.rs index 9b978e83..ace9b6cc 100644 --- a/crates/ui-egui/src/panels/tests_asset_export.rs +++ b/crates/ui-egui/src/panels/tests_asset_export.rs @@ -21,7 +21,10 @@ fn app(web: bool) -> (VectorcraftApp, Log, [u64; 2]) { let l = log.clone(); let mut services = Services::default(); if web { - services.download = Some(Box::new(move |name: &str, bytes: &[u8]| l.borrow_mut().push((name.to_string(), bytes.to_vec())))); + services.download = Some(Box::new(move |name: &str, bytes: &[u8]| { + l.borrow_mut().push((name.to_string(), bytes.to_vec())); + Ok(()) + })); } let mut app = VectorcraftApp::new(Session::new(), services); app.run("file.new", json!({"width": 200, "height": 100})).unwrap(); diff --git a/crates/ui-egui/src/shortcut_editor.rs b/crates/ui-egui/src/shortcut_editor.rs index 612f1370..7602867b 100644 --- a/crates/ui-egui/src/shortcut_editor.rs +++ b/crates/ui-egui/src/shortcut_editor.rs @@ -485,10 +485,7 @@ pub fn run_command(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option Err("cancelled".into()), None => match app.services.download.as_mut() { - Some(dl) => { - dl("VectorCraft Shortcuts.json", &bytes); - Ok(Value::Null) - } + Some(dl) => dl("VectorCraft Shortcuts.json", &bytes).map(|()| Value::Null), None => Ok(v), }, } diff --git a/crates/ui-egui/src/tests_nativeoptions.rs b/crates/ui-egui/src/tests_nativeoptions.rs index aec94be4..59b8c91d 100644 --- a/crates/ui-egui/src/tests_nativeoptions.rs +++ b/crates/ui-egui/src/tests_nativeoptions.rs @@ -86,7 +86,10 @@ fn the_web_save_as_dialog_shows_the_native_options() { let written = crate::tests_svg::Written::default(); let w = written.clone(); let services = crate::Services { - download: Some(Box::new(move |name: &str, b: &[u8]| w.borrow_mut().push((name.to_string(), b.to_vec())))), + download: Some(Box::new(move |name: &str, b: &[u8]| { + w.borrow_mut().push((name.to_string(), b.to_vec())); + Ok(()) + })), ..Default::default() }; let mut app = crate::VectorcraftApp::new(vectorcraft_engine::Session::new(), services);