diff --git a/Cargo.lock b/Cargo.lock index 60afde3..de3cab4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -865,9 +865,9 @@ checksum = "57c0d7b74b563b49d38dae00a0c37d4d6de9b432382b2892f0574ddcae73fd0a" [[package]] name = "patois" -version = "0.2.2" +version = "0.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2d8a7a8ed1b780906a3ee4cfb797cf467c0a758176701c10fd783ca0f2d39938" +checksum = "75b8d37756e8066067907b01d2cb1ffb7f2886707af4909280a523b2f98f59cc" dependencies = [ "inventory", "patois-macros", @@ -1223,6 +1223,7 @@ dependencies = [ "serde", "tempfile", "ureq", + "wx-utils", "wxdragon", ] @@ -1770,6 +1771,16 @@ version = "0.6.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3ad82d2a33cdc9674dc7465672f271e096168fcdbe0f799d9e6db8c5892679dc" +[[package]] +name = "wx-utils" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9ec56dbb82e24c9eff07f9e637f5a44410a4166ac9c8f25fe0fce34d8c58d17" +dependencies = [ + "patois", + "wxdragon", +] + [[package]] name = "wxdragon" version = "0.9.22" diff --git a/Cargo.toml b/Cargo.toml index b9270e4..8c9df98 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,7 @@ patois = "0.2.2" pulldown-cmark = { version = "0.13.4", default-features = false } serde = { version = "1.0.229", default-features = false, features = ["derive"] } ureq = { version = "3.4.2", default-features = false, features = ["json", "rustls"] } +wx-utils = "0.4.2" # default-features = false so this does not decide for the application # which parts of wxWidgets get built; nothing here needs any of them. wxdragon = { version = "0.9.22", default-features = false } diff --git a/README.md b/README.md index b9e14eb..6ba2c6b 100644 --- a/README.md +++ b/README.md @@ -98,10 +98,13 @@ that identity, fixtures are ad-hoc signed and demonstrate the manual fallback in trusts only its generated local fixture and keeps the production signer checks. It is a repository test harness, excluded from the published crate package. -The download gauge stays below 100 until verification and preparation finish. Native progress -updates can yield to the event loop, so completion waits for the active Update/Pulse call to -return before destroying the dialog or calling the shutdown handler. Regression tests cover this -ordering without requiring a graphical session. +The download window comes from the `progress` module of +[wx-utils](https://github.com/trypsynth/wx-utils). Its gauge stays below 100 until verification +and preparation finish. Native progress updates can yield to the event loop, so completion waits +for the active Update/Pulse call to return before destroying the dialog or calling the shutdown +handler. wx-utils' regression tests cover this ordering without requiring a graphical session. +wx-utils shows one progress window at a time, so a download that starts while the application +shows another one waits for it. Run `cargo test` on macOS for coverage of cancellation, helper timeouts, replacement and rollback, failed restore and launch, competing destination changes, protected locations, bundle selection, diff --git a/src/ui.rs b/src/ui.rs index 7ce57cb..b3c74fb 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -1,14 +1,15 @@ use std::{ + path::PathBuf, process, ptr, sync::{ Arc, - atomic::{AtomicBool, AtomicU64, Ordering}, + atomic::{AtomicBool, Ordering}, }, thread, - time::Duration, }; use patois::t; +use wx_utils::progress::{Ended, Progress, run_with_progress}; use wxdragon::{ffi, prelude::*, window::WxWidget}; pub use self::markdown::markdown_to_text; @@ -19,15 +20,6 @@ use crate::{ }; mod markdown; -mod progress; - -use progress::{ProgressLifecycle, download_percent}; - -const PROGRESS_INTERVAL: Duration = Duration::from_millis(200); - -thread_local! { - static ACTIVE_PROGRESS: ProgressLifecycle = const { ProgressLifecycle::new() }; -} /// Guards against a second update-check flow (silent startup check, manual "Check for /// Updates", or an impatient double-click while a download is stuck) from starting while @@ -78,16 +70,6 @@ pub enum CheckTrigger { Manual, } -#[derive(Default)] -struct DownloadProgress { - downloaded: AtomicU64, - total: AtomicU64, - finished: AtomicBool, - #[cfg(target_os = "macos")] - preparing: AtomicBool, - cancelled: AtomicBool, -} - /// Show the "update available" dialog and return `true` if the user accepted. /// /// `app_display_name` appears in the body label (e.g. `"A new version of My App is available."`). @@ -203,99 +185,56 @@ fn start_download( on_exit: Box, ) { let title = t("%s Update").replace("%s", &config.app_display_name); - let progress_dialog = ProgressDialog::builder(&parent, &title, &t("Downloading update..."), 100) - .with_style( - ProgressDialogStyle::AutoHide - | ProgressDialogStyle::AppModal - | ProgressDialogStyle::RemainingTime - | ProgressDialogStyle::CanAbort, - ) - .build(); - ACTIVE_PROGRESS.with(|p| p.set(progress_dialog)); - let progress = Arc::new(DownloadProgress::default()); - spawn_progress_heartbeat(Arc::clone(&progress)); - thread::spawn(move || { - let download = - download_update_file(&config, &result.download_url, &result.signature_url, &progress.cancelled, |d, t| { - progress.downloaded.store(d, Ordering::Relaxed); - progress.total.store(t, Ordering::Relaxed); - }); - #[cfg(target_os = "macos")] - let download = if progress.cancelled.load(Ordering::Relaxed) { - if let Ok(ref path) = download { - platform::remove_private_download(path); - } - Err(t("Update cancelled.")) - } else { - download.map_err(|e| format!("{}: {e}", t("Update failed"))).and_then(|path| { - progress.preparing.store(true, Ordering::Relaxed); - let result = platform::install_with_cancel(&config, &path, &progress.cancelled); - // A manual fallback still needs its DMG. Automatic preparation has copied the - // bundle, so its private verified download can be removed. - if !matches!(result, Ok(InstallOutcome::ManualStep(_))) { - platform::remove_private_download(&path); - } - result - }) - }; - progress.finished.store(true, Ordering::Relaxed); - wxdragon::call_after(Box::new(move || { - ACTIVE_PROGRESS.with(|p| { - p.finish(Box::new(move || { - ACTIVE_PROGRESS.with(ProgressLifecycle::clear); - if !progress.cancelled.load(Ordering::Relaxed) { - #[cfg(not(target_os = "macos"))] - let download = download - .map_err(|e| format!("{}: {e}", t("Update failed"))) - .and_then(|path| platform::install(&config, &path)); - install_update(parent, download, on_exit); - } - drop(check); - })); - }); - })); - wxdragon::wake_up_idle(); + #[cfg(not(target_os = "macos"))] + let install_config = Arc::clone(&config); + #[cfg(not(target_os = "macos"))] + let work = move |progress: &Progress| fetch(&config, &result, progress); + #[cfg(target_os = "macos")] + let work = move |progress: &Progress| prepare(&config, fetch(&config, &result, progress), progress); + run_with_progress(&parent, &title, &t("Downloading update..."), work, move |download, ended| { + if ended == Ended::Completed { + #[cfg(not(target_os = "macos"))] + let download = download + .map_err(|e| format!("{}: {e}", t("Update failed"))) + .and_then(|path| platform::install(&install_config, &path)); + install_update(parent, download, on_exit); + } + drop(check); }); } -/// Updates the progress dialog from the main thread every [`PROGRESS_INTERVAL`] until the -/// download finishes or is cancelled. -fn spawn_progress_heartbeat(progress: Arc) { - thread::spawn(move || { - while !progress.finished.load(Ordering::Relaxed) && !progress.cancelled.load(Ordering::Relaxed) { - let downloaded = progress.downloaded.load(Ordering::Relaxed); - let total = progress.total.load(Ordering::Relaxed); - let progress = Arc::clone(&progress); - wxdragon::call_after(Box::new(move || update_progress_dialog(&progress, downloaded, total))); - wxdragon::wake_up_idle(); - thread::sleep(PROGRESS_INTERVAL); - } - }); +/// Downloads and verifies the update, moving the download window's gauge as it goes. +fn fetch(config: &UpdaterConfig, result: &UpdateAvailableResult, progress: &Progress) -> Result { + download_update_file(config, &result.download_url, &result.signature_url, progress.cancel_flag(), |done, total| { + progress.set(done, Some(total)); + }) } -fn update_progress_dialog(progress: &DownloadProgress, downloaded: u64, total: u64) { - ACTIVE_PROGRESS.with(|p| { - let Some(update) = p.begin_update() else { - return; - }; - let dialog = update.dialog(); - #[cfg(target_os = "macos")] - let preparing = progress.preparing.load(Ordering::Relaxed); - #[cfg(not(target_os = "macos"))] - let preparing = false; - let keep_going = if preparing { - dialog.pulse(Some(&t("Preparing update..."))) - } else { - download_percent(downloaded, total) - .map_or_else(|| dialog.pulse(None), |percent| dialog.update(percent, None)) - }; - if !keep_going { - progress.cancelled.store(true, Ordering::Relaxed); - p.clear(); +/// Prepares the downloaded update for installation, while the download window pulses. +#[cfg(target_os = "macos")] +fn prepare( + config: &UpdaterConfig, + download: Result, + progress: &Progress, +) -> Result { + if progress.is_cancelled() { + if let Ok(ref path) = download { + platform::remove_private_download(path); } - // Dropping `update` invokes any completion queued during the native event-loop yield, - // after Update/Pulse returns and its reference to the dialog is released. - }); + Err(t("Update cancelled.")) + } else { + download.map_err(|e| format!("{}: {e}", t("Update failed"))).and_then(|path| { + progress.set_message(&t("Preparing update...")); + progress.set(0, None); + let result = platform::install_with_cancel(config, &path, progress.cancel_flag()); + // A manual fallback still needs its DMG. Automatic preparation has copied the + // bundle, so its private verified download can be removed. + if !matches!(result, Ok(InstallOutcome::ManualStep(_))) { + platform::remove_private_download(&path); + } + result + }) + } } fn install_update(parent: ParentWindow, outcome: Result, on_exit: Box) { diff --git a/src/ui/progress.rs b/src/ui/progress.rs deleted file mode 100644 index 2fded34..0000000 --- a/src/ui/progress.rs +++ /dev/null @@ -1,127 +0,0 @@ -//! wx progress updates yield to the event loop. Keep dialog ownership and completion separate -//! so a nested callback cannot destroy a dialog, or borrow its storage, during Update/Pulse. -use std::{ - cell::{Cell, RefCell}, - rc::Rc, -}; - -pub(super) struct ProgressLifecycle { - dialog: RefCell>>, - updating: Cell, - completion: RefCell>>, -} - -impl ProgressLifecycle { - pub(super) const fn new() -> Self { - Self { dialog: RefCell::new(None), updating: Cell::new(false), completion: RefCell::new(None) } - } - - pub(super) fn set(&self, dialog: D) { - *self.dialog.borrow_mut() = Some(Rc::new(dialog)); - } - - pub(super) fn clear(&self) { - // Native dialog destruction can process events too. Release the borrow before dropping. - let dialog = self.dialog.borrow_mut().take(); - drop(dialog); - } - - pub(super) fn begin_update(&self) -> Option> { - if self.updating.get() { - return None; - } - let dialog = self.dialog.borrow().as_ref().map(Rc::clone)?; - self.updating.set(true); - Some(ProgressUpdate { state: self, dialog: Some(dialog) }) - } - - pub(super) fn finish(&self, completion: Box) { - if self.updating.get() { - *self.completion.borrow_mut() = Some(completion); - } else { - completion(); - } - } -} - -pub(super) struct ProgressUpdate<'a, D> { - state: &'a ProgressLifecycle, - dialog: Option>, -} - -impl ProgressUpdate<'_, D> { - pub(super) fn dialog(&self) -> &D { - self.dialog.as_ref().expect("progress update owns its dialog") - } -} - -impl Drop for ProgressUpdate<'_, D> { - fn drop(&mut self) { - // Native Update/Pulse has returned. Release its reference before handing off shutdown. - drop(self.dialog.take()); - self.state.updating.set(false); - let completion = self.state.completion.borrow_mut().take(); - if let Some(completion) = completion { - completion(); - } - } -} - -/// Keep the gauge below its native completion value until preparation and verification finish. -/// `Update(100)` without `AutoHide` opens a nested modal loop and waits for the user to close it. -pub(super) fn download_percent(downloaded: u64, total: u64) -> Option { - downloaded.saturating_mul(100).checked_div(total).map(|value| i32::try_from(value.min(99)).unwrap_or(99)) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn completed_download_never_enters_native_completion_loop() { - assert_eq!(download_percent(50, 100), Some(50)); - assert_eq!(download_percent(100, 100), Some(99)); - assert_eq!(download_percent(200, 100), Some(99)); - assert_eq!(download_percent(u64::MAX, 1), Some(99)); - assert_eq!(download_percent(0, 0), None); - } - - #[test] - fn nested_completion_waits_for_native_update_to_return_and_destroy_dialog() { - struct Dialog(Rc>>); - impl Drop for Dialog { - fn drop(&mut self) { - self.0.borrow_mut().push("destroyed"); - } - } - let events = Rc::new(RefCell::new(Vec::new())); - let state = Rc::new(ProgressLifecycle::new()); - state.set(Dialog(Rc::clone(&events))); - let guard = state.begin_update().unwrap(); - assert!(state.begin_update().is_none()); - let complete_state = Rc::clone(&state); - let complete_events = Rc::clone(&events); - state.finish(Box::new(move || { - complete_state.clear(); - complete_events.borrow_mut().push("shutdown"); - })); - assert!(events.borrow().is_empty()); - // A reentrant callback can borrow the storage while Update/Pulse is active. - assert!(state.dialog.try_borrow_mut().is_ok()); - let _ = guard.dialog(); - drop(guard); - assert_eq!(*events.borrow(), ["destroyed", "shutdown"]); - assert!(state.begin_update().is_none()); - } - - #[test] - fn cancellation_keeps_native_dialog_alive_until_update_returns() { - let state = ProgressLifecycle::new(); - state.set(42); - let guard = state.begin_update().unwrap(); - state.clear(); - assert_eq!(*guard.dialog(), 42); - drop(guard); - assert!(state.begin_update().is_none()); - } -}