From c7b85518abdd24421943501eb5eae5001902cf85 Mon Sep 17 00:00:00 2001 From: Luke Street Date: Sat, 28 Sep 2024 12:13:46 -0600 Subject: [PATCH] Rework jobs view & error handling improvements Job status is now shown in the top menu bar, with a new Jobs window that can be toggled. Build and diff errors are now handled more gracefully. Fixes #40 --- objdiff-gui/src/app.rs | 10 +- objdiff-gui/src/jobs/mod.rs | 12 +- objdiff-gui/src/jobs/objdiff.rs | 95 +++++++++----- objdiff-gui/src/views/jobs.rs | 180 +++++++++++++++++++++------ objdiff-gui/src/views/symbol_diff.rs | 14 ++- 5 files changed, 233 insertions(+), 78 deletions(-) diff --git a/objdiff-gui/src/app.rs b/objdiff-gui/src/app.rs index 0ca6e5e..fbd3afd 100644 --- a/objdiff-gui/src/app.rs +++ b/objdiff-gui/src/app.rs @@ -40,7 +40,7 @@ use crate::{ frame_history::FrameHistory, function_diff::function_diff_ui, graphics::{graphics_window, GraphicsConfig, GraphicsViewState}, - jobs::jobs_ui, + jobs::{jobs_menu_ui, jobs_window}, rlwinm::{rlwinm_decode_window, RlwinmDecodeViewState}, symbol_diff::{symbol_diff_ui, DiffViewState, View}, }, @@ -62,6 +62,7 @@ pub struct ViewState { pub show_arch_config: bool, pub show_debug: bool, pub show_graphics: bool, + pub show_jobs: bool, } /// The configuration for a single object file. @@ -483,6 +484,7 @@ impl eframe::App for App { show_arch_config, show_debug, show_graphics, + show_jobs, } = view_state; frame_history.on_new_frame(ctx.input(|i| i.time), frame.info().cpu_usage); @@ -598,6 +600,10 @@ impl eframe::App for App { state.queue_reload = true; } }); + ui.separator(); + if jobs_menu_ui(ui, jobs, appearance) { + *show_jobs = !*show_jobs; + } }); }); @@ -618,7 +624,6 @@ impl eframe::App for App { egui::SidePanel::left("side_panel").show(ctx, |ui| { egui::ScrollArea::both().show(ui, |ui| { config_ui(ui, state, show_project_config, config_state, appearance); - jobs_ui(ui, jobs, appearance); }); }); @@ -634,6 +639,7 @@ impl eframe::App for App { arch_config_window(ctx, state, show_arch_config, appearance); debug_window(ctx, show_debug, frame_history, appearance); graphics_window(ctx, show_graphics, frame_history, graphics_state, appearance); + jobs_window(ctx, show_jobs, jobs, appearance); self.post_update(ctx); } diff --git a/objdiff-gui/src/jobs/mod.rs b/objdiff-gui/src/jobs/mod.rs index f339eff..85ff3f5 100644 --- a/objdiff-gui/src/jobs/mod.rs +++ b/objdiff-gui/src/jobs/mod.rs @@ -85,12 +85,15 @@ impl JobQueue { /// Clears all finished jobs. pub fn clear_finished(&mut self) { self.jobs.retain(|job| { - !(job.should_remove - && job.handle.is_none() - && job.context.status.read().unwrap().error.is_none()) + !(job.handle.is_none() && job.context.status.read().unwrap().error.is_none()) }); } + /// Clears all errored jobs. + pub fn clear_errored(&mut self) { + self.jobs.retain(|job| job.context.status.read().unwrap().error.is_none()); + } + /// Removes a job from the queue given its ID. pub fn remove(&mut self, id: usize) { self.jobs.retain(|job| job.id != id); } } @@ -107,7 +110,6 @@ pub struct JobState { pub handle: Option>, pub context: JobContext, pub cancel: Sender<()>, - pub should_remove: bool, } #[derive(Default)] @@ -163,7 +165,7 @@ fn start_job( }); let id = JOB_ID.fetch_add(1, Ordering::Relaxed); log::info!("Started job {}", id); - JobState { id, kind, handle: Some(handle), context, cancel: tx, should_remove: true } + JobState { id, kind, handle: Some(handle), context, cancel: tx } } fn update_status( diff --git a/objdiff-gui/src/jobs/objdiff.rs b/objdiff-gui/src/jobs/objdiff.rs index 23649ae..60bcd6d 100644 --- a/objdiff-gui/src/jobs/objdiff.rs +++ b/objdiff-gui/src/jobs/objdiff.rs @@ -4,7 +4,7 @@ use std::{ sync::mpsc::Receiver, }; -use anyhow::{anyhow, Context, Error, Result}; +use anyhow::{anyhow, Error, Result}; use objdiff_core::{ diff::{diff_objs, DiffObjConfig, ObjDiff}, obj::{read, ObjInfo}, @@ -189,36 +189,46 @@ fn run_build( None }; - let mut total = 3; + let mut total = 1; if config.build_target && target_path_rel.is_some() { total += 1; } if config.build_base && base_path_rel.is_some() { total += 1; } - let first_status = match target_path_rel { + if target_path_rel.is_some() { + total += 1; + } + if base_path_rel.is_some() { + total += 1; + } + + let mut step_idx = 0; + let mut first_status = match target_path_rel { Some(target_path_rel) if config.build_target => { update_status( context, format!("Building target {}", target_path_rel.display()), - 0, + step_idx, total, &cancel, )?; + step_idx += 1; run_make(&config.build_config, target_path_rel) } _ => BuildStatus::default(), }; - let second_status = match base_path_rel { + let mut second_status = match base_path_rel { Some(base_path_rel) if config.build_base => { update_status( context, format!("Building base {}", base_path_rel.display()), - 0, + step_idx, total, &cancel, )?; + step_idx += 1; run_make(&config.build_config, base_path_rel) } _ => BuildStatus::default(), @@ -226,44 +236,71 @@ fn run_build( let time = OffsetDateTime::now_utc(); - let first_obj = - match &obj_config.target_path { - Some(target_path) if first_status.success => { - update_status( - context, - format!("Loading target {}", target_path_rel.unwrap().display()), - 2, - total, - &cancel, - )?; - Some(read::read(target_path, &config.diff_obj_config).with_context(|| { - format!("Failed to read object '{}'", target_path.display()) - })?) + let first_obj = match &obj_config.target_path { + Some(target_path) if first_status.success => { + update_status( + context, + format!("Loading target {}", target_path_rel.unwrap().display()), + step_idx, + total, + &cancel, + )?; + step_idx += 1; + match read::read(target_path, &config.diff_obj_config) { + Ok(obj) => Some(obj), + Err(e) => { + first_status = BuildStatus { + success: false, + stdout: format!("Loading object '{}'", target_path.display()), + stderr: format!("{:#}", e), + ..Default::default() + }; + None + } } - _ => None, - }; + } + Some(_) => { + step_idx += 1; + None + } + _ => None, + }; let second_obj = match &obj_config.base_path { Some(base_path) if second_status.success => { update_status( context, format!("Loading base {}", base_path_rel.unwrap().display()), - 3, + step_idx, total, &cancel, )?; - Some( - read::read(base_path, &config.diff_obj_config) - .with_context(|| format!("Failed to read object '{}'", base_path.display()))?, - ) + step_idx += 1; + match read::read(base_path, &config.diff_obj_config) { + Ok(obj) => Some(obj), + Err(e) => { + second_status = BuildStatus { + success: false, + stdout: format!("Loading object '{}'", base_path.display()), + stderr: format!("{:#}", e), + ..Default::default() + }; + None + } + } + } + Some(_) => { + step_idx += 1; + None } _ => None, }; - update_status(context, "Performing diff".to_string(), 4, total, &cancel)?; + update_status(context, "Performing diff".to_string(), step_idx, total, &cancel)?; + step_idx += 1; let result = diff_objs(&config.diff_obj_config, first_obj.as_ref(), second_obj.as_ref(), None)?; - update_status(context, "Complete".to_string(), total, total, &cancel)?; + update_status(context, "Complete".to_string(), step_idx, total, &cancel)?; Ok(Box::new(ObjDiffResult { first_status, second_status, @@ -274,7 +311,7 @@ fn run_build( } pub fn start_build(ctx: &egui::Context, config: ObjDiffConfig) -> JobState { - start_job(ctx, "Object diff", Job::ObjDiff, move |context, cancel| { + start_job(ctx, "Build", Job::ObjDiff, move |context, cancel| { run_build(&context, cancel, config).map(|result| JobResult::ObjDiff(Some(result))) }) } diff --git a/objdiff-gui/src/views/jobs.rs b/objdiff-gui/src/views/jobs.rs index fc1f61e..f6fc2d5 100644 --- a/objdiff-gui/src/views/jobs.rs +++ b/objdiff-gui/src/views/jobs.rs @@ -1,58 +1,162 @@ +use std::cmp::Ordering; + use egui::{ProgressBar, RichText, Widget}; -use crate::{jobs::JobQueue, views::appearance::Appearance}; +use crate::{ + jobs::{JobQueue, JobStatus}, + views::appearance::Appearance, +}; pub fn jobs_ui(ui: &mut egui::Ui, jobs: &mut JobQueue, appearance: &Appearance) { - ui.label("Jobs"); + if ui.button("Clear").clicked() { + jobs.clear_errored(); + } let mut remove_job: Option = None; + let mut any_jobs = false; for job in jobs.iter_mut() { let Ok(status) = job.context.status.read() else { continue; }; - ui.group(|ui| { - ui.horizontal(|ui| { - ui.label(&status.title); - if ui.small_button("✖").clicked() { - if job.handle.is_some() { - job.should_remove = true; - if let Err(e) = job.cancel.send(()) { - log::error!("Failed to cancel job: {e:?}"); - } - } else { - remove_job = Some(job.id); + any_jobs = true; + ui.separator(); + ui.horizontal(|ui| { + ui.label(&status.title); + if ui.small_button("✖").clicked() { + if job.handle.is_some() { + if let Err(e) = job.cancel.send(()) { + log::error!("Failed to cancel job: {e:?}"); } - } - }); - let mut bar = ProgressBar::new(status.progress_percent); - if let Some(items) = &status.progress_items { - bar = bar.text(format!("{} / {}", items[0], items[1])); - } - bar.ui(ui); - const STATUS_LENGTH: usize = 80; - if let Some(err) = &status.error { - let err_string = format!("{:#}", err); - ui.colored_label( - appearance.delete_color, - if err_string.len() > STATUS_LENGTH - 10 { - format!("Error: {}…", &err_string[0..STATUS_LENGTH - 10]) - } else { - format!("Error: {:width$}", err_string, width = STATUS_LENGTH - 7) - }, - ) - .on_hover_text_at_pointer(RichText::new(err_string).color(appearance.delete_color)); - } else { - ui.label(if status.status.len() > STATUS_LENGTH - 3 { - format!("{}…", &status.status[0..STATUS_LENGTH - 3]) } else { - format!("{:width$}", &status.status, width = STATUS_LENGTH) - }) - .on_hover_text_at_pointer(&status.status); + remove_job = Some(job.id); + } } }); + let mut bar = ProgressBar::new(status.progress_percent); + if let Some(items) = &status.progress_items { + bar = bar.text(format!("{} / {}", items[0], items[1])); + } + bar.ui(ui); + const STATUS_LENGTH: usize = 80; + if let Some(err) = &status.error { + let err_string = format!("{:#}", err); + ui.colored_label( + appearance.delete_color, + if err_string.len() > STATUS_LENGTH - 10 { + format!("Error: {}…", &err_string[0..STATUS_LENGTH - 10]) + } else { + format!("Error: {:width$}", err_string, width = STATUS_LENGTH - 7) + }, + ) + .on_hover_text_at_pointer(RichText::new(&err_string).color(appearance.delete_color)) + .context_menu(|ui| { + if ui.button("Copy full message").clicked() { + ui.output_mut(|o| o.copied_text = err_string); + } + }); + } else { + ui.label(if status.status.len() > STATUS_LENGTH - 3 { + format!("{}…", &status.status[0..STATUS_LENGTH - 3]) + } else { + format!("{:width$}", &status.status, width = STATUS_LENGTH) + }) + .on_hover_text_at_pointer(&status.status) + .context_menu(|ui| { + if ui.button("Copy full message").clicked() { + ui.output_mut(|o| o.copied_text = status.status.clone()); + } + }); + } + } + if !any_jobs { + ui.label("No jobs"); } if let Some(idx) = remove_job { jobs.remove(idx); } } + +struct JobStatusDisplay { + title: String, + progress_items: Option<[u32; 2]>, + error: bool, +} + +impl From<&JobStatus> for JobStatusDisplay { + fn from(status: &JobStatus) -> Self { + Self { + title: status.title.clone(), + progress_items: status.progress_items, + error: status.error.is_some(), + } + } +} + +pub fn jobs_menu_ui(ui: &mut egui::Ui, jobs: &mut JobQueue, appearance: &Appearance) -> bool { + ui.label("Jobs:"); + let mut statuses = Vec::new(); + for job in jobs.iter_mut() { + let Ok(status) = job.context.status.read() else { + continue; + }; + statuses.push(JobStatusDisplay::from(&*status)); + } + let running_jobs = statuses.iter().filter(|s| !s.error).count(); + let error_jobs = statuses.iter().filter(|s| s.error).count(); + + let mut clicked = false; + let spinner = + egui::Spinner::new().size(appearance.ui_font.size * 0.9).color(appearance.text_color); + match running_jobs.cmp(&1) { + Ordering::Equal => { + spinner.ui(ui); + let running_job = statuses.iter().find(|s| !s.error).unwrap(); + let text = if let Some(items) = running_job.progress_items { + format!("{} ({}/{})", running_job.title, items[0], items[1]) + } else { + running_job.title.clone() + }; + clicked |= ui.link(RichText::new(text)).clicked(); + } + Ordering::Greater => { + spinner.ui(ui); + clicked |= ui.link(format!("{} running", running_jobs)).clicked(); + } + _ => (), + } + match error_jobs.cmp(&1) { + Ordering::Equal => { + let error_job = statuses.iter().find(|s| s.error).unwrap(); + clicked |= ui + .link( + RichText::new(format!("{} error", error_job.title)) + .color(appearance.delete_color), + ) + .clicked(); + } + Ordering::Greater => { + clicked |= ui + .link( + RichText::new(format!("{} errors", error_jobs)).color(appearance.delete_color), + ) + .clicked(); + } + _ => (), + } + if running_jobs == 0 && error_jobs == 0 { + clicked |= ui.link("None").clicked(); + } + clicked +} + +pub fn jobs_window( + ctx: &egui::Context, + show: &mut bool, + jobs: &mut JobQueue, + appearance: &Appearance, +) { + egui::Window::new("Jobs").open(show).show(ctx, |ui| { + jobs_ui(ui, jobs, appearance); + }); +} diff --git a/objdiff-gui/src/views/symbol_diff.rs b/objdiff-gui/src/views/symbol_diff.rs index 5107147..3e19b58 100644 --- a/objdiff-gui/src/views/symbol_diff.rs +++ b/objdiff-gui/src/views/symbol_diff.rs @@ -452,7 +452,7 @@ fn symbol_list_ui( fn build_log_ui(ui: &mut Ui, status: &BuildStatus, appearance: &Appearance) { ScrollArea::both().auto_shrink([false, false]).show(ui, |ui| { ui.horizontal(|ui| { - if ui.button("Copy command").clicked() { + if !status.cmdline.is_empty() && ui.button("Copy command").clicked() { ui.output_mut(|output| output.copied_text.clone_from(&status.cmdline)); } if ui.button("Copy log").clicked() { @@ -465,9 +465,15 @@ fn build_log_ui(ui: &mut Ui, status: &BuildStatus, appearance: &Appearance) { ui.style_mut().override_text_style = Some(egui::TextStyle::Monospace); ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); - ui.label(&status.cmdline); - ui.colored_label(appearance.replace_color, &status.stdout); - ui.colored_label(appearance.delete_color, &status.stderr); + if !status.cmdline.is_empty() { + ui.label(&status.cmdline); + } + if !status.stdout.is_empty() { + ui.colored_label(appearance.replace_color, &status.stdout); + } + if !status.stderr.is_empty() { + ui.colored_label(appearance.delete_color, &status.stderr); + } }); }); }