From 5f7402717681ec12c0e6ff61cc4bf263f84bf59a Mon Sep 17 00:00:00 2001 From: RealtaNua Date: Sat, 10 Oct 2026 12:29:55 -0400 Subject: [PATCH] Keep the verified download when the install step fails A download that passed its SHA-256 check was deleted whether or not the install succeeded, so every retry downloaded it again (usually after the password prompt was dismissed). It now stays until the install succeeds, is hashed again before it is reused, and shows as "Retry install" (or "Retry update to ") with "Download again" in the menu. A dismissed system prompt now says "Install cancelled" rather than "Download cancelled". A download checked by size only is still deleted. Ported onto main from the original PR: the check thread, rewritten for ArtCraft's release feed, reports kept downloads after it marks the apps loaded; the Retry button uses the restyled widgets::button; the backend tests build on main's Shared and ask no real service; the grid test scrolls the card into view. Finished files kept after a failed install are now cleaned up after 7 days instead of 30 (paused downloads keep 30). Fixes #5 Co-Authored-By: Claude Opus 5.5 --- crates/core/src/protocol.rs | 11 ++- crates/native/src/backend.rs | 155 ++++++++++++++++++++++++++++++++--- crates/native/src/http.rs | 101 +++++++++++++++++++---- crates/native/src/install.rs | 62 +++++++++++++- crates/ui-egui/src/app.rs | 15 +++- crates/ui-egui/src/model.rs | 4 +- crates/ui-egui/src/views.rs | 18 ++++ crates/ui-egui/tests/ui.rs | 87 +++++++++++++++++++- 8 files changed, 415 insertions(+), 38 deletions(-) diff --git a/crates/core/src/protocol.rs b/crates/core/src/protocol.rs index 2e6fee7..b5ac08d 100644 --- a/crates/core/src/protocol.rs +++ b/crates/core/src/protocol.rs @@ -37,7 +37,7 @@ pub enum Command { Pause { app: String, }, - /// Stop a running download, or throw away a paused one. + /// Stop a running download, or throw away a paused one or one kept after a failed install. Cancel { app: String, }, @@ -96,6 +96,9 @@ pub struct PausedDownload { pub const PAUSED: &str = "paused"; /// The `JobFinished` error a cancelled job ends with (not a failure). pub const CANCELLED: &str = "cancelled"; +/// The `JobFinished` error of an install whose system prompt (password, Windows Installer) the +/// user dismissed after the download finished (not a failure). +pub const INSTALL_CANCELLED: &str = "install cancelled"; /// Which service an app's release information came from (the footer names them). #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)] @@ -171,6 +174,12 @@ pub enum Event { app: String, download: Option, }, + /// Whether a finished download is waiting because its install step failed; `Install` checks + /// it again and goes straight to installing. + ReadyToInstall { + app: String, + ready: bool, + }, /// A job ended: `Ok` carries a short success message, `Err` what went wrong. JobFinished { app: String, diff --git a/crates/native/src/backend.rs b/crates/native/src/backend.rs index 9f2eea5..f665a5a 100644 --- a/crates/native/src/backend.rs +++ b/crates/native/src/backend.rs @@ -169,8 +169,9 @@ impl NativeBackend { let platform = self.platform; let options = self.options(); std::thread::spawn(move || { - // Leftovers from interrupted installs or older versions of the launcher. - http::clean_downloads(&paths.downloads(), Duration::from_secs(30 * 86_400)); + // Leftovers from interrupted installs or older versions of the launcher. A paused + // download stays a month; a finished one kept after a failed install, a week. + http::clean_downloads(&paths.downloads(), Duration::from_secs(30 * 86_400), Duration::from_secs(7 * 86_400)); let snapshot = crate::detect::Snapshot::take(); let manifest = shared.manifest.lock().unwrap_or_else(PoisonError::into_inner).clone(); for app in &SUITE { @@ -213,6 +214,7 @@ impl NativeBackend { } let shared = self.shared.clone(); let paths = self.paths.clone(); + let platform = self.platform; let pre = self.options().prereleases; std::thread::spawn(move || { shared.emit(Event::CheckStarted); @@ -268,6 +270,11 @@ impl NativeBackend { shared.loaded.store(true, Ordering::SeqCst); } } + // A newer release changes which file an install would use. + let options = shared.options.lock().unwrap_or_else(PoisonError::into_inner).clone(); + for app in &SUITE { + report_partial(app, &shared, &paths, platform, &options); + } let sources = sources.into_inner().unwrap_or_else(PoisonError::into_inner); let source_used = if sources.contains(&Source::Cache) { Source::Cache @@ -463,20 +470,28 @@ fn planned_asset( pick_asset(release, &stems, platform, &kinds).map(|(a, k)| (a.clone(), k)) } -/// Tells the UI whether `app` has a paused download waiting. +/// Tells the UI whether `app` has a paused download waiting, or a finished one to install. fn report_partial(app: &CraftApp, shared: &Shared, paths: &Paths, platform: Option, options: &BackendOptions) { - let download = partial_for(app, shared, paths, platform, options).map(|(asset, done)| PausedDownload { done, total: asset.size }); + let planned = planned_download(app, shared, paths, platform, options); + let download = planned.as_ref().and_then(|(dest, size)| Some(PausedDownload { done: http::partial_bytes(dest, *size)?, total: *size })); shared.emit(Event::Paused { app: app.id.to_string(), download }); + let ready = planned.is_some_and(|(dest, size)| http::finished(&dest, size)); + shared.emit(Event::ReadyToInstall { app: app.id.to_string(), ready }); } -/// The planned download of `app` and how many bytes of it are already on disk. -fn partial_for(app: &CraftApp, shared: &Shared, paths: &Paths, platform: Option, options: &BackendOptions) -> Option<(Asset, u64)> { +/// Where the planned download of `app` is kept, and its size. +fn planned_download( + app: &CraftApp, + shared: &Shared, + paths: &Paths, + platform: Option, + options: &BackendOptions, +) -> Option<(std::path::PathBuf, u64)> { let release = shared.latest.lock().unwrap_or_else(PoisonError::into_inner).get(app.id).cloned()?; let existing = shared.installed.lock().unwrap_or_else(PoisonError::into_inner).get(app.id).cloned(); let record = shared.manifest.lock().unwrap_or_else(PoisonError::into_inner).apps.get(app.id).cloned(); let (asset, _) = planned_asset(app, &release, platform?, options, existing.as_ref(), record.as_ref())?; - let done = http::partial_bytes(&install::download_path(paths, &asset), asset.size)?; - Some((asset, done)) + artcraft_launcher_core::release::valid_file_name(&asset.name).then(|| (install::download_path(paths, &asset), asset.size)) } /// How much the feed is trusted in one check, and how freely GitHub may be asked. @@ -686,14 +701,15 @@ impl Backend for NativeBackend { let running = self.shared.jobs.lock().unwrap_or_else(PoisonError::into_inner).get(&app).cloned(); match running { Some(stop) => stop.cancel(), - // A paused download: throw its partial file away. + // A paused download, or one kept after a failed install: throw the file away. None => { if let Some(def) = CraftApp::by_id(&app) - && let Some((asset, _)) = partial_for(def, &self.shared, &self.paths, self.platform, &self.options()) + && let Some((dest, size)) = planned_download(def, &self.shared, &self.paths, self.platform, &self.options()) { - http::discard_partial(&install::download_path(&self.paths, &asset), asset.size); + http::discard(&dest, size); } - self.shared.emit(Event::Paused { app, download: None }); + self.shared.emit(Event::Paused { app: app.clone(), download: None }); + self.shared.emit(Event::ReadyToInstall { app, ready: false }); } } } @@ -724,6 +740,11 @@ impl Backend for NativeBackend { *o = options; changed }; + // Another package format means another file to download. + let options = self.options(); + for app in &SUITE { + report_partial(app, &self.shared, &self.paths, self.platform, &options); + } if changed_pre { self.check(true); } @@ -945,4 +966,114 @@ mod tests { let (_, _, used) = latest_for(photo, Some(&feed), LOADED, &github, &fresh_shared, false, false, NOW + 2 * 86_400); assert_eq!(used, Some(Source::GitHubApi)); } + + const APP: &str = "photocraft"; + + /// A PhotoCraft release with one file per platform and format, so whichever this machine + /// installs is there. + fn release(version: &str) -> Release { + let suffixes = ["-portable.zip", ".tar.gz", ".msi", ".dmg", ".AppImage", ".deb", ".rpm", ".flatpak"]; + let tokens = ["windows-x64", "windows-x86", "windows-arm64", "macos-universal", "linux-x86_64", "linux-aarch64", "freebsd-x86_64"]; + let assets = tokens + .iter() + .flat_map(|t| suffixes.iter().map(move |s| format!("{APP}-{version}-{t}{s}"))) + .map(|name| Asset { url: format!("https://example.invalid/{name}"), name, size: 3, sha256: None }) + .collect(); + Release { assets, ..Release::default() } + } + + /// A backend that knows `release("0.5.0")` and keeps its files under `root`. + fn backend(root: &std::path::Path, platform: Platform, source: Option>) -> NativeBackend { + let shared = shared(); + shared.latest.lock().unwrap().insert(APP.to_string(), release("0.5.0")); + let shared = Arc::new(shared); + NativeBackend { shared, paths: Paths::under(root), http: None, source, platform: Some(platform), self_install: None } + } + + /// Reports again, and returns what the UI was last told about a kept download. + fn ready(backend: &mut NativeBackend) -> Option { + let app = CraftApp::by_id(APP).unwrap(); + report_partial(app, &backend.shared, &backend.paths, backend.platform, &BackendOptions::default()); + last_ready(backend.poll()) + } + + fn last_ready(events: Vec) -> Option { + events.into_iter().rev().find_map(|e| match e { + Event::ReadyToInstall { app, ready } if app == APP => Some(ready), + _ => None, + }) + } + + /// Puts the planned download on disk, as a failed install leaves it. + fn keep_file(backend: &NativeBackend) -> std::path::PathBuf { + let app = CraftApp::by_id(APP).unwrap(); + let (dest, size) = planned_download(app, &backend.shared, &backend.paths, backend.platform, &BackendOptions::default()).unwrap(); + std::fs::create_dir_all(backend.paths.downloads()).unwrap(); + std::fs::write(&dest, vec![0u8; size as usize]).unwrap(); + dest + } + + #[test] + fn a_kept_download_is_reported_and_can_be_thrown_away() { + let Some(platform) = native_platform() else { return }; + let dir = tempfile::tempdir().unwrap(); + let mut backend = backend(dir.path(), platform, None); + assert_eq!(ready(&mut backend), Some(false)); + let dest = keep_file(&backend); + assert_eq!(ready(&mut backend), Some(true)); + + backend.send(Command::Cancel { app: APP.to_string() }); + assert!(!dest.exists()); + assert_eq!(last_ready(backend.poll()), Some(false)); + } + + #[test] + fn changing_the_settings_reports_kept_downloads_again() { + let Some(platform) = native_platform() else { return }; + let dir = tempfile::tempdir().unwrap(); + let mut backend = backend(dir.path(), platform, None); + keep_file(&backend); + assert_eq!(ready(&mut backend), Some(true)); + // The planned file changed under the UI; only a new report can tell it. + backend.shared.latest.lock().unwrap().insert(APP.to_string(), release("0.6.0")); + backend.send(Command::Configure(BackendOptions::default())); + assert_eq!(last_ready(backend.poll()), Some(false)); + } + + #[test] + fn a_check_that_finds_a_newer_release_ends_the_retry_offer() { + struct Newer; + impl ReleaseSource for Newer { + fn latest(&self, repo: &str, _prereleases: bool) -> crate::source::Answer { + let found = CraftApp::by_id(APP).is_some_and(|a| a.repo == repo); + let latest = if found { LatestRelease::Found(release("0.6.0")) } else { LatestRelease::None }; + crate::source::Answer { latest, source: Source::GitHubApi } + } + fn cached(&self, _repo: &str, _prereleases: bool) -> Option { + None + } + fn rate_limited_until(&self) -> Option { + None + } + fn save(&self) {} + } + let Some(platform) = native_platform() else { return }; + let dir = tempfile::tempdir().unwrap(); + let mut backend = backend(dir.path(), platform, Some(Arc::new(Newer))); + // A check needs a client; with pre-releases on it asks only `Newer`, never ArtCraft's API. + backend.http = Some(Http::new().unwrap()); + backend.shared.options.lock().unwrap().prereleases = true; + keep_file(&backend); + assert_eq!(ready(&mut backend), Some(true)); + + backend.send(Command::CheckForUpdates { force: true }); + let mut events = Vec::new(); + let deadline = Instant::now() + Duration::from_secs(20); + while !events.iter().any(|e| matches!(e, Event::CheckFinished { .. })) { + assert!(Instant::now() < deadline, "the check never finished"); + std::thread::sleep(Duration::from_millis(10)); + events.extend(backend.poll()); + } + assert_eq!(last_ready(events), Some(false)); + } } diff --git a/crates/native/src/http.rs b/crates/native/src/http.rs index b5a75e9..29faba5 100644 --- a/crates/native/src/http.rs +++ b/crates/native/src/http.rs @@ -186,8 +186,9 @@ impl Http { std::fs::create_dir_all(dir).map_err(|e| format!("couldn't create {} ({e})", dir.display()))?; } let verified = if sha256.is_some() { Verified::Checksum } else { Verified::SizeOnly }; - // A finished copy from an earlier attempt that never got installed. - if dest.is_file() && matches_published(dest, size, sha256) { + // A finished copy from an earlier attempt that never got installed. Marking it as just + // used, before checking it, keeps `clean_downloads` away from it while it's installed. + if dest.is_file() && touch(dest) && matches_published(dest, size, sha256) { progress(Transfer::Bytes { done: size, total: size }); return Ok(verified); } @@ -402,10 +403,22 @@ pub fn partial_bytes(dest: &Path, size: u64) -> Option { std::fs::metadata(part_path(dest, size)).ok().map(|m| m.len()).filter(|n| *n > 0) } -/// Removes leftovers from the download folder: files in an old or unknown naming scheme, and -/// paused downloads untouched for `max_age` (they're for releases long since replaced). Files -/// written within the last minute are always kept, so a running download is never touched. -pub fn clean_downloads(dir: &Path, max_age: Duration) { +/// Sets a file's modification time to now. +fn touch(path: &Path) -> bool { + std::fs::File::options().write(true).open(path).and_then(|f| f.set_modified(std::time::SystemTime::now())).is_ok() +} + +/// Whether a finished download is waiting at `dest`. Judged by size alone: `download` checks it +/// against the published checksum before anything uses it. +pub fn finished(dest: &Path, size: u64) -> bool { + std::fs::metadata(dest).is_ok_and(|m| m.is_file() && m.len() == size) +} + +/// Removes leftovers from the download folder: partial files in an old naming scheme, paused +/// downloads untouched for `partial_max_age`, and finished ones (kept after a failed install) +/// untouched for `finished_max_age` (they're for releases long since replaced, or given up on). +/// Files written within the last minute are always kept, so a running download is never touched. +pub fn clean_downloads(dir: &Path, partial_max_age: Duration, finished_max_age: Duration) { let Ok(entries) = std::fs::read_dir(dir) else { return }; let now = std::time::SystemTime::now(); for entry in entries.flatten() { @@ -424,16 +437,19 @@ pub fn clean_downloads(dir: &Path, max_age: Duration) { .strip_suffix(".part") .and_then(|stem| stem.rsplit_once('.')) .is_some_and(|(_, size)| !size.is_empty() && size.bytes().all(|b| b.is_ascii_digit())); - if !current_part || age > max_age { + let legacy_part = name.ends_with(".part") && !current_part; + let max_age = if name.ends_with(".part") { partial_max_age } else { finished_max_age }; + if legacy_part || age > max_age { log::info!("removing stale download {}", path.display()); let _ = std::fs::remove_file(&path); } } } -/// Throws away a paused download. -pub fn discard_partial(dest: &Path, size: u64) { +/// Throws away a paused download, and a finished one kept for another try at installing. +pub fn discard(dest: &Path, size: u64) { let _ = std::fs::remove_file(part_path(dest, size)); + let _ = std::fs::remove_file(dest); } enum AttemptError { @@ -485,9 +501,12 @@ fn host(url: &str) -> &str { } #[cfg(test)] -mod tests { +pub(crate) mod tests { use super::*; + const DAYS_30: Duration = Duration::from_secs(30 * 86_400); + const DAYS_7: Duration = Duration::from_secs(7 * 86_400); + #[test] fn cleaning_keeps_current_partial_files_and_removes_leftovers() { let dir = tempfile::tempdir().unwrap(); @@ -503,13 +522,61 @@ mod tests { let finished = make("photocraft-0.5.0-macos-universal.dmg"); let fresh = dir.path().join("x.part"); std::fs::write(&fresh, b"x").unwrap(); - clean_downloads(dir.path(), Duration::from_secs(30 * 86_400)); - assert!(keep.exists() && fresh.exists()); - assert!(!legacy.exists() && !finished.exists()); - clean_downloads(dir.path(), Duration::from_secs(60)); + clean_downloads(dir.path(), DAYS_30, DAYS_7); + assert!(keep.exists() && fresh.exists() && finished.exists()); + assert!(!legacy.exists()); + // A finished file goes sooner than a paused one. + clean_downloads(dir.path(), DAYS_30, Duration::from_secs(60)); + assert!(keep.exists() && !finished.exists()); + clean_downloads(dir.path(), Duration::from_secs(60), DAYS_7); assert!(!keep.exists()); } + #[test] + fn a_kept_file_that_still_matches_is_not_fetched_again() { + let (body, sha) = body(); + let len = body.len() as u64; + let (url, requests) = serve(body.clone(), vec![Reply::Full, Reply::Full]); + let dir = tempfile::tempdir().unwrap(); + let dest = dir.path().join("file.bin"); + let http = Http::new().unwrap(); + http.download(&url, &dest, len, Some(&sha), &Stop::default(), &mut |_| {}).unwrap(); + assert!(finished(&dest, len) && !finished(&dest, 0)); + // Old enough to be cleaned up, were it not about to be used. + let old = std::time::SystemTime::now() - Duration::from_secs(40 * 86_400); + std::fs::File::options().write(true).open(&dest).unwrap().set_modified(old).unwrap(); + let mut last = (0, 0); + let again = http.download(&url, &dest, len, Some(&sha), &Stop::default(), &mut |t| { + if let Transfer::Bytes { done, total } = t { + last = (done, total); + } + }); + assert_eq!(again, Ok(Verified::Checksum)); + assert_eq!(last, (len, len)); + assert_eq!(requests.lock().unwrap().len(), 1); + // A cleanup while the installer runs leaves the reused file alone. + clean_downloads(dir.path(), DAYS_30, DAYS_7); + assert_eq!(std::fs::read(&dest).unwrap(), body); + discard(&dest, len); + assert!(!finished(&dest, len)); + } + + #[test] + fn a_kept_file_that_no_longer_matches_is_fetched_again() { + let (body, sha) = body(); + let len = body.len() as u64; + let (url, requests) = serve(body.clone(), vec![Reply::Full, Reply::Full]); + let dir = tempfile::tempdir().unwrap(); + let dest = dir.path().join("file.bin"); + let http = Http::new().unwrap(); + http.download(&url, &dest, len, Some(&sha), &Stop::default(), &mut |_| {}).unwrap(); + // The same size, so only the checksum can tell. + std::fs::write(&dest, vec![b'x'; body.len()]).unwrap(); + http.download(&url, &dest, len, Some(&sha), &Stop::default(), &mut |_| {}).unwrap(); + assert_eq!(std::fs::read(&dest).unwrap(), body); + assert_eq!(requests.lock().unwrap().len(), 2); + } + #[test] fn reads_content_ranges() { assert_eq!(parse_content_range("bytes 100-199/1000"), Some((100, Some(1000)))); @@ -524,7 +591,7 @@ mod tests { /// How the fake server answers one request. #[derive(Clone, Copy, Debug)] - enum Reply { + pub(crate) enum Reply { /// The whole file. Full, /// Promise the whole file, send `n` bytes, hang up. @@ -539,7 +606,7 @@ mod tests { /// Serves `body` over plain HTTP following `script`, one reply per connection; records the /// `Range` header each request carried. - fn serve(body: Vec, script: Vec) -> (String, Arc>>>) { + pub(crate) fn serve(body: Vec, script: Vec) -> (String, Arc>>>) { let listener = TcpListener::bind("127.0.0.1:0").unwrap(); let url = format!("http://{}/file.bin", listener.local_addr().unwrap()); let ranges = Arc::new(Mutex::new(Vec::new())); @@ -585,7 +652,7 @@ mod tests { (url, ranges) } - fn body() -> (Vec, String) { + pub(crate) fn body() -> (Vec, String) { let body: Vec = (0..700_000u32).map(|i| (i % 251) as u8).collect(); let sha = hex(&Sha256::digest(&body)); (body, sha) diff --git a/crates/native/src/install.rs b/crates/native/src/install.rs index f82f154..1e10e7c 100644 --- a/crates/native/src/install.rs +++ b/crates/native/src/install.rs @@ -14,7 +14,7 @@ use std::path::{Path, PathBuf}; -use artcraft_launcher_core::protocol::JobPhase; +use artcraft_launcher_core::protocol::{CANCELLED, INSTALL_CANCELLED, JobPhase}; use artcraft_launcher_core::release::{self, Asset, Release}; use artcraft_launcher_core::{CraftApp, InstallSource, Installed, PackageKind}; @@ -67,9 +67,18 @@ pub fn install( PackageKind::TarGz => freebsd::install_tree(app, &file, job.paths).map(|(bin, files, root)| Some(record(&version, kind, bin, files, root))), other => Err(format!("{} isn't supported on this system", other.label())), }; - // The download has served its purpose either way; a failed install starts fresh next time. - let _ = std::fs::remove_file(&file); - result.map(|r| (r, verified)) + if !keep_download(result.is_ok(), verified) { + let _ = std::fs::remove_file(&file); + } + // The system's own prompt was dismissed: the download itself wasn't cancelled. + result.map(|r| (r, verified)).map_err(|e| if e == CANCELLED { INSTALL_CANCELLED.to_string() } else { e }) +} + +/// Whether the download stays on disk once the install step has ended. A failed install keeps it: +/// the next try checks it against its published checksum again and skips the transfer. Without a +/// checksum there's nothing to check it against later, so it goes. +fn keep_download(installed: bool, verified: Verified) -> bool { + !installed && verified == Verified::Checksum } /// Where `asset` is downloaded to (and its partial file kept while unfinished). @@ -710,6 +719,51 @@ mod tests { assert_eq!(std::fs::read_to_string(dest.join("v")).unwrap(), "new"); } + #[test] + fn only_a_failed_install_of_a_checksummed_download_keeps_it() { + for (installed, verified, keep) in [ + (false, Verified::Checksum, true), + (false, Verified::SizeOnly, false), + (true, Verified::Checksum, false), + (true, Verified::SizeOnly, false), + ] { + assert_eq!(keep_download(installed, verified), keep, "installed: {installed}, {verified:?}"); + } + } + + #[test] + fn a_failed_install_keeps_the_download_for_the_next_try() { + use crate::http::tests::{Reply, body, serve}; + let app = CraftApp::by_id("wordcraft").unwrap(); + let (body, sha) = body(); + let (url, requests) = serve(body.clone(), vec![Reply::Full, Reply::Full]); + let asset = Asset { name: "wordcraft-0.3.0-test.pkg".into(), size: body.len() as u64, url, sha256: Some(sha) }; + let release = Release { assets: vec![asset.clone()], ..Release::default() }; + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let http = Http::new().unwrap(); + // A format no build of the launcher installs here stands in for any failing install step. + let kind = if cfg!(windows) { PackageKind::Dmg } else { PackageKind::Msi }; + let mut phases = Vec::new(); + for _ in 0..2 { + let mut phase = |p: JobPhase| phases.push(p); + let mut job = Job { http: &http, paths: &paths, stop: &Stop::default(), phase: &mut phase }; + let err = install(app, &release, &asset, kind, None, &mut job).unwrap_err(); + assert!(err.contains("isn't supported"), "{err}"); + assert_eq!(std::fs::read(download_path(&paths, &asset)).unwrap(), body); + } + assert_eq!(requests.lock().unwrap().len(), 1, "the second try reuses the kept file"); + assert_eq!(phases.iter().filter(|p| **p == JobPhase::Installing).count(), 2); + + // Checked by size only: not kept. + let unchecked = Asset { name: "wordcraft-0.3.1-test.pkg".into(), sha256: None, ..asset }; + let mut phase = |_: JobPhase| {}; + let mut job = Job { http: &http, paths: &paths, stop: &Stop::default(), phase: &mut phase }; + install(app, &release, &unchecked, kind, None, &mut job).unwrap_err(); + assert!(!download_path(&paths, &unchecked).exists()); + assert_eq!(requests.lock().unwrap().len(), 2); + } + #[cfg(unix)] fn tarball(path: &Path, entries: &[&str]) { let f = std::fs::File::create(path).unwrap(); diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index 451841f..146d6db 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -2,7 +2,9 @@ use std::time::Duration; -use artcraft_launcher_core::protocol::{Backend, BackendOptions, CANCELLED, Capabilities, Command, Event, LatestRelease, PAUSED, Source}; +use artcraft_launcher_core::protocol::{ + Backend, BackendOptions, CANCELLED, Capabilities, Command, Event, INSTALL_CANCELLED, LatestRelease, PAUSED, Source, +}; use artcraft_launcher_core::release::Release; use artcraft_launcher_core::status::AppStatus; use artcraft_launcher_core::{Platform, SUITE}; @@ -267,6 +269,12 @@ impl LauncherUi { } false } + Event::ReadyToInstall { app, ready } => { + if let Some(e) = self.entry_mut(&app) { + e.ready = ready; + } + false + } Event::JobFinished { app, result } => { if app == artcraft_launcher_core::catalog::LAUNCHER_ID { self.launcher_job = None; @@ -274,11 +282,16 @@ impl LauncherUi { if let Some(e) = self.entry_mut(&app) { e.job = None; } + let kept = self.entry(&app).is_some_and(|e| e.ready); match result { Ok(msg) => self.toast(ctx, msg, false), // The paused card says it all. Err(e) if e == PAUSED => {} Err(e) if e == CANCELLED => self.toast(ctx, "Download cancelled", false), + Err(e) if e == INSTALL_CANCELLED && kept => { + self.toast(ctx, "Install cancelled. The download is kept for the next try.", false); + } + Err(e) if e == INSTALL_CANCELLED => self.toast(ctx, "Install cancelled", false), Err(e) => self.toast(ctx, e, true), } true diff --git a/crates/ui-egui/src/model.rs b/crates/ui-egui/src/model.rs index 24e84e9..1a8a34c 100644 --- a/crates/ui-egui/src/model.rs +++ b/crates/ui-egui/src/model.rs @@ -18,11 +18,13 @@ pub struct AppEntry { pub origin: Option, /// A download waiting to be resumed. pub paused: Option, + /// A finished download waiting for another try at installing it. + pub ready: bool, } impl AppEntry { pub fn new(app: &'static CraftApp) -> Self { - Self { app, latest: None, installed: None, detected: false, job: None, origin: None, paused: None } + Self { app, latest: None, installed: None, detected: false, job: None, origin: None, paused: None, ready: false } } pub fn release(&self) -> Option<&Release> { diff --git a/crates/ui-egui/src/views.rs b/crates/ui-egui/src/views.rs index cb4a9a8..59371e7 100644 --- a/crates/ui-egui/src/views.rs +++ b/crates/ui-egui/src/views.rs @@ -16,6 +16,8 @@ use crate::widgets::{self, Cell, Kind}; /// What the main button of an app does. enum Primary { Install, + /// The download is already here; only the install step is left. Carries the button's label. + Retry(String), Update(String), Open, Download(String), @@ -582,6 +584,8 @@ impl LauncherUi { }; } match status { + AppStatus::Available if entry.ready => Primary::Retry("Retry install".into()), + AppStatus::UpdateAvailable if entry.ready => Primary::Retry(format!("Retry update to {}", entry.latest_label().unwrap_or_default())), AppStatus::Available => Primary::Install, AppStatus::UpdateAvailable => Primary::Update(entry.latest_label().unwrap_or_default()), AppStatus::UpToDate | AppStatus::Installed => Primary::Open, @@ -674,6 +678,12 @@ impl LauncherUi { self.send(Command::Install { app: id.clone() }); } } + Primary::Retry(label) => { + let tip = "The download is still here. It's checked again, then installed."; + if widgets::button(ui, &label, Kind::Primary, main_w).on_hover_text(tip).clicked() { + self.send(Command::Install { app: id.clone() }); + } + } Primary::Update(v) => { if widgets::button(ui, &format!("Update to {v}"), Kind::Primary, main_w).clicked() { self.send(Command::Install { app: id.clone() }); @@ -709,6 +719,14 @@ impl LauncherUi { self.dialog = Some(Dialog::Notes(id.clone())); ui.close(); } + // Only where Install would download: an up-to-date app would be reinstalled. + let can_download_again = + manage && entry.ready && entry.paused.is_none() && matches!(status, AppStatus::Available | AppStatus::UpdateAvailable); + if can_download_again && ui.button("Download again").clicked() { + self.send(Command::Cancel { app: id.clone() }); + self.send(Command::Install { app: id.clone() }); + ui.close(); + } if manage && status.is_installed() { if ui.button("Open").clicked() { self.send(Command::Launch { app: id.clone() }); diff --git a/crates/ui-egui/tests/ui.rs b/crates/ui-egui/tests/ui.rs index ba72731..3c81746 100644 --- a/crates/ui-egui/tests/ui.rs +++ b/crates/ui-egui/tests/ui.rs @@ -6,7 +6,9 @@ use std::cell::{Cell, RefCell}; use std::rc::Rc; -use artcraft_launcher_core::protocol::{Backend, Capabilities, Command, Event, JobPhase, LatestRelease, Origin, PausedDownload, Source}; +use artcraft_launcher_core::protocol::{ + Backend, CANCELLED, Capabilities, Command, Event, INSTALL_CANCELLED, JobPhase, LatestRelease, Origin, PausedDownload, Source, +}; use artcraft_launcher_core::release::{Asset, Release}; use artcraft_launcher_core::status::InstallSource; use artcraft_launcher_core::{Arch, Installed, Os, Platform, SUITE, version}; @@ -101,6 +103,7 @@ fn events() -> Vec { ev.push(Event::Progress { app: "filmcraft".into(), phase: JobPhase::Downloading { done: 48 << 20, total: 131 << 20 } }); ev.push(Event::Paused { app: "deckcraft".into(), download: Some(PausedDownload { done: 30 << 20, total: 120 << 20 }) }); ev.push(Event::Progress { app: "gridcraft".into(), phase: JobPhase::Reconnecting { done: 10 << 20, total: 90 << 20 } }); + ev.push(Event::ReadyToInstall { app: "cadcraft".into(), ready: true }); ev.push(Event::CheckFinished { at: NOW - 180, source: Source::GitHubApi, rate_limited_until: None }); ev } @@ -110,9 +113,19 @@ fn harness(view: ViewMode, theme: egui::Theme, manage: bool) -> (Harness<'static } fn harness_at(view: ViewMode, theme: egui::Theme, manage: bool, scale: f32) -> (Harness<'static, Option>, Rc>>) { + harness_with(events(), view, theme, manage, scale) +} + +fn harness_with( + events: Vec, + view: ViewMode, + theme: egui::Theme, + manage: bool, + scale: f32, +) -> (Harness<'static, Option>, Rc>>) { let sent = Rc::new(RefCell::new(Vec::new())); let sent2 = sent.clone(); - let mut backend = Some(Fake { events: events(), sent: sent2, manage, clock: Rc::new(Cell::new(NOW)) }); + let mut backend = Some(Fake { events, sent: sent2, manage, clock: Rc::new(Cell::new(NOW)) }); let settings = Settings { view, notify: false, ..Settings::default() }; // No `.wgpu()`: it gives every test its own GPU device, and those segfault on parallel test threads (see above). // The default renderer creates one only when a test renders. @@ -222,6 +235,76 @@ fn downloads_can_be_paused_resumed_and_discarded() { assert!(sent.borrow().iter().any(|c| matches!(c, Command::Pause { .. }))); } +#[test] +fn a_kept_download_offers_retry_install_and_download_again() { + let (mut h, sent) = harness(ViewMode::Grid, egui::Theme::Dark, true); + h.run_steps(4); + if let Some(l) = h.state_mut().as_mut() { + l.settings.filter = artcraft_launcher_ui_egui::settings::Filter::NotInstalled; + } + h.run_steps(6); + // Below the fold in the restyled grid: bring the card on screen first. + h.get_by_label("Retry install").scroll_to_me(); + h.run_steps(4); + snapshot(&mut h, "grid-retry-install-dark"); + sent.borrow_mut().clear(); + let retry = h.get_by_label("Retry install"); + let row = retry.rect(); + retry.click(); + h.run_steps(2); + assert_eq!(*sent.borrow(), [Command::Install { app: "cadcraft".into() }]); + sent.borrow_mut().clear(); + // The … menu beside it. + let beside = |r: egui::Rect| row.y_range().contains(r.center().y) && (row.right()..row.right() + 60.0).contains(&r.left()); + h.get_all_by_label("…").find(|n| beside(n.rect())).unwrap().click(); + h.run_steps(3); + snapshot(&mut h, "grid-retry-install-menu-dark"); + h.get_by_label("Download again").click(); + h.run_steps(2); + assert_eq!(*sent.borrow(), [Command::Cancel { app: "cadcraft".into() }, Command::Install { app: "cadcraft".into() }]); + // Apps without a kept download don't offer it. + assert_eq!(h.query_all_by_label("Retry install").count(), 1); +} + +#[test] +fn a_dismissed_install_prompt_is_not_called_a_cancelled_download() { + let finished = |app: &str, why: &str| Event::JobFinished { app: app.into(), result: Err(why.into()) }; + // The password prompt was dismissed with the download kept (cadcraft) and without (pdfcraft); + // a download was cancelled (vectorcraft). + let mut ev = events(); + ev.push(finished("cadcraft", INSTALL_CANCELLED)); + ev.push(finished("pdfcraft", INSTALL_CANCELLED)); + ev.push(finished("vectorcraft", CANCELLED)); + let (mut h, _) = harness_with(ev, ViewMode::Grid, egui::Theme::Dark, true, 1.0); + h.run_steps(6); + snapshot(&mut h, "grid-install-cancelled-dark"); + for toast in ["Install cancelled. The download is kept for the next try.", "Install cancelled", "Download cancelled"] { + assert_eq!(h.query_all_by_label(toast).count(), 1, "{toast}"); + } +} + +#[test] +fn a_kept_update_names_its_version_and_an_up_to_date_app_is_not_downloaded_again() { + let mut ev = events(); + for app in ["effectcraft", "wordcraft"] { + ev.push(Event::ReadyToInstall { app: app.into(), ready: true }); + } + let (mut h, sent) = harness_with(ev, ViewMode::Table, egui::Theme::Dark, true, 1.0); + h.run_steps(8); + snapshot(&mut h, "table-retry-update-dark"); + sent.borrow_mut().clear(); + assert!(h.query_by_label("Update to 0.6.0").is_none()); + h.get_by_label("Retry update to 0.6.0").click(); + h.run_steps(2); + assert_eq!(*sent.borrow(), [Command::Install { app: "effectcraft".into() }]); + // WordCraft is up to date: its menu offers Reinstall, which uses the kept file. + let row = h.get_all_by_label("WordCraft").next().unwrap().rect().center().y; + h.get_all_by_label("…").find(|n| (n.rect().center().y - row).abs() < 25.0).unwrap().click(); + h.run_steps(3); + h.get_by_label("Reinstall"); + assert!(h.query_by_label("Download again").is_none()); +} + #[test] fn table_view_and_light_theme() { let (mut h, sent) = harness(ViewMode::Table, egui::Theme::Light, true);