From f9ae561392731b99a0454de1a65672b7d018dec0 Mon Sep 17 00:00:00 2001 From: Nathan Wells Date: Tue, 3 Nov 2026 15:05:49 -0800 Subject: [PATCH 1/6] Add craft-proof-ui: a shared spelling review for the Craft apps A panel that steps through a document's issues beside the page, with click-to-apply suggestions, Ignore Once, Ignore All, Add to Dictionary and a Change field, plus a section for the right-click menu. Apps supply their documents through a small Host trait, their translations and their theme. The document stays editable during a review, so a correction is checked against a fresh check first and changed text asks for a new pass. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/proof-ui.yml | 35 ++++ Cargo.lock | 8 + Cargo.toml | 1 + README.md | 2 + crates/proof-ui/Cargo.toml | 18 ++ crates/proof-ui/README.md | 32 +++ crates/proof-ui/src/lib.rs | 64 ++++++ crates/proof-ui/src/review.rs | 209 ++++++++++++++++++++ crates/proof-ui/src/tests.rs | 346 +++++++++++++++++++++++++++++++++ crates/proof-ui/src/ui.rs | 235 ++++++++++++++++++++++ 10 files changed, 950 insertions(+) create mode 100644 .github/workflows/proof-ui.yml create mode 100644 crates/proof-ui/Cargo.toml create mode 100644 crates/proof-ui/README.md create mode 100644 crates/proof-ui/src/lib.rs create mode 100644 crates/proof-ui/src/review.rs create mode 100644 crates/proof-ui/src/tests.rs create mode 100644 crates/proof-ui/src/ui.rs diff --git a/.github/workflows/proof-ui.yml b/.github/workflows/proof-ui.yml new file mode 100644 index 000000000..b7d2d870d --- /dev/null +++ b/.github/workflows/proof-ui.yml @@ -0,0 +1,35 @@ +name: Proof UI + +on: + push: + branches: [main] + paths: ["crates/proof-ui/**", "Cargo.toml", "Cargo.lock", ".github/workflows/proof-ui.yml"] + pull_request: + paths: ["crates/proof-ui/**", "Cargo.toml", "Cargo.lock", ".github/workflows/proof-ui.yml"] + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: proof-ui-${{ github.ref }} + cancel-in-progress: true + +jobs: + linux: + timeout-minutes: 15 + name: Proof UI (Linux) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + - uses: dtolnay/rust-toolchain@b725eda525362e7cf34af9a7243cac1e3f01a7ec # stable + with: + components: rustfmt, clippy + - uses: Swatinem/rust-cache@d1527c8f67ef9af357290475d4af4b0fab8f12e0 # v2.9.3 + with: + key: craft-proof-ui-linux + - run: cargo fmt -p craft-proof-ui --check + - run: cargo clippy -p craft-proof-ui --all-targets --locked -- -D warnings + - run: cargo test -p craft-proof-ui --locked diff --git a/Cargo.lock b/Cargo.lock index fd5e504fe..00f3e6fc4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1077,6 +1077,14 @@ dependencies = [ "unicode-normalization", ] +[[package]] +name = "craft-proof-ui" +version = "0.1.0" +dependencies = [ + "egui", + "egui_kittest", +] + [[package]] name = "craft-quit" version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml index 24e75777b..305c68139 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,6 +44,7 @@ craft-pcraft-format = { path = "crates/pcraft-format" } craft-prepress = { path = "crates/prepress" } craft-print = { path = "crates/print" } craft-proof = { path = "crates/proof" } +craft-proof-ui = { path = "crates/proof-ui" } craft-quit = { path = "crates/quit" } craft-settings = { path = "crates/settings" } craft-text = { path = "crates/text" } diff --git a/README.md b/README.md index 7747fb095..162db3d8a 100644 --- a/README.md +++ b/README.md @@ -95,6 +95,8 @@ craft- = { git = "https://github.com/storytold/craft-libs", rev = ", +} + +impl Issue { + fn same_finding(&self, other: &Issue) -> bool { + (self.story, self.start, self.end, self.kind) == (other.story, other.start, other.end, other.kind) && self.word == other.word + } +} + +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum IssueKind { + #[default] + Spelling, + /// A preferred form or a simple rule: the word is known, so it can't join the dictionary. + Grammar, +} + +/// What the review needs from the application, for its open document. +pub trait Host { + /// Every issue in the document in reading order. Suggestions may be left empty. + fn check(&mut self) -> Result, String>; + /// The candidates for one issue, best first. An error when it no longer applies. + fn suggestions(&mut self, issue: &Issue) -> Result, String>; + /// The current text of a story, `None` when it is gone. + fn text(&self, story: u64) -> Option<&str>; + /// A token that changes whenever a story's text might have, cheap enough for every frame. + fn version(&self, story: u64) -> Option; + /// Identifies the document: the review ends when it changes. + fn document(&self) -> Option; + /// Show the issue in the document, usually by selecting it. + fn reveal(&mut self, issue: &Issue); + /// Replace the issue's text, as one undoable edit. + fn change(&mut self, issue: &Issue, to: &str) -> Result<(), String>; + /// Accept the word from now on. + fn add_word(&mut self, word: &str) -> Result<(), String>; + /// A line above the review about what it can't cover (a language with no data), if any. + fn notice(&self) -> Option { + None + } +} + +#[cfg(test)] +mod tests; diff --git a/crates/proof-ui/src/review.rs b/crates/proof-ui/src/review.rs new file mode 100644 index 000000000..34a56ccd9 --- /dev/null +++ b/crates/proof-ui/src/review.rs @@ -0,0 +1,209 @@ +//! A pass over a document's issues, apart from how it is drawn. +use std::collections::{HashMap, HashSet}; + +use crate::{Host, Issue, IssueKind}; + +pub(crate) const CHANGED: &str = "The text changed. Check again to continue."; +pub(crate) const OTHER_DOCUMENT: &str = "The document changed."; +pub(crate) const SAME_REPLACEMENT: &str = "Choose a suggestion or enter a different replacement."; +pub(crate) const NO_ISSUE: &str = "No issue to review."; +pub(crate) const NOT_SPELLING: &str = "Only spelling issues can be added to the dictionary."; + +/// What can be done with the current issue. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum Action { + /// Replace it. `None` takes the typed replacement, which starts as the first suggestion. + Change(Option), + /// Skip this occurrence for this pass. + IgnoreOnce, + /// Skip this word for this pass. + IgnoreAll, + AddToDictionary, + /// Start a new pass from the top, forgetting what was ignored. + Check, +} + +/// One pass over a document. Empty until [`Review::start`]. +#[derive(Clone, Debug, Default)] +pub struct Review { + document: Option, + /// The issues left, the current one first. Only the current one has suggestions. + issues: Vec, + skipped: Vec, + ignored: HashSet, + /// [`Host::version`] of each issue's story when it was checked. + versions: HashMap, + pub(crate) before: String, + pub(crate) after: String, + /// The Change… field's text. + pub replacement: String, + pub(crate) manual: bool, + message: String, + started: bool, +} + +impl Review { + /// Check the document from the top, forgetting any earlier pass. + pub fn start(&mut self, host: &mut dyn Host) { + *self = Review { started: true, document: host.document(), ..Default::default() }; + // A failure is kept as the message. + let _ = self.report(|review| review.refresh(host)); + } + + pub fn started(&self) -> bool { + self.started + } + + pub fn current(&self) -> Option<&Issue> { + self.issues.first() + } + + pub fn remaining(&self) -> usize { + self.issues.len() + } + + pub fn ignored_any(&self) -> bool { + !self.skipped.is_empty() || !self.ignored.is_empty() + } + + /// The last failure, in English: look it up through [`crate::Text::ui`]. + pub fn message(&self) -> &str { + &self.message + } + + /// The host's document isn't the one this pass checked. + pub fn other_document(&self, host: &dyn Host) -> bool { + self.started && host.document() != self.document + } + + /// The current issue's text has changed since it was checked. + pub fn stale(&self, host: &dyn Host) -> bool { + self.current().is_some_and(|issue| host.version(issue.story).is_none_or(|v| self.versions.get(&issue.story) != Some(&v))) + } + + /// Carry out `action` on the current issue. A failure is also kept as [`Review::message`]. + pub fn act(&mut self, host: &mut dyn Host, action: Action) -> Result<(), String> { + self.report(|review| review.apply(host, action)) + } + + fn report(&mut self, run: impl FnOnce(&mut Self) -> Result<(), String>) -> Result<(), String> { + self.message.clear(); + let result = run(self); + if let Err(e) = &result { + self.message.clone_from(e); + } + result + } + + fn apply(&mut self, host: &mut dyn Host, action: Action) -> Result<(), String> { + if self.other_document(host) { + return Err(OTHER_DOCUMENT.into()); + } + if action == Action::Check { + self.skipped.clear(); + self.ignored.clear(); + return self.refresh(host); + } + let issue = self.current().cloned().ok_or(NO_ISSUE)?; + self.validate(host, &issue)?; + match action { + Action::Change(to) => { + let to = to.unwrap_or_else(|| self.replacement.clone()); + if to.is_empty() || to == issue.word { + return Err(SAME_REPLACEMENT.into()); + } + host.change(&issue, &to)?; + self.shift_skips(&issue, to.len()); + } + Action::IgnoreOnce => { + self.skipped.push(issue); + self.issues.remove(0); + return self.load_current(host); + } + Action::IgnoreAll => { + self.issues.retain(|i| i.word != issue.word); + self.ignored.insert(issue.word); + return self.load_current(host); + } + Action::AddToDictionary => { + if issue.kind != IssueKind::Spelling { + return Err(NOT_SPELLING.into()); + } + host.add_word(&issue.word)?; + } + Action::Check => {} + } + self.refresh(host) + } + + /// The text must not have changed, and a fresh check must still find the issue: language, + /// style and dictionary edits leave the text alone. + fn validate(&self, host: &mut dyn Host, issue: &Issue) -> Result<(), String> { + if self.stale(host) || !host.check()?.iter().any(|found| found.same_finding(issue)) { + return Err(CHANGED.into()); + } + Ok(()) + } + + /// A correction moves later offsets in its story; ignored occurrences follow them. + fn shift_skips(&mut self, change: &Issue, new_len: usize) { + let removed = change.end.saturating_sub(change.start); + self.skipped.retain_mut(|skip| { + if skip.story != change.story || skip.end <= change.start { + return true; + } + if skip.start < change.end { + return false; + } + skip.start = skip.start.saturating_sub(removed).saturating_add(new_len); + skip.end = skip.end.saturating_sub(removed).saturating_add(new_len); + true + }); + // This pass made the change, so `refresh` must not take it for someone else's edit. + self.versions.remove(&change.story); + } + + /// Check again, keeping this pass's ignored occurrences while their text is unchanged. + fn refresh(&mut self, host: &mut dyn Host) -> Result<(), String> { + let found = host.check()?; + let versions = &self.versions; + self.skipped.retain(|skip| versions.get(&skip.story).is_none_or(|v| host.version(skip.story) == Some(*v))); + let (skipped, ignored) = (&self.skipped, &self.ignored); + self.issues = found.into_iter().filter(|i| !skipped.iter().any(|skip| skip.same_finding(i)) && !ignored.contains(&i.word)).collect(); + let stories: HashSet = self.issues.iter().chain(&self.skipped).map(|i| i.story).collect(); + self.versions = stories.into_iter().filter_map(|story| Some((story, host.version(story)?))).collect(); + self.load_current(host) + } + + /// Suggestions for the current issue, its surroundings, and showing it in the document. + fn load_current(&mut self, host: &mut dyn Host) -> Result<(), String> { + self.manual = false; + self.replacement.clear(); + self.before.clear(); + self.after.clear(); + let Some(issue) = self.issues.first().cloned() else { return Ok(()) }; + let suggestions = host.suggestions(&issue).map_err(|_| CHANGED.to_string())?; + self.replacement = suggestions.first().cloned().unwrap_or_default(); + if let Some(text) = host.text(issue.story) { + (self.before, self.after) = surroundings(text, issue.start, issue.end); + } + host.reveal(&issue); + if let Some(current) = self.issues.first_mut() { + current.suggestions = suggestions; + } + Ok(()) + } +} + +/// What reads around a word in a narrow panel: its paragraph, up to 60 characters each side. +fn surroundings(text: &str, start: usize, end: usize) -> (String, String) { + const CHARS: usize = 60; + let before = text.get(..start).unwrap_or("").rsplit('\n').next().unwrap_or(""); + let after = text.get(end..).unwrap_or("").split('\n').next().unwrap_or(""); + let left = before.char_indices().rev().nth(CHARS - 1).map_or(0, |(i, _)| i); + let right = after.char_indices().nth(CHARS).map_or(after.len(), |(i, _)| i); + ( + format!("{}{}", if left > 0 { "…" } else { "" }, before.get(left..).unwrap_or("")), + format!("{}{}", after.get(..right).unwrap_or(""), if right < after.len() { "…" } else { "" }), + ) +} diff --git a/crates/proof-ui/src/tests.rs b/crates/proof-ui/src/tests.rs new file mode 100644 index 000000000..769e2c29d --- /dev/null +++ b/crates/proof-ui/src/tests.rs @@ -0,0 +1,346 @@ +use std::borrow::Cow; +use std::collections::HashSet; + +use egui_kittest::{Harness, kittest::Queryable}; + +use super::*; +use crate::review::SAME_REPLACEMENT; + +const TEXT: &str = "The jumpd fox. A jumpd dog. The wrng cat."; + +/// A one-story document that knows two misspellings. +#[derive(Default)] +struct Doc { + text: String, + version: u64, + id: u64, + known: HashSet, + revealed: Vec<(usize, usize)>, + asked: Vec, + edits: u32, + notice: Option, + preferred: bool, +} + +impl Doc { + fn new(text: &str) -> Self { + Doc { text: text.into(), id: 1, ..Default::default() } + } + fn edit(&mut self, at: usize, insert: &str) { + self.text.insert_str(at, insert); + self.version += 1; + } +} + +impl Host for Doc { + fn check(&mut self) -> Result, String> { + let mut found = Vec::new(); + for word in ["jumpd", "wrng"] { + if self.known.contains(word) { + continue; + } + for (start, _) in self.text.match_indices(word) { + let kind = if self.preferred { IssueKind::Grammar } else { IssueKind::Spelling }; + found.push(Issue { story: 7, start, end: start + word.len(), word: word.into(), kind, ..Default::default() }); + } + } + found.sort_by_key(|i| i.start); + Ok(found) + } + fn suggestions(&mut self, issue: &Issue) -> Result, String> { + self.asked.push(issue.word.clone()); + Ok(match issue.word.as_str() { + "jumpd" => vec!["jumped".into(), "jumps".into()], + "wrng" => vec!["wrong".into()], + _ => vec![], + }) + } + fn text(&self, story: u64) -> Option<&str> { + (story == 7).then_some(self.text.as_str()) + } + fn version(&self, story: u64) -> Option { + (story == 7).then_some(self.version) + } + fn document(&self) -> Option { + Some(self.id) + } + fn reveal(&mut self, issue: &Issue) { + self.revealed.push((issue.start, issue.end)); + } + fn change(&mut self, issue: &Issue, to: &str) -> Result<(), String> { + self.text.replace_range(issue.start..issue.end, to); + self.version += 1; + self.edits += 1; + Ok(()) + } + fn add_word(&mut self, word: &str) -> Result<(), String> { + self.known.insert(word.into()); + Ok(()) + } + fn notice(&self) -> Option { + self.notice.clone() + } +} + +struct English; +impl Text for English { + fn ui<'a>(&'a self, source: &'a str) -> Cow<'a, str> { + source.into() + } +} + +const COLORS: Colors = Colors { + text: egui::Color32::WHITE, + dim: egui::Color32::GRAY, + field: egui::Color32::BLACK, + field_border: egui::Color32::GRAY, + hover: egui::Color32::DARK_GRAY, + spelling: egui::Color32::RED, + grammar: egui::Color32::BLUE, + error: egui::Color32::RED, +}; + +type State = (Review, Doc); + +fn harness(state: State) -> Harness<'static, State> { + let mut h = Harness::builder() + .with_size(egui::vec2(320.0, 800.0)) + .build_ui_state(|ui, (review, doc): &mut State| panel(ui, review, doc, &English, &COLORS), state); + h.run_steps(2); + h +} + +fn started(text: &str) -> State { + let mut doc = Doc::new(text); + let mut review = Review::default(); + review.start(&mut doc); + (review, doc) +} + +#[test] +fn a_pass_starts_on_the_first_issue_shows_it_and_asks_only_for_its_suggestions() { + let (review, doc) = started(TEXT); + assert_eq!(review.remaining(), 3); + assert_eq!(review.current().unwrap().suggestions, ["jumped", "jumps"]); + assert_eq!(doc.revealed, [(4, 9)], "the word is shown in the document"); + assert_eq!(doc.asked, ["jumpd"], "never the rest of a book's suggestions"); + assert_eq!(review.replacement, "jumped"); + assert_eq!((review.before.as_str(), review.after.as_str()), ("The ", " fox. A jumpd dog. The wrng cat.")); +} + +#[test] +fn the_panel_starts_a_review_from_its_button() { + let mut h = harness((Review::default(), Doc::new(TEXT))); + h.get_by_label("Check Spelling").click(); + h.run_steps(2); + assert_eq!(h.state().0.current().unwrap().word, "jumpd"); +} + +#[test] +fn clicking_a_suggestion_corrects_the_word_and_moves_on() { + let mut h = harness(started(TEXT)); + h.get_by_label("jumps").click(); + h.run_steps(2); + assert_eq!(h.state().1.text, "The jumps fox. A jumpd dog. The wrng cat."); + assert_eq!(h.state().1.edits, 1); + assert_eq!(h.state().0.current().unwrap().start, 17); +} + +#[test] +fn ignore_once_skips_one_occurrence_and_ignore_all_the_word() { + let mut h = harness(started(TEXT)); + h.get_by_label("Ignore Once").click(); + h.run_steps(2); + assert_eq!((h.state().0.remaining(), h.state().0.current().unwrap().start), (2, 17)); + assert_eq!(h.state().0.current().unwrap().suggestions, ["jumped", "jumps"], "the next one gets its suggestions"); + h.get_by_label("Ignore All").click(); + h.run_steps(2); + assert_eq!(h.state().0.current().unwrap().word, "wrng"); + h.get_by_label("Ignore Once").click(); + h.run_steps(2); + assert!(h.state().0.current().is_none()); + assert_eq!(h.state().1.edits, 0, "ignoring edits nothing"); + assert!(h.query_by_label("The spelling check is complete. Some issues were ignored.").is_some()); + h.get_by_label("Check Again").click(); + h.run_steps(2); + assert_eq!(h.state().0.remaining(), 3, "a new pass checks everything again"); +} + +#[test] +fn add_to_dictionary_clears_every_occurrence_and_a_clean_document_says_so() { + let mut h = harness(started("The jumpd fox. A jumpd dog.")); + h.get_by_label("Add to Dictionary").click(); + h.run_steps(2); + assert!(h.state().1.known.contains("jumpd")); + assert!(h.state().0.current().is_none()); + assert!(h.query_by_label("No issues found.").is_some()); +} + +#[test] +fn a_preferred_form_cant_join_the_dictionary() { + let mut doc = Doc::new(TEXT); + doc.preferred = true; + let mut review = Review::default(); + review.start(&mut doc); + let mut h = harness((review, doc)); + assert!(h.query_by_label("Preferred Form").is_some()); + assert!(h.query_by_label("Add to Dictionary").is_none()); + assert!(h.query_by_label("Ignore All").is_none()); + let (review, doc) = h.state_mut(); + assert!(review.act(doc, Action::AddToDictionary).unwrap_err().contains("Only spelling issues")); +} + +#[test] +fn a_typed_replacement_is_applied_from_the_change_field() { + let mut h = harness(started(TEXT)); + assert!(h.query_by_label("Change to:").is_none(), "out of the way until asked for"); + h.get_by_label("Change…").click(); + h.run_steps(2); + h.state_mut().0.replacement = "typed".into(); + h.run_steps(1); + h.get_by_label("Change").click(); + h.run_steps(2); + assert_eq!(h.state().1.text, "The typed fox. A jumpd dog. The wrng cat."); +} + +#[test] +fn the_same_or_an_empty_replacement_is_refused() { + let (mut review, mut doc) = started(TEXT); + for to in ["", "jumpd"] { + assert_eq!(review.act(&mut doc, Action::Change(Some(to.into()))).unwrap_err(), SAME_REPLACEMENT); + } + assert_eq!(doc.edits, 0); +} + +#[test] +fn edited_text_stops_corrections_until_checked_again() { + let (mut review, mut doc) = started(TEXT); + doc.edit(0, "A "); + let before = doc.text.clone(); + assert!(review.stale(&doc)); + assert!(review.act(&mut doc, Action::Change(Some("jumped".into()))).unwrap_err().contains("text changed")); + assert_eq!(doc.text, before, "stale offsets are never applied"); + let mut h = harness((review, doc)); + assert!(h.query_by_label("jumped").is_none(), "no suggestion is offered for changed text"); + h.get_by_label("Check Again").click(); + h.run_steps(2); + assert_eq!(h.state().0.current().unwrap().start, 6, "checked again at its new place"); + assert!(!h.state().0.stale(&h.state().1)); +} + +/// A language or dictionary edit leaves the text alone, and the issue is gone all the same. +#[test] +fn a_correction_needs_the_issue_to_still_be_found() { + let (mut review, mut doc) = started(TEXT); + doc.known.insert("jumpd".into()); + assert!(review.act(&mut doc, Action::Change(Some("jumped".into()))).unwrap_err().contains("text changed")); + assert_eq!(doc.edits, 0); +} + +#[test] +fn a_correction_keeps_ignored_occurrences_at_their_new_offsets() { + // Before the correction: the ignored offsets stand. + let (mut review, mut doc) = started("jumpd one wrng two jumpd"); + review.act(&mut doc, Action::IgnoreOnce).unwrap(); + review.act(&mut doc, Action::Change(Some("wrong".into()))).unwrap(); + assert_eq!(doc.text, "jumpd one wrong two jumpd"); + assert_eq!(review.current().unwrap().start, 20, "the first stays ignored; the last is next"); + // After the correction: the ignored offset moves with its word. "wrng" is ignored here by + // the second pass reaching it first, so correct the word before an ignored one. + let (mut review, mut doc) = started("wrng one jumpd"); + review.act(&mut doc, Action::Change(Some("wrong".into()))).unwrap(); + assert_eq!(review.current().unwrap().start, 10); + review.act(&mut doc, Action::IgnoreOnce).unwrap(); + assert!(review.current().is_none()); + assert!(review.ignored_any()); +} + +#[test] +fn someone_elses_edit_forgets_what_was_ignored_in_that_story() { + let (mut review, mut doc) = started(TEXT); + review.act(&mut doc, Action::IgnoreOnce).unwrap(); + doc.edit(0, "A "); + review.act(&mut doc, Action::Check).unwrap(); + assert_eq!(review.remaining(), 3); + assert!(!review.ignored_any()); +} + +#[test] +fn another_document_ends_the_review() { + let (mut review, mut doc) = started(TEXT); + doc.id = 2; + assert!(review.act(&mut doc, Action::IgnoreOnce).unwrap_err().contains("document changed")); + let mut h = harness((review, doc)); + assert!(h.query_by_label("jumped").is_none()); + h.get_by_label("Check This Document").click(); + h.run_steps(2); + assert!(!h.state().0.other_document(&h.state().1)); + assert_eq!(h.state().0.current().unwrap().word, "jumpd"); +} + +#[test] +fn a_notice_and_a_failure_are_shown() { + let mut doc = Doc::new(TEXT); + doc.notice = Some("Khmer text has not been checked.".into()); + let mut review = Review::default(); + review.start(&mut doc); + doc.edit(0, "A "); + let _ = review.act(&mut doc, Action::Change(Some("jumped".into()))); + let h = harness((review, doc)); + assert!(h.query_by_label("Khmer text has not been checked.").is_some()); + assert_eq!(h.query_all_by_label("The text changed. Check again to continue.").count(), 2, "the state and the failure"); +} + +#[test] +fn every_string_is_listed_once() { + for source in + ["Spelling", "Preferred Form", "Suggestions", "Ignore Once", "Ignore All", "Add to Dictionary", "Change…", "Change to:", "Check Again"] + { + assert!(STRINGS.contains(&source), "{source}"); + } + let mut unique = STRINGS.to_vec(); + unique.sort_unstable(); + unique.dedup(); + assert_eq!(unique.len(), STRINGS.len()); +} + +fn context_harness(issue: Issue) -> Harness<'static, (Issue, Option)> { + let mut h = Harness::builder().with_size(egui::vec2(320.0, 400.0)).build_ui_state( + |ui, (issue, choice): &mut (Issue, Option)| { + if let Some(chosen) = context_section(ui, issue, &English) { + *choice = Some(chosen); + } + }, + (issue, None), + ); + h.run_steps(1); + h +} + +#[test] +fn the_right_click_section_offers_each_choice() { + let issue = Issue { word: "jumpd".into(), suggestions: vec!["jumped".into()], ..Default::default() }; + for (button, expected) in [ + ("jumped", ContextChoice::Change("jumped".into())), + ("Add to Dictionary", ContextChoice::AddToDictionary), + ("Check Spelling…", ContextChoice::Review), + ] { + let mut h = context_harness(issue.clone()); + h.get_by_label(button).click(); + h.run_steps(2); + assert_eq!(h.state().1.as_ref(), Some(&expected)); + } + let preferred = Issue { kind: IssueKind::Grammar, ..issue }; + assert!(context_harness(preferred).query_by_label("Add to Dictionary").is_none()); + let none = Issue { word: "x".into(), ..Default::default() }; + assert!(context_harness(none).query_by_label("(no suggestions)").is_some()); +} + +#[test] +fn context_is_bounded_and_stays_in_the_paragraph() { + let long = format!("{}\n{} jumpd {}\nnext", "x".repeat(10), "b".repeat(200), "a".repeat(200)); + let (review, _) = started(&long); + assert!(review.before.starts_with('…') && !review.before.contains('x')); + assert!(review.after.ends_with('…') && !review.after.contains("next")); + assert!(review.before.chars().count() <= 61 && review.after.chars().count() <= 61); +} diff --git a/crates/proof-ui/src/ui.rs b/crates/proof-ui/src/ui.rs new file mode 100644 index 000000000..3fd9cb673 --- /dev/null +++ b/crates/proof-ui/src/ui.rs @@ -0,0 +1,235 @@ +//! The panel and the right-click section. Looks follow the host's theme through [`Colors`]. +use std::borrow::Cow; + +use egui::{Align2, Color32, FontId, RichText, Sense, Stroke, Ui, vec2}; + +use crate::review::{CHANGED, NO_ISSUE, NOT_SPELLING, OTHER_DOCUMENT, SAME_REPLACEMENT}; +use crate::{Action, Host, Issue, IssueKind, Review}; + +/// The application's translations. +pub trait Text { + /// A string from [`STRINGS`], by its English source text. + fn ui<'a>(&'a self, source: &'a str) -> Cow<'a, str>; + /// A label put into display order (the app's bidi reordering). + fn visual(&self, s: &str) -> String { + s.to_string() + } +} + +/// Every string looked up with [`Text::ui`], for the shared catalog. +pub const STRINGS: &[&str] = &[ + "Spelling", + "Preferred Form", + "Suggestions", + "(no suggestions)", + "Ignore Once", + "Ignore All", + "Add to Dictionary", + "Change…", + "Change to:", + "Change", + "Check Spelling", + "Check Spelling…", + "Check Again", + "Check This Document", + "Check the spelling of the whole document.", + "No issues found.", + "The spelling check is complete. Some issues were ignored.", + NO_ISSUE, + NOT_SPELLING, + CHANGED, + OTHER_DOCUMENT, + SAME_REPLACEMENT, +]; + +/// The host's theme. +#[derive(Clone, Copy, Debug)] +pub struct Colors { + pub text: Color32, + pub dim: Color32, + pub field: Color32, + pub field_border: Color32, + pub hover: Color32, + /// The underline of a misspelled word. + pub spelling: Color32, + /// The underline of a preferred form. + pub grammar: Color32, + pub error: Color32, +} + +fn label(ui: &mut Ui, text: &dyn Text, source: &str, size: f32, color: Color32) -> egui::Response { + ui.label(RichText::new(text.visual(&text.ui(source))).size(size).color(color)) +} + +fn link(ui: &mut Ui, text: &dyn Text, source: &str, colors: &Colors) -> bool { + ui.add(egui::Button::new(RichText::new(text.visual(&text.ui(source))).color(colors.text).underline()).frame(false)).clicked() +} + +fn context(ui: &mut Ui, review: &Review, issue: &Issue, colors: &Colors) { + let format = egui::TextFormat { font_id: FontId::proportional(13.0), color: colors.text, ..Default::default() }; + let mark = if issue.kind == IssueKind::Spelling { colors.spelling } else { colors.grammar }; + let mut job = egui::text::LayoutJob::default(); + job.append(&review.before, 0.0, format.clone()); + job.append(&issue.word, 0.0, egui::TextFormat { underline: Stroke::new(1.5, mark), ..format.clone() }); + job.append(&review.after, 0.0, format); + let width = ui.available_width(); + egui::Frame::NONE.fill(colors.field).stroke(Stroke::new(1.0, colors.field_border)).corner_radius(2.0).inner_margin(8).show(ui, |ui| { + ui.set_min_width((width - 18.0).max(0.0)); + ui.add(egui::Label::new(job).wrap()); + }); +} + +/// A full-width row that applies its suggestion when clicked. +fn suggestion_row(ui: &mut Ui, suggestion: &str, colors: &Colors) -> bool { + let (rect, response) = ui.allocate_exact_size(vec2(ui.available_width(), 30.0), Sense::click()); + response.widget_info(|| egui::WidgetInfo::labeled(egui::Role::Button, true, suggestion)); + if response.hovered() { + ui.painter().rect_filled(rect, 0.0, colors.hover); + } + ui.painter().with_clip_rect(rect.shrink2(vec2(8.0, 0.0))).text( + rect.left_center() + vec2(8.0, 0.0), + Align2::LEFT_CENTER, + suggestion, + FontId::proportional(13.0), + colors.text, + ); + response.clicked() +} + +/// What to do after the panel has been drawn, so that nothing changes under its borrows. +enum After { + Start, + Act(Action), +} + +/// Draws the review. Starts one from its own button if there is none yet. +pub fn panel(ui: &mut Ui, review: &mut Review, host: &mut dyn Host, text: &dyn Text, colors: &Colors) { + let after = egui::ScrollArea::vertical().auto_shrink([false, true]).show(ui, |ui| body(ui, review, &*host, text, colors)).inner; + match after { + Some(After::Start) => review.start(host), + Some(After::Act(action)) => { + // A failure stays in the review for the next frame. + let _ = review.act(host, action); + } + None => {} + } +} + +fn body(ui: &mut Ui, review: &mut Review, host: &dyn Host, text: &dyn Text, colors: &Colors) -> Option { + if !review.started() { + label(ui, text, "Check the spelling of the whole document.", 12.0, colors.text); + ui.add_space(8.0); + return ui.button(text.visual(&text.ui("Check Spelling"))).clicked().then_some(After::Start); + } + if review.other_document(host) { + label(ui, text, OTHER_DOCUMENT, 12.0, colors.text); + ui.add_space(8.0); + return ui.button(text.visual(&text.ui("Check This Document"))).clicked().then_some(After::Start); + } + if let Some(notice) = host.notice() { + ui.label(RichText::new(notice).size(11.0).color(colors.dim)); + ui.add_space(8.0); + } + let mut after = None; + match review.current().cloned() { + Some(issue) => after = current_issue(ui, review, &issue, host, text, colors), + None => { + let done = if review.ignored_any() { "The spelling check is complete. Some issues were ignored." } else { "No issues found." }; + label(ui, text, done, 12.0, colors.text); + ui.add_space(8.0); + if link(ui, text, "Check Again", colors) { + after = Some(After::Act(Action::Check)); + } + } + } + if !review.message().is_empty() { + ui.add_space(8.0); + label(ui, text, review.message(), 12.0, colors.error); + } + after +} + +fn current_issue(ui: &mut Ui, review: &mut Review, issue: &Issue, host: &dyn Host, text: &dyn Text, colors: &Colors) -> Option { + let spelling = issue.kind == IssueKind::Spelling; + let heading = text.visual(&text.ui(if spelling { "Spelling" } else { "Preferred Form" })); + ui.label(RichText::new(heading).size(14.0).strong().color(colors.text)); + if !issue.message.is_empty() { + ui.label(RichText::new(&issue.message).size(11.0).color(colors.dim)); + } + ui.add_space(8.0); + context(ui, review, issue, colors); + ui.add_space(12.0); + if review.stale(host) { + label(ui, text, CHANGED, 12.0, colors.text); + ui.add_space(8.0); + return link(ui, text, "Check Again", colors).then_some(After::Act(Action::Check)); + } + let mut after = None; + label(ui, text, "Suggestions", 11.0, colors.dim); + if issue.suggestions.is_empty() { + ui.label(RichText::new(text.visual(&text.ui("(no suggestions)"))).italics().color(colors.text)); + } else { + egui::Frame::NONE.fill(colors.field).stroke(Stroke::new(1.0, colors.field_border)).corner_radius(2.0).show(ui, |ui| { + ui.spacing_mut().item_spacing.y = 0.0; + for suggestion in &issue.suggestions { + if suggestion_row(ui, suggestion, colors) { + after = Some(After::Act(Action::Change(Some(suggestion.clone())))); + } + } + }); + } + ui.add_space(8.0); + if link(ui, text, "Ignore Once", colors) { + after = Some(After::Act(Action::IgnoreOnce)); + } + if spelling && link(ui, text, "Ignore All", colors) { + after = Some(After::Act(Action::IgnoreAll)); + } + if spelling && link(ui, text, "Add to Dictionary", colors) { + after = Some(After::Act(Action::AddToDictionary)); + } + if !review.manual && link(ui, text, "Change…", colors) { + review.manual = true; + } + if review.manual { + ui.add_space(8.0); + let name = label(ui, text, "Change to:", 12.0, colors.text); + let field = ui.add(egui::TextEdit::singleline(&mut review.replacement).desired_width(f32::INFINITY)).labelled_by(name.id); + let enter = field.lost_focus() && ui.input(|i| i.key_pressed(egui::Key::Enter)); + let button = egui::Button::new(text.visual(&text.ui("Change"))); + if (ui.add_enabled(!review.replacement.is_empty(), button).clicked() || enter) && !review.replacement.is_empty() { + after = Some(After::Act(Action::Change(None))); + } + } + after +} + +/// What was chosen in the right-click section. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum ContextChoice { + Change(String), + AddToDictionary, + /// Open the panel. + Review, +} + +/// The first section of the right-click menu on `issue`, whose suggestions have been asked for. +pub fn context_section(ui: &mut Ui, issue: &Issue, text: &dyn Text) -> Option { + let mut choice = None; + if issue.suggestions.is_empty() { + ui.add_enabled(false, egui::Button::new(text.visual(&text.ui("(no suggestions)")))); + } + for suggestion in &issue.suggestions { + if ui.button(RichText::new(suggestion).strong()).clicked() { + choice = Some(ContextChoice::Change(suggestion.clone())); + } + } + if issue.kind == IssueKind::Spelling && ui.button(text.visual(&text.ui("Add to Dictionary"))).clicked() { + choice = Some(ContextChoice::AddToDictionary); + } + if ui.button(text.visual(&text.ui("Check Spelling…"))).clicked() { + choice = Some(ContextChoice::Review); + } + ui.separator(); + choice +} From 5c7a410e6ce1dc9672ad75b75d5dd424986fa956 Mon Sep 17 00:00:00 2001 From: Nathan Wells Date: Tue, 3 Nov 2026 15:57:03 -0800 Subject: [PATCH 2/6] craft-proof-ui: keep the review's actions in view in a short panel Co-Authored-By: Claude Opus 5.5 --- crates/proof-ui/src/ui.rs | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/crates/proof-ui/src/ui.rs b/crates/proof-ui/src/ui.rs index 3fd9cb673..5c0ad067d 100644 --- a/crates/proof-ui/src/ui.rs +++ b/crates/proof-ui/src/ui.rs @@ -81,7 +81,7 @@ fn context(ui: &mut Ui, review: &Review, issue: &Issue, colors: &Colors) { /// A full-width row that applies its suggestion when clicked. fn suggestion_row(ui: &mut Ui, suggestion: &str, colors: &Colors) -> bool { - let (rect, response) = ui.allocate_exact_size(vec2(ui.available_width(), 30.0), Sense::click()); + let (rect, response) = ui.allocate_exact_size(vec2(ui.available_width(), 25.0), Sense::click()); response.widget_info(|| egui::WidgetInfo::labeled(egui::Role::Button, true, suggestion)); if response.hovered() { ui.painter().rect_filled(rect, 0.0, colors.hover); @@ -179,18 +179,21 @@ fn current_issue(ui: &mut Ui, review: &mut Review, issue: &Issue, host: &dyn Hos }); } ui.add_space(8.0); - if link(ui, text, "Ignore Once", colors) { - after = Some(After::Act(Action::IgnoreOnce)); - } - if spelling && link(ui, text, "Ignore All", colors) { - after = Some(After::Act(Action::IgnoreAll)); - } - if spelling && link(ui, text, "Add to Dictionary", colors) { - after = Some(After::Act(Action::AddToDictionary)); - } - if !review.manual && link(ui, text, "Change…", colors) { - review.manual = true; - } + // On wrapping lines: a docked panel is often too short for a column of them to stay in view. + ui.horizontal_wrapped(|ui| { + if link(ui, text, "Ignore Once", colors) { + after = Some(After::Act(Action::IgnoreOnce)); + } + if spelling && link(ui, text, "Ignore All", colors) { + after = Some(After::Act(Action::IgnoreAll)); + } + if spelling && link(ui, text, "Add to Dictionary", colors) { + after = Some(After::Act(Action::AddToDictionary)); + } + if !review.manual && link(ui, text, "Change…", colors) { + review.manual = true; + } + }); if review.manual { ui.add_space(8.0); let name = label(ui, text, "Change to:", 12.0, colors.text); From f3d689bdad0816e7d5e00891a789acd31b5e34cb Mon Sep 17 00:00:00 2001 From: Nathan Wells Date: Tue, 3 Nov 2026 20:23:40 -0800 Subject: [PATCH 3/6] craft-proof-ui: real buttons, a Change to field and marks for the page The panel's actions were underlined text, and a click on a suggestion changed the document at once. Change is now a filled button with Ignore Once, Ignore All and Add to Dictionary as outlined buttons under it, in a footer that stays in view when a short panel scrolls and keeps its place from one issue to the next. Change to is always there, holding the best suggestion; a click chooses a suggestion and a double-click applies it. The heading says how many issues are left. Review::marks and Review::covers tell an app what to underline in its document. From review: Action::Check starts a review that hadn't been; ignoring takes in what its fresh check found; a failed check no longer says the document is clean; an issue a fresh check lost offers Check Again; Host::change and Text::ui say what they promise. Co-Authored-By: Claude Opus 5.5 --- crates/proof-ui/README.md | 4 +- crates/proof-ui/src/lib.rs | 2 +- crates/proof-ui/src/review.rs | 44 +++++-- crates/proof-ui/src/tests.rs | 193 +++++++++++++++++++++++++++-- crates/proof-ui/src/ui.rs | 222 +++++++++++++++++++++++++++------- 5 files changed, 397 insertions(+), 68 deletions(-) diff --git a/crates/proof-ui/README.md b/crates/proof-ui/README.md index c4e6ca217..1312796f6 100644 --- a/crates/proof-ui/README.md +++ b/crates/proof-ui/README.md @@ -23,8 +23,10 @@ use craft_proof_ui::{Colors, Host, Review, context_section, panel}; // Keep a `Review` while the panel is open. `start` checks the document from the top. let mut review = Review::default(); review.start(&mut host); -// Each frame, inside the app's dock: +// Each frame, inside the app's dock. It fills the height it's given and scrolls its own content: panel(ui, &mut review, &mut host, &text, &colors); +// While the panel shows, the app can underline what's left in the document: +for issue in review.marks(document, |story| version(story)) { /* draw it */ } // On a right-click the app finds the issue under the caret, once. Then, in its menu: if let Some(chosen) = context_section(ui, &issue, &text) { /* act on it, close the menu */ } ``` diff --git a/crates/proof-ui/src/lib.rs b/crates/proof-ui/src/lib.rs index a9290f452..bbf9cc0dc 100644 --- a/crates/proof-ui/src/lib.rs +++ b/crates/proof-ui/src/lib.rs @@ -50,7 +50,7 @@ pub trait Host { fn document(&self) -> Option; /// Show the issue in the document, usually by selecting it. fn reveal(&mut self, issue: &Issue); - /// Replace the issue's text, as one undoable edit. + /// Replace the issue's text with exactly `to`, as one undoable edit. fn change(&mut self, issue: &Issue, to: &str) -> Result<(), String>; /// Accept the word from now on. fn add_word(&mut self, word: &str) -> Result<(), String>; diff --git a/crates/proof-ui/src/review.rs b/crates/proof-ui/src/review.rs index 34a56ccd9..70f1b7e36 100644 --- a/crates/proof-ui/src/review.rs +++ b/crates/proof-ui/src/review.rs @@ -35,9 +35,8 @@ pub struct Review { versions: HashMap, pub(crate) before: String, pub(crate) after: String, - /// The Change… field's text. + /// The Change to field's text. pub replacement: String, - pub(crate) manual: bool, message: String, started: bool, } @@ -62,6 +61,19 @@ impl Review { self.issues.len() } + /// The pass still speaks for a story: `document` and `version` are the answers of + /// [`Host::document`] and [`Host::version`] now, and neither has changed since it was checked. + /// Even a story with nothing left to mark may hold ignored words. + pub fn covers(&self, document: Option, story: u64, version: Option) -> bool { + self.started && document == self.document && version.is_some_and(|v| self.versions.get(&story) == Some(&v)) + } + + /// The issues left that can be marked in the document, the current one first: only in stories + /// the pass [covers](Self::covers), as other offsets can't be trusted. + pub fn marks(&self, document: Option, version: impl Fn(u64) -> Option) -> impl Iterator { + self.issues.iter().filter(move |issue| self.covers(document, issue.story, version(issue.story))) + } + pub fn ignored_any(&self) -> bool { !self.skipped.is_empty() || !self.ignored.is_empty() } @@ -100,12 +112,14 @@ impl Review { return Err(OTHER_DOCUMENT.into()); } if action == Action::Check { + self.started = true; + self.document = host.document(); self.skipped.clear(); self.ignored.clear(); return self.refresh(host); } let issue = self.current().cloned().ok_or(NO_ISSUE)?; - self.validate(host, &issue)?; + let found = self.validate(host, &issue)?; match action { Action::Change(to) => { let to = to.unwrap_or_else(|| self.replacement.clone()); @@ -117,13 +131,11 @@ impl Review { } Action::IgnoreOnce => { self.skipped.push(issue); - self.issues.remove(0); - return self.load_current(host); + return self.update(host, found); } Action::IgnoreAll => { - self.issues.retain(|i| i.word != issue.word); self.ignored.insert(issue.word); - return self.load_current(host); + return self.update(host, found); } Action::AddToDictionary => { if issue.kind != IssueKind::Spelling { @@ -138,11 +150,15 @@ impl Review { /// The text must not have changed, and a fresh check must still find the issue: language, /// style and dictionary edits leave the text alone. - fn validate(&self, host: &mut dyn Host, issue: &Issue) -> Result<(), String> { - if self.stale(host) || !host.check()?.iter().any(|found| found.same_finding(issue)) { + fn validate(&self, host: &mut dyn Host, issue: &Issue) -> Result, String> { + if self.stale(host) { return Err(CHANGED.into()); } - Ok(()) + let found = host.check()?; + if !found.iter().any(|found| found.same_finding(issue)) { + return Err(CHANGED.into()); + } + Ok(found) } /// A correction moves later offsets in its story; ignored occurrences follow them. @@ -166,18 +182,22 @@ impl Review { /// Check again, keeping this pass's ignored occurrences while their text is unchanged. fn refresh(&mut self, host: &mut dyn Host) -> Result<(), String> { let found = host.check()?; + self.update(host, found) + } + + /// Take `found`, a check of the document as it is now, as the issues left. + fn update(&mut self, host: &mut dyn Host, found: Vec) -> Result<(), String> { let versions = &self.versions; self.skipped.retain(|skip| versions.get(&skip.story).is_none_or(|v| host.version(skip.story) == Some(*v))); + let stories: HashSet = found.iter().chain(&self.skipped).map(|i| i.story).collect(); let (skipped, ignored) = (&self.skipped, &self.ignored); self.issues = found.into_iter().filter(|i| !skipped.iter().any(|skip| skip.same_finding(i)) && !ignored.contains(&i.word)).collect(); - let stories: HashSet = self.issues.iter().chain(&self.skipped).map(|i| i.story).collect(); self.versions = stories.into_iter().filter_map(|story| Some((story, host.version(story)?))).collect(); self.load_current(host) } /// Suggestions for the current issue, its surroundings, and showing it in the document. fn load_current(&mut self, host: &mut dyn Host) -> Result<(), String> { - self.manual = false; self.replacement.clear(); self.before.clear(); self.after.clear(); diff --git a/crates/proof-ui/src/tests.rs b/crates/proof-ui/src/tests.rs index 769e2c29d..877eacfd0 100644 --- a/crates/proof-ui/src/tests.rs +++ b/crates/proof-ui/src/tests.rs @@ -1,7 +1,10 @@ use std::borrow::Cow; use std::collections::HashSet; -use egui_kittest::{Harness, kittest::Queryable}; +use egui_kittest::{ + Harness, + kittest::{NodeT, Queryable}, +}; use super::*; use crate::review::SAME_REPLACEMENT; @@ -20,6 +23,8 @@ struct Doc { edits: u32, notice: Option, preferred: bool, + many: Option>, + fail_check: bool, } impl Doc { @@ -34,6 +39,9 @@ impl Doc { impl Host for Doc { fn check(&mut self) -> Result, String> { + if self.fail_check { + return Err("the dictionary could not be read".into()); + } let mut found = Vec::new(); for word in ["jumpd", "wrng"] { if self.known.contains(word) { @@ -49,6 +57,9 @@ impl Host for Doc { } fn suggestions(&mut self, issue: &Issue) -> Result, String> { self.asked.push(issue.word.clone()); + if let Some(many) = self.many.clone().filter(|_| issue.word == "jumpd") { + return Ok(many); + } Ok(match issue.word.as_str() { "jumpd" => vec!["jumped".into(), "jumps".into()], "wrng" => vec!["wrong".into()], @@ -95,6 +106,9 @@ const COLORS: Colors = Colors { field: egui::Color32::BLACK, field_border: egui::Color32::GRAY, hover: egui::Color32::DARK_GRAY, + selected: egui::Color32::DARK_BLUE, + accent: egui::Color32::BLUE, + on_accent: egui::Color32::WHITE, spelling: egui::Color32::RED, grammar: egui::Color32::BLUE, error: egui::Color32::RED, @@ -137,15 +151,106 @@ fn the_panel_starts_a_review_from_its_button() { } #[test] -fn clicking_a_suggestion_corrects_the_word_and_moves_on() { +fn the_first_suggestion_is_ready_to_apply_and_a_click_chooses_another() { let mut h = harness(started(TEXT)); + assert_eq!(h.state().0.replacement, "jumped"); h.get_by_label("jumps").click(); h.run_steps(2); + assert_eq!(h.state().0.replacement, "jumps"); + assert_eq!(h.state().1.edits, 0, "a click only chooses"); + h.get_by_label("Change").click(); + h.run_steps(2); assert_eq!(h.state().1.text, "The jumps fox. A jumpd dog. The wrng cat."); assert_eq!(h.state().1.edits, 1); assert_eq!(h.state().0.current().unwrap().start, 17); } +#[test] +fn a_double_click_applies_a_suggestion() { + // Frames 1 ms apart, so two clicks make a double-click. + let mut h = Harness::builder() + .with_size(egui::vec2(320.0, 800.0)) + .with_step_dt(0.001) + .build_ui_state(|ui, (review, doc): &mut State| panel(ui, review, doc, &English, &COLORS), started(TEXT)); + h.run_steps(2); + let at = h.get_by_label("jumps").rect().center(); + h.hover_at(at); + h.step(); + for pressed in [true, false, true, false] { + h.event(egui::Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed, modifiers: egui::Modifiers::NONE }); + h.step(); + } + h.run_steps(2); + assert_eq!(h.state().1.text, "The jumps fox. A jumpd dog. The wrng cat."); + assert_eq!(h.state().1.edits, 1); +} + +#[test] +fn nothing_to_change_to_disables_change() { + let mut h = harness(started("An unknownword jumpd.")); + h.state_mut().0.replacement.clear(); + h.run_steps(1); + assert!(h.get_by_label("Change").accesskit_node().is_disabled()); + h.state_mut().0.replacement = "jumpd".into(); + h.run_steps(1); + assert!(h.get_by_label("Change").accesskit_node().is_disabled(), "the same word changes nothing"); +} + +/// A docked panel can be short, and a long list must not push the actions out of reach. +#[test] +fn the_actions_stay_in_view_in_a_short_panel() { + let (mut review, mut doc) = started(TEXT); + let many = (0..30).map(|i| format!("jump{i}")).collect(); + doc.many = Some(many); + review.start(&mut doc); + let height = 240.0; + let mut h = Harness::builder() + .with_size(egui::vec2(320.0, height)) + .build_ui_state(|ui, (review, doc): &mut State| panel(ui, review, doc, &English, &COLORS), (review, doc)); + h.run_steps(2); + for action in ["Change", "Ignore Once", "Ignore All", "Add to Dictionary"] { + let bottom = h.get_by_label(action).rect().max.y; + assert!(bottom <= height, "{action} ends at {bottom}"); + } + h.get_by_label("Ignore All").click(); + h.run_steps(2); + assert_eq!(h.state().0.current().unwrap().word, "wrng"); +} + +#[test] +fn the_actions_keep_their_place_from_one_issue_to_the_next() { + let mut h = harness(started(TEXT)); + let place = h.get_by_label("Ignore Once").rect(); + h.get_by_label("Ignore All").click(); + h.run_steps(2); + assert_eq!(h.state().0.current().unwrap().suggestions.len(), 1, "a shorter list than the last"); + assert_eq!(h.get_by_label("Ignore Once").rect(), place); +} + +/// Going through issues, a click on Ignore Once must not land on Add to Dictionary. +#[test] +fn a_preferred_form_keeps_ignore_once_in_its_place() { + let place = harness(started(TEXT)).get_by_label("Ignore Once").rect().min; + let mut doc = Doc::new(TEXT); + doc.preferred = true; + let mut review = Review::default(); + review.start(&mut doc); + assert_eq!(harness((review, doc)).get_by_label("Ignore Once").rect().min, place); +} + +#[test] +fn a_narrow_panel_stacks_the_ignore_buttons() { + let mut h = Harness::builder() + .with_size(egui::vec2(110.0, 600.0)) + .build_ui_state(|ui, (review, doc): &mut State| panel(ui, review, doc, &English, &COLORS), started(TEXT)); + h.run_steps(2); + let (once, all) = (h.get_by_label("Ignore Once").rect(), h.get_by_label("Ignore All").rect()); + assert!(once.max.y <= all.min.y, "{once:?} above {all:?}"); + let wide = harness(started(TEXT)); + let (once, all) = (wide.get_by_label("Ignore Once").rect(), wide.get_by_label("Ignore All").rect()); + assert_eq!(once.min.y, all.min.y); +} + #[test] fn ignore_once_skips_one_occurrence_and_ignore_all_the_word() { let mut h = harness(started(TEXT)); @@ -193,9 +298,7 @@ fn a_preferred_form_cant_join_the_dictionary() { #[test] fn a_typed_replacement_is_applied_from_the_change_field() { let mut h = harness(started(TEXT)); - assert!(h.query_by_label("Change to:").is_none(), "out of the way until asked for"); - h.get_by_label("Change…").click(); - h.run_steps(2); + assert!(h.query_by_label("Change to:").is_some()); h.state_mut().0.replacement = "typed".into(); h.run_steps(1); h.get_by_label("Change").click(); @@ -288,13 +391,89 @@ fn a_notice_and_a_failure_are_shown() { let _ = review.act(&mut doc, Action::Change(Some("jumped".into()))); let h = harness((review, doc)); assert!(h.query_by_label("Khmer text has not been checked.").is_some()); - assert_eq!(h.query_all_by_label("The text changed. Check again to continue.").count(), 2, "the state and the failure"); + assert_eq!(h.query_all_by_label("The text changed. Check again to continue.").count(), 1, "said once"); +} + +#[test] +fn a_check_that_fails_doesnt_say_the_document_is_clean() { + let mut doc = Doc::new(TEXT); + doc.fail_check = true; + let mut review = Review::default(); + review.start(&mut doc); + let mut h = harness((review, doc)); + assert!(h.query_by_label("No issues found.").is_none()); + assert!(h.query_by_label("the dictionary could not be read").is_some()); + h.state_mut().1.fail_check = false; + h.get_by_label("Check Again").click(); + h.run_steps(2); + assert_eq!(h.state().0.current().unwrap().word, "jumpd"); +} + +/// A dictionary or language change leaves the text and its version alone: only the failure says so. +#[test] +fn an_issue_a_fresh_check_lost_offers_check_again() { + let (mut review, mut doc) = started(TEXT); + doc.known.insert("jumpd".into()); + let _ = review.act(&mut doc, Action::IgnoreOnce); + let mut h = harness((review, doc)); + assert_eq!(h.query_all_by_label("The text changed. Check again to continue.").count(), 1); + assert!(h.query_by_label("Ignore Once").is_none()); + h.get_by_label("Check Again").click(); + h.run_steps(2); + assert_eq!(h.state().0.current().unwrap().word, "wrng"); +} + +/// A dictionary or language change can add findings without touching the text. +#[test] +fn ignoring_takes_in_what_a_fresh_check_found() { + let mut doc = Doc::new(TEXT); + doc.known.insert("wrng".into()); + let mut review = Review::default(); + review.start(&mut doc); + assert_eq!(review.remaining(), 2); + doc.known.clear(); + review.act(&mut doc, Action::IgnoreAll).unwrap(); + assert_eq!(review.current().unwrap().word, "wrng"); +} + +#[test] +fn checking_starts_a_review_that_wasnt_started() { + let mut doc = Doc::new(TEXT); + let mut review = Review::default(); + review.act(&mut doc, Action::Check).unwrap(); + assert!(review.started()); + assert_eq!(review.current().unwrap().word, "jumpd"); + assert!(!review.other_document(&doc)); +} + +#[test] +fn marks_are_the_issues_left_in_unchanged_stories() { + let (mut review, mut doc) = started(TEXT); + let words = + |review: &Review, doc: &Doc| -> Vec { review.marks(doc.document(), |story| doc.version(story)).map(|i| i.word.clone()).collect() }; + assert_eq!(words(&review, &doc), ["jumpd", "jumpd", "wrng"]); + review.act(&mut doc, Action::IgnoreOnce).unwrap(); + assert_eq!(words(&review, &doc), ["jumpd", "wrng"], "an ignored occurrence isn't marked"); + review.act(&mut doc, Action::IgnoreAll).unwrap(); + assert_eq!(words(&review, &doc), ["wrng"]); + review.act(&mut doc, Action::IgnoreAll).unwrap(); + assert!(words(&review, &doc).is_empty()); + assert!(review.covers(doc.document(), 7, doc.version(7)), "it still speaks for a story of ignored words"); + assert!(!review.covers(doc.document(), 8, doc.version(8)), "not for one it hasn't seen"); + review.act(&mut doc, Action::Check).unwrap(); + doc.edit(0, "A "); + assert!(!review.covers(doc.document(), 7, doc.version(7))); + assert!(words(&review, &doc).is_empty(), "offsets into changed text aren't drawn"); + review.act(&mut doc, Action::Check).unwrap(); + doc.id = 2; + assert!(words(&review, &doc).is_empty(), "nor into another document"); + assert_eq!(Review::default().marks(None, |_| None).count(), 0); } #[test] fn every_string_is_listed_once() { for source in - ["Spelling", "Preferred Form", "Suggestions", "Ignore Once", "Ignore All", "Add to Dictionary", "Change…", "Change to:", "Check Again"] + ["Spelling", "Preferred Form", "Suggestions", "Ignore Once", "Ignore All", "Add to Dictionary", "{count} left", "Change to:", "Check Again"] { assert!(STRINGS.contains(&source), "{source}"); } diff --git a/crates/proof-ui/src/ui.rs b/crates/proof-ui/src/ui.rs index 5c0ad067d..e36e96943 100644 --- a/crates/proof-ui/src/ui.rs +++ b/crates/proof-ui/src/ui.rs @@ -1,14 +1,15 @@ //! The panel and the right-click section. Looks follow the host's theme through [`Colors`]. use std::borrow::Cow; -use egui::{Align2, Color32, FontId, RichText, Sense, Stroke, Ui, vec2}; +use egui::{Align, Align2, Color32, FontId, Layout, RichText, Sense, Stroke, TextStyle, TextWrapMode, Ui, vec2}; use crate::review::{CHANGED, NO_ISSUE, NOT_SPELLING, OTHER_DOCUMENT, SAME_REPLACEMENT}; use crate::{Action, Host, Issue, IssueKind, Review}; /// The application's translations. pub trait Text { - /// A string from [`STRINGS`], by its English source text. + /// A string from [`STRINGS`], by its English source text. Anything else, such as a host's + /// error message, comes back unchanged. fn ui<'a>(&'a self, source: &'a str) -> Cow<'a, str>; /// A label put into display order (the app's bidi reordering). fn visual(&self, s: &str) -> String { @@ -25,7 +26,7 @@ pub const STRINGS: &[&str] = &[ "Ignore Once", "Ignore All", "Add to Dictionary", - "Change…", + "{count} left", "Change to:", "Change", "Check Spelling", @@ -50,6 +51,11 @@ pub struct Colors { pub field: Color32, pub field_border: Color32, pub hover: Color32, + /// The suggestion chosen in the list. + pub selected: Color32, + /// The main action's button, and its text. + pub accent: Color32, + pub on_accent: Color32, /// The underline of a misspelled word. pub spelling: Color32, /// The underline of a preferred form. @@ -61,8 +67,35 @@ fn label(ui: &mut Ui, text: &dyn Text, source: &str, size: f32, color: Color32) ui.label(RichText::new(text.visual(&text.ui(source))).size(size).color(color)) } -fn link(ui: &mut Ui, text: &dyn Text, source: &str, colors: &Colors) -> bool { - ui.add(egui::Button::new(RichText::new(text.visual(&text.ui(source))).color(colors.text).underline()).frame(false)).clicked() +fn tr(text: &dyn Text, source: &str) -> String { + text.visual(&text.ui(source)) +} + +/// Tall enough for a label in Khmer or Arabic. +const BUTTON: f32 = 24.0; +const BUTTON_GAP: f32 = 6.0; +/// The least of the review left to scroll above actions kept in view. +const MIN_BODY: f32 = 48.0; +/// Below actions kept in view, clear of the host's frame. +const BOTTOM_MARGIN: f32 = 2.0; + +fn button(ui: &mut Ui, label: &str, width: f32) -> bool { + ui.add(egui::Button::new(label).min_size(vec2(width, BUTTON)).wrap_mode(TextWrapMode::Truncate)).clicked() +} + +/// Across the panel, its label centered. +fn wide_button(ui: &mut Ui, label: &str) -> bool { + let width = ui.available_width(); + ui.with_layout(Layout::top_down(Align::Center), |ui| button(ui, label, width)).inner +} + +/// The one action this state is for, across the panel. +fn primary(ui: &mut Ui, label: &str, enabled: bool, colors: &Colors) -> bool { + let button = egui::Button::new(RichText::new(label).color(colors.on_accent)) + .fill(colors.accent) + .min_size(vec2(ui.available_width(), BUTTON)) + .wrap_mode(TextWrapMode::Truncate); + ui.with_layout(Layout::top_down(Align::Center), |ui| ui.add_enabled(enabled, button).clicked()).inner } fn context(ui: &mut Ui, review: &Review, issue: &Issue, colors: &Colors) { @@ -70,7 +103,7 @@ fn context(ui: &mut Ui, review: &Review, issue: &Issue, colors: &Colors) { let mark = if issue.kind == IssueKind::Spelling { colors.spelling } else { colors.grammar }; let mut job = egui::text::LayoutJob::default(); job.append(&review.before, 0.0, format.clone()); - job.append(&issue.word, 0.0, egui::TextFormat { underline: Stroke::new(1.5, mark), ..format.clone() }); + job.append(&issue.word, 0.0, egui::TextFormat { underline: Stroke::new(1.5, mark), background: mark.gamma_multiply(0.22), ..format.clone() }); job.append(&review.after, 0.0, format); let width = ui.available_width(); egui::Frame::NONE.fill(colors.field).stroke(Stroke::new(1.0, colors.field_border)).corner_radius(2.0).inner_margin(8).show(ui, |ui| { @@ -79,11 +112,13 @@ fn context(ui: &mut Ui, review: &Review, issue: &Issue, colors: &Colors) { }); } -/// A full-width row that applies its suggestion when clicked. -fn suggestion_row(ui: &mut Ui, suggestion: &str, colors: &Colors) -> bool { +/// A full-width row of the suggestion list. +fn suggestion_row(ui: &mut Ui, suggestion: &str, selected: bool, colors: &Colors) -> egui::Response { let (rect, response) = ui.allocate_exact_size(vec2(ui.available_width(), 25.0), Sense::click()); - response.widget_info(|| egui::WidgetInfo::labeled(egui::Role::Button, true, suggestion)); - if response.hovered() { + response.widget_info(|| egui::WidgetInfo::selected(egui::Role::Button, true, selected, suggestion)); + if selected { + ui.painter().rect_filled(rect, 0.0, colors.selected); + } else if response.hovered() { ui.painter().rect_filled(rect, 0.0, colors.hover); } ui.painter().with_clip_rect(rect.shrink2(vec2(8.0, 0.0))).text( @@ -93,7 +128,7 @@ fn suggestion_row(ui: &mut Ui, suggestion: &str, colors: &Colors) -> bool { FontId::proportional(13.0), colors.text, ); - response.clicked() + response } /// What to do after the panel has been drawn, so that nothing changes under its borrows. @@ -102,9 +137,37 @@ enum After { Act(Action), } -/// Draws the review. Starts one from its own button if there is none yet. +/// Draws the review. Starts one from its own button if there is none yet. The current issue's +/// actions keep their place at the bottom, so that they stay in view when a short panel has to +/// scroll the rest, and don't move from one issue to the next under a pointer going through them. +/// Give it the height to fill, not a scrolling parent. pub fn panel(ui: &mut Ui, review: &mut Review, host: &mut dyn Host, text: &dyn Text, colors: &Colors) { - let after = egui::ScrollArea::vertical().auto_shrink([false, true]).show(ui, |ui| body(ui, review, &*host, text, colors)).inner; + let actionable = actionable_issue(review, &*host); + let height = ui.available_height(); + let reserve = actionable.as_ref().map_or(0.0, |_| BUTTON_GAP + actions_height(ui, text) + BOTTOM_MARGIN); + let pinned = actionable.is_some() && height.is_finite() && height >= reserve + MIN_BODY; + let scroll = if pinned { + egui::ScrollArea::vertical().auto_shrink([false, false]).max_height(height - reserve).min_scrolled_height(height - reserve) + } else { + egui::ScrollArea::vertical().auto_shrink([false, true]) + }; + let mut after = scroll + .show(ui, |ui| { + let after = body(ui, review, &*host, text, colors); + match &actionable { + Some(issue) if !pinned => { + ui.add_space(BUTTON_GAP); + actions(ui, review, issue, text, colors).or(after) + } + _ => after, + } + }) + .inner; + if pinned && let Some(issue) = &actionable { + ui.add_space(BUTTON_GAP); + after = actions(ui, review, issue, text, colors).or(after); + ui.add_space(BOTTOM_MARGIN); + } match after { Some(After::Start) => review.start(host), Some(After::Act(action)) => { @@ -115,16 +178,29 @@ pub fn panel(ui: &mut Ui, review: &mut Review, host: &mut dyn Host, text: &dyn T } } +/// The current issue, if the panel can act on it. +fn actionable_issue(review: &Review, host: &dyn Host) -> Option { + if !review.started() || review.other_document(host) || must_check_again(review, host) { + return None; + } + review.current().cloned() +} + +/// The text under the current issue changed, or a fresh check no longer finds the issue. +fn must_check_again(review: &Review, host: &dyn Host) -> bool { + review.stale(host) || (review.current().is_some() && review.message() == CHANGED) +} + fn body(ui: &mut Ui, review: &mut Review, host: &dyn Host, text: &dyn Text, colors: &Colors) -> Option { if !review.started() { label(ui, text, "Check the spelling of the whole document.", 12.0, colors.text); ui.add_space(8.0); - return ui.button(text.visual(&text.ui("Check Spelling"))).clicked().then_some(After::Start); + return primary(ui, &tr(text, "Check Spelling"), true, colors).then_some(After::Start); } if review.other_document(host) { label(ui, text, OTHER_DOCUMENT, 12.0, colors.text); ui.add_space(8.0); - return ui.button(text.visual(&text.ui("Check This Document"))).clicked().then_some(After::Start); + return primary(ui, &tr(text, "Check This Document"), true, colors).then_some(After::Start); } if let Some(notice) = host.notice() { ui.label(RichText::new(notice).size(11.0).color(colors.dim)); @@ -134,15 +210,17 @@ fn body(ui: &mut Ui, review: &mut Review, host: &dyn Host, text: &dyn Text, colo match review.current().cloned() { Some(issue) => after = current_issue(ui, review, &issue, host, text, colors), None => { - let done = if review.ignored_any() { "The spelling check is complete. Some issues were ignored." } else { "No issues found." }; - label(ui, text, done, 12.0, colors.text); - ui.add_space(8.0); - if link(ui, text, "Check Again", colors) { + if review.message().is_empty() { + let done = if review.ignored_any() { "The spelling check is complete. Some issues were ignored." } else { "No issues found." }; + label(ui, text, done, 12.0, colors.text); + ui.add_space(8.0); + } + if wide_button(ui, &tr(text, "Check Again")) { after = Some(After::Act(Action::Check)); } } } - if !review.message().is_empty() { + if !review.message().is_empty() && !must_check_again(review, host) { ui.add_space(8.0); label(ui, text, review.message(), 12.0, colors.error); } @@ -151,59 +229,109 @@ fn body(ui: &mut Ui, review: &mut Review, host: &dyn Host, text: &dyn Text, colo fn current_issue(ui: &mut Ui, review: &mut Review, issue: &Issue, host: &dyn Host, text: &dyn Text, colors: &Colors) -> Option { let spelling = issue.kind == IssueKind::Spelling; - let heading = text.visual(&text.ui(if spelling { "Spelling" } else { "Preferred Form" })); - ui.label(RichText::new(heading).size(14.0).strong().color(colors.text)); + ui.horizontal(|ui| { + ui.label(RichText::new(tr(text, if spelling { "Spelling" } else { "Preferred Form" })).size(14.0).strong().color(colors.text)); + ui.with_layout(Layout::right_to_left(Align::Center), |ui| { + let left = text.ui("{count} left").replace("{count}", &review.remaining().to_string()); + ui.label(RichText::new(text.visual(&left)).size(11.0).color(colors.dim)); + }); + }); if !issue.message.is_empty() { ui.label(RichText::new(&issue.message).size(11.0).color(colors.dim)); } ui.add_space(8.0); context(ui, review, issue, colors); ui.add_space(12.0); - if review.stale(host) { + if must_check_again(review, host) { label(ui, text, CHANGED, 12.0, colors.text); ui.add_space(8.0); - return link(ui, text, "Check Again", colors).then_some(After::Act(Action::Check)); + return primary(ui, &tr(text, "Check Again"), true, colors).then_some(After::Act(Action::Check)); } let mut after = None; + let name = label(ui, text, "Change to:", 11.0, colors.dim); + let field = + egui::TextEdit::singleline(&mut review.replacement).font(FontId::proportional(13.0)).margin(vec2(8.0, 5.0)).desired_width(f32::INFINITY); + let field = ui.add(field).labelled_by(name.id); + if field.lost_focus() && ui.input(|i| i.key_pressed(egui::Key::Enter)) && can_change(review, issue) { + after = Some(After::Act(Action::Change(None))); + } + ui.add_space(8.0); label(ui, text, "Suggestions", 11.0, colors.dim); if issue.suggestions.is_empty() { - ui.label(RichText::new(text.visual(&text.ui("(no suggestions)"))).italics().color(colors.text)); + ui.label(RichText::new(tr(text, "(no suggestions)")).italics().color(colors.text)); } else { egui::Frame::NONE.fill(colors.field).stroke(Stroke::new(1.0, colors.field_border)).corner_radius(2.0).show(ui, |ui| { ui.spacing_mut().item_spacing.y = 0.0; for suggestion in &issue.suggestions { - if suggestion_row(ui, suggestion, colors) { + let row = suggestion_row(ui, suggestion, *suggestion == review.replacement, colors); + if row.clicked() { + review.replacement.clone_from(suggestion); + } + if row.double_clicked() { after = Some(After::Act(Action::Change(Some(suggestion.clone())))); } } }); } - ui.add_space(8.0); - // On wrapping lines: a docked panel is often too short for a column of them to stay in view. - ui.horizontal_wrapped(|ui| { - if link(ui, text, "Ignore Once", colors) { - after = Some(After::Act(Action::IgnoreOnce)); + after +} + +fn can_change(review: &Review, issue: &Issue) -> bool { + !review.replacement.is_empty() && review.replacement != issue.word +} + +/// Ignore Once and Ignore All don't fit side by side, in a narrow panel or a long translation. +fn ignores_stacked(ui: &Ui, text: &dyn Text) -> bool { + let font = TextStyle::Button.resolve(ui.style()); + let pad = 2.0 * ui.spacing().button_padding.x + 4.0; + let width = |source: &str| ui.fonts_mut(|fonts| fonts.layout_no_wrap(tr(text, source), font.clone(), Color32::WHITE).size().x) + pad; + (ui.available_width() - BUTTON_GAP) / 2.0 < width("Ignore Once").max(width("Ignore All")) +} + +/// The same for either kind of issue, so that no button lands under the pointer on another's place. +fn actions_height(ui: &Ui, text: &dyn Text) -> f32 { + let rows: u8 = if ignores_stacked(ui, text) { 4 } else { 3 }; + f32::from(rows) * BUTTON + f32::from(rows - 1) * BUTTON_GAP +} + +/// Change, then what else can be done with the issue. A preferred form's word is known, so it +/// can't be ignored for good or join the dictionary, and those rows stay empty. +fn actions(ui: &mut Ui, review: &Review, issue: &Issue, text: &dyn Text, colors: &Colors) -> Option { + let mut after = None; + ui.scope(|ui| { + ui.spacing_mut().item_spacing = vec2(BUTTON_GAP, BUTTON_GAP); + if primary(ui, &tr(text, "Change"), can_change(review, issue), colors) { + after = Some(After::Act(Action::Change(None))); } - if spelling && link(ui, text, "Ignore All", colors) { - after = Some(After::Act(Action::IgnoreAll)); + if issue.kind != IssueKind::Spelling { + if wide_button(ui, &tr(text, "Ignore Once")) { + after = Some(After::Act(Action::IgnoreOnce)); + } + ui.add_space(actions_height(ui, text) - 2.0 * BUTTON - 2.0 * BUTTON_GAP); + return; } - if spelling && link(ui, text, "Add to Dictionary", colors) { - after = Some(After::Act(Action::AddToDictionary)); + if ignores_stacked(ui, text) { + if wide_button(ui, &tr(text, "Ignore Once")) { + after = Some(After::Act(Action::IgnoreOnce)); + } + if wide_button(ui, &tr(text, "Ignore All")) { + after = Some(After::Act(Action::IgnoreAll)); + } + } else { + ui.horizontal(|ui| { + let half = (ui.available_width() - BUTTON_GAP) / 2.0; + if button(ui, &tr(text, "Ignore Once"), half) { + after = Some(After::Act(Action::IgnoreOnce)); + } + if button(ui, &tr(text, "Ignore All"), half) { + after = Some(After::Act(Action::IgnoreAll)); + } + }); } - if !review.manual && link(ui, text, "Change…", colors) { - review.manual = true; + if wide_button(ui, &tr(text, "Add to Dictionary")) { + after = Some(After::Act(Action::AddToDictionary)); } }); - if review.manual { - ui.add_space(8.0); - let name = label(ui, text, "Change to:", 12.0, colors.text); - let field = ui.add(egui::TextEdit::singleline(&mut review.replacement).desired_width(f32::INFINITY)).labelled_by(name.id); - let enter = field.lost_focus() && ui.input(|i| i.key_pressed(egui::Key::Enter)); - let button = egui::Button::new(text.visual(&text.ui("Change"))); - if (ui.add_enabled(!review.replacement.is_empty(), button).clicked() || enter) && !review.replacement.is_empty() { - after = Some(After::Act(Action::Change(None))); - } - } after } From 1dbdcdc1b2fb000b7407c031018287031cc927d3 Mon Sep 17 00:00:00 2001 From: Nathan Wells Date: Tue, 3 Nov 2026 21:31:23 -0800 Subject: [PATCH 4/6] craft-proof-ui: the suggestion list fills its room and scrolls inside itself With the actions kept at the bottom, a short list left a gap above them and a long one scrolled the whole review. The list now reaches down to the actions and scrolls on its own, under a Change to field that stays put. A failure shows above the issue, so that it doesn't take the list's room. Co-Authored-By: Claude Opus 5.5 --- crates/proof-ui/src/tests.rs | 40 +++++++++++++++++++++++++++ crates/proof-ui/src/ui.rs | 53 +++++++++++++++++++++++------------- 2 files changed, 74 insertions(+), 19 deletions(-) diff --git a/crates/proof-ui/src/tests.rs b/crates/proof-ui/src/tests.rs index 877eacfd0..49efc202d 100644 --- a/crates/proof-ui/src/tests.rs +++ b/crates/proof-ui/src/tests.rs @@ -227,6 +227,46 @@ fn the_actions_keep_their_place_from_one_issue_to_the_next() { assert_eq!(h.get_by_label("Ignore Once").rect(), place); } +/// No gap above the actions, and a list too long for the room scrolls inside itself. +#[test] +fn the_suggestion_list_fills_the_room_and_scrolls_inside_itself() { + let height = 420.0; + let panel_of = |many: usize| { + let mut doc = Doc::new(TEXT); + doc.many = Some((0..many).map(|i| format!("jump{i}")).collect()); + let mut review = Review::default(); + review.start(&mut doc); + let mut h = Harness::builder() + .with_size(egui::vec2(320.0, height)) + .build_ui_state(|ui, (review, doc): &mut State| panel(ui, review, doc, &English, &COLORS), (review, doc)); + h.run_steps(2); + h + }; + let long = panel_of(40); + let change = long.get_by_label("Change").rect(); + assert!(long.get_by_label("Change to:").rect().min.y >= 0.0, "the field above the list isn't scrolled away"); + let last_row = (0..40).map(|i| long.get_by_label(&format!("jump{i}")).rect()).find(|row| row.max.y >= change.min.y - 12.0); + assert!(last_row.is_some_and(|row| row.min.y < change.min.y), "rows reach the actions"); + assert!(long.get_by_label("jump39").rect().min.y > height, "the rest is scrolled, not squeezed"); + assert_eq!(panel_of(2).get_by_label("Change").rect(), change, "the actions don't move with the list"); + let mut long = long; + let field = long.get_by_label("Change to:").rect(); + let row = long.get_by_label("jump0").rect(); + long.hover_at(row.center()); + long.step(); + long.event(egui::Event::MouseWheel { + unit: egui::MouseWheelUnit::Point, + delta: egui::vec2(0.0, -60.0), + phase: egui::TouchPhase::Move, + modifiers: egui::Modifiers::NONE, + source: egui::MouseWheelSource::Unknown, + }); + long.run_steps(4); + assert!(long.get_by_label("jump0").rect().min.y < row.min.y, "the list scrolled"); + assert_eq!(long.get_by_label("Change to:").rect(), field, "and only the list"); + assert_eq!(long.get_by_label("Change").rect(), change); +} + /// Going through issues, a click on Ignore Once must not land on Add to Dictionary. #[test] fn a_preferred_form_keeps_ignore_once_in_its_place() { diff --git a/crates/proof-ui/src/ui.rs b/crates/proof-ui/src/ui.rs index e36e96943..4d35a4bc1 100644 --- a/crates/proof-ui/src/ui.rs +++ b/crates/proof-ui/src/ui.rs @@ -76,6 +76,10 @@ const BUTTON: f32 = 24.0; const BUTTON_GAP: f32 = 6.0; /// The least of the review left to scroll above actions kept in view. const MIN_BODY: f32 = 48.0; +/// A row of the suggestion list. +const ROW: f32 = 25.0; +/// The suggestion list's least height, when the panel is too short to give it more. +const MIN_LIST: f32 = 2.0 * ROW; /// Below actions kept in view, clear of the host's frame. const BOTTOM_MARGIN: f32 = 2.0; @@ -114,7 +118,7 @@ fn context(ui: &mut Ui, review: &Review, issue: &Issue, colors: &Colors) { /// A full-width row of the suggestion list. fn suggestion_row(ui: &mut Ui, suggestion: &str, selected: bool, colors: &Colors) -> egui::Response { - let (rect, response) = ui.allocate_exact_size(vec2(ui.available_width(), 25.0), Sense::click()); + let (rect, response) = ui.allocate_exact_size(vec2(ui.available_width(), ROW), Sense::click()); response.widget_info(|| egui::WidgetInfo::selected(egui::Role::Button, true, selected, suggestion)); if selected { ui.painter().rect_filled(rect, 0.0, colors.selected); @@ -206,6 +210,10 @@ fn body(ui: &mut Ui, review: &mut Review, host: &dyn Host, text: &dyn Text, colo ui.label(RichText::new(notice).size(11.0).color(colors.dim)); ui.add_space(8.0); } + if !review.message().is_empty() && !must_check_again(review, host) { + label(ui, text, review.message(), 12.0, colors.error); + ui.add_space(8.0); + } let mut after = None; match review.current().cloned() { Some(issue) => after = current_issue(ui, review, &issue, host, text, colors), @@ -220,10 +228,6 @@ fn body(ui: &mut Ui, review: &mut Review, host: &dyn Host, text: &dyn Text, colo } } } - if !review.message().is_empty() && !must_check_again(review, host) { - ui.add_space(8.0); - label(ui, text, review.message(), 12.0, colors.error); - } after } @@ -257,22 +261,33 @@ fn current_issue(ui: &mut Ui, review: &mut Review, issue: &Issue, host: &dyn Hos } ui.add_space(8.0); label(ui, text, "Suggestions", 11.0, colors.dim); - if issue.suggestions.is_empty() { - ui.label(RichText::new(tr(text, "(no suggestions)")).italics().color(colors.text)); - } else { - egui::Frame::NONE.fill(colors.field).stroke(Stroke::new(1.0, colors.field_border)).corner_radius(2.0).show(ui, |ui| { - ui.spacing_mut().item_spacing.y = 0.0; - for suggestion in &issue.suggestions { - let row = suggestion_row(ui, suggestion, *suggestion == review.replacement, colors); - if row.clicked() { - review.replacement.clone_from(suggestion); + // Down to the actions, scrolling inside itself: nothing between the list and them. + let room = ui.available_height(); + let natural = ROW * issue.suggestions.len().max(1) as f32; + // Short of the very bottom, or the area scrolling this would clip the list's own border. + let height = if room.is_finite() { (room - 2.0).max(MIN_LIST) } else { natural }; + egui::Frame::NONE.fill(colors.field).stroke(Stroke::new(1.0, colors.field_border)).corner_radius(2.0).show(ui, |ui| { + ui.spacing_mut().item_spacing.y = 0.0; + egui::ScrollArea::vertical().id_salt("suggestions").auto_shrink([false, false]).max_height(height).min_scrolled_height(height).show( + ui, + |ui| { + if issue.suggestions.is_empty() { + egui::Frame::NONE.inner_margin(8).show(ui, |ui| { + ui.label(RichText::new(tr(text, "(no suggestions)")).italics().color(colors.text)); + }); } - if row.double_clicked() { - after = Some(After::Act(Action::Change(Some(suggestion.clone())))); + for suggestion in &issue.suggestions { + let row = suggestion_row(ui, suggestion, *suggestion == review.replacement, colors); + if row.clicked() { + review.replacement.clone_from(suggestion); + } + if row.double_clicked() { + after = Some(After::Act(Action::Change(Some(suggestion.clone())))); + } } - } - }); - } + }, + ); + }); after } From 3b30656f84d372af6d0df1b126eee85c6b87c3ef Mon Sep 17 00:00:00 2001 From: Nathan Wells Date: Tue, 3 Nov 2026 22:07:36 -0800 Subject: [PATCH 5/6] craft-proof-ui: keep a pass through failed checks, and open each list at the top From review: Check Again checks before forgetting what was ignored, so a failed check keeps the pass. A failed check after Add to Dictionary only offers Check Again, as the word is already accepted. Each issue has its own list scroll, as the next issue's best suggestions opened scrolled out of view. The panel says how a very short panel behaves, with a test. Co-Authored-By: Claude Opus 5.5 --- crates/proof-ui/src/review.rs | 6 ++- crates/proof-ui/src/tests.rs | 86 +++++++++++++++++++++++++++++++++++ crates/proof-ui/src/ui.rs | 15 +++--- 3 files changed, 100 insertions(+), 7 deletions(-) diff --git a/crates/proof-ui/src/review.rs b/crates/proof-ui/src/review.rs index 70f1b7e36..31b48a9ef 100644 --- a/crates/proof-ui/src/review.rs +++ b/crates/proof-ui/src/review.rs @@ -112,11 +112,13 @@ impl Review { return Err(OTHER_DOCUMENT.into()); } if action == Action::Check { + // A failed check leaves this pass as it was. + let found = host.check()?; self.started = true; self.document = host.document(); self.skipped.clear(); self.ignored.clear(); - return self.refresh(host); + return self.update(host, found); } let issue = self.current().cloned().ok_or(NO_ISSUE)?; let found = self.validate(host, &issue)?; @@ -142,6 +144,8 @@ impl Review { return Err(NOT_SPELLING.into()); } host.add_word(&issue.word)?; + // The word is accepted, so its issue can't be acted on again: only checking is left. + return self.refresh(host).map_err(|_| CHANGED.to_string()); } Action::Check => {} } diff --git a/crates/proof-ui/src/tests.rs b/crates/proof-ui/src/tests.rs index 49efc202d..02aa5731d 100644 --- a/crates/proof-ui/src/tests.rs +++ b/crates/proof-ui/src/tests.rs @@ -25,6 +25,8 @@ struct Doc { preferred: bool, many: Option>, fail_check: bool, + /// `check` fails after this many more successes. + checks_left: Option, } impl Doc { @@ -39,6 +41,10 @@ impl Doc { impl Host for Doc { fn check(&mut self) -> Result, String> { + if let Some(left) = &mut self.checks_left { + self.fail_check = *left == 0; + *left = left.saturating_sub(1); + } if self.fail_check { return Err("the dictionary could not be read".into()); } @@ -476,6 +482,86 @@ fn ignoring_takes_in_what_a_fresh_check_found() { assert_eq!(review.current().unwrap().word, "wrng"); } +#[test] +fn a_failed_check_again_keeps_what_was_ignored() { + let (mut review, mut doc) = started(TEXT); + review.act(&mut doc, Action::IgnoreAll).unwrap(); + assert_eq!(review.current().unwrap().word, "wrng"); + doc.fail_check = true; + review.act(&mut doc, Action::Check).unwrap_err(); + doc.fail_check = false; + review.act(&mut doc, Action::IgnoreOnce).unwrap(); + assert!(review.current().is_none(), "jumpd stays ignored"); +} + +/// The word is in the dictionary, so its issue must not be offered again. +#[test] +fn a_failed_check_after_adding_a_word_only_offers_check_again() { + let (mut review, mut doc) = started(TEXT); + doc.checks_left = Some(1); + assert!(review.act(&mut doc, Action::AddToDictionary).unwrap_err().contains("text changed")); + assert!(doc.known.contains("jumpd")); + doc.checks_left = None; + doc.fail_check = false; + let mut h = harness((review, doc)); + assert!(h.query_by_label("Add to Dictionary").is_none()); + h.get_by_label("Check Again").click(); + h.run_steps(2); + assert_eq!(h.state().0.current().unwrap().word, "wrng"); +} + +#[test] +fn each_issue_opens_its_list_at_the_top() { + let mut doc = Doc::new(TEXT); + doc.many = Some((0..40).map(|i| format!("jump{i}")).collect()); + let mut review = Review::default(); + review.start(&mut doc); + let mut h = Harness::builder() + .with_size(egui::vec2(320.0, 420.0)) + .build_ui_state(|ui, (review, doc): &mut State| panel(ui, review, doc, &English, &COLORS), (review, doc)); + h.run_steps(2); + let row = h.get_by_label("jump0").rect(); + h.hover_at(row.center()); + h.step(); + h.event(egui::Event::MouseWheel { + unit: egui::MouseWheelUnit::Point, + delta: egui::vec2(0.0, -60.0), + phase: egui::TouchPhase::Move, + modifiers: egui::Modifiers::NONE, + source: egui::MouseWheelSource::Unknown, + }); + h.run_steps(4); + assert!(h.get_by_label("jump0").rect().min.y < row.min.y); + h.get_by_label("Ignore Once").click(); + h.run_steps(3); + assert_eq!(h.state().0.current().unwrap().start, 17); + assert_eq!(h.get_by_label("jump0").rect(), row); +} + +/// Too short to keep the actions in view: they scroll with the rest, and are still there. +#[test] +fn a_very_short_panel_still_has_its_actions() { + let mut h = Harness::builder() + .with_size(egui::vec2(320.0, 110.0)) + .build_ui_state(|ui, (review, doc): &mut State| panel(ui, review, doc, &English, &COLORS), started(TEXT)); + h.run_steps(2); + assert!(h.get_by_label("Add to Dictionary").rect().min.y > 110.0); + h.hover_at(egui::pos2(160.0, 20.0)); + h.step(); + h.event(egui::Event::MouseWheel { + unit: egui::MouseWheelUnit::Point, + delta: egui::vec2(0.0, -2000.0), + phase: egui::TouchPhase::Move, + modifiers: egui::Modifiers::NONE, + source: egui::MouseWheelSource::Unknown, + }); + h.run_steps(4); + assert!(h.get_by_label("Add to Dictionary").rect().max.y <= 110.0); + h.get_by_label("Ignore All").click(); + h.run_steps(2); + assert_eq!(h.state().0.current().unwrap().word, "wrng"); +} + #[test] fn checking_starts_a_review_that_wasnt_started() { let mut doc = Doc::new(TEXT); diff --git a/crates/proof-ui/src/ui.rs b/crates/proof-ui/src/ui.rs index 4d35a4bc1..95c9b10cc 100644 --- a/crates/proof-ui/src/ui.rs +++ b/crates/proof-ui/src/ui.rs @@ -144,7 +144,8 @@ enum After { /// Draws the review. Starts one from its own button if there is none yet. The current issue's /// actions keep their place at the bottom, so that they stay in view when a short panel has to /// scroll the rest, and don't move from one issue to the next under a pointer going through them. -/// Give it the height to fill, not a scrolling parent. +/// A panel too short for that scrolls the actions with the rest. Give it the height to fill, not a +/// scrolling parent. pub fn panel(ui: &mut Ui, review: &mut Review, host: &mut dyn Host, text: &dyn Text, colors: &Colors) { let actionable = actionable_issue(review, &*host); let height = ui.available_height(); @@ -268,9 +269,12 @@ fn current_issue(ui: &mut Ui, review: &mut Review, issue: &Issue, host: &dyn Hos let height = if room.is_finite() { (room - 2.0).max(MIN_LIST) } else { natural }; egui::Frame::NONE.fill(colors.field).stroke(Stroke::new(1.0, colors.field_border)).corner_radius(2.0).show(ui, |ui| { ui.spacing_mut().item_spacing.y = 0.0; - egui::ScrollArea::vertical().id_salt("suggestions").auto_shrink([false, false]).max_height(height).min_scrolled_height(height).show( - ui, - |ui| { + egui::ScrollArea::vertical() + .id_salt(("suggestions", issue.story, issue.start, &issue.word)) + .auto_shrink([false, false]) + .max_height(height) + .min_scrolled_height(height) + .show(ui, |ui| { if issue.suggestions.is_empty() { egui::Frame::NONE.inner_margin(8).show(ui, |ui| { ui.label(RichText::new(tr(text, "(no suggestions)")).italics().color(colors.text)); @@ -285,8 +289,7 @@ fn current_issue(ui: &mut Ui, review: &mut Review, issue: &Issue, host: &dyn Hos after = Some(After::Act(Action::Change(Some(suggestion.clone())))); } } - }, - ); + }); }); after } From 12c2eb7e9db0bc4215ff034f7ad53ea83cd35b39 Mon Sep 17 00:00:00 2001 From: Nathan Wells Date: Tue, 3 Nov 2026 22:32:32 -0800 Subject: [PATCH 6/6] craft-proof-ui: ask for the notice at each check, not each frame Host::notice may read a whole document, and the panel asked on every frame. Co-Authored-By: Claude Opus 5.5 --- crates/proof-ui/src/lib.rs | 1 + crates/proof-ui/src/review.rs | 3 +++ crates/proof-ui/src/ui.rs | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/proof-ui/src/lib.rs b/crates/proof-ui/src/lib.rs index bbf9cc0dc..bac901362 100644 --- a/crates/proof-ui/src/lib.rs +++ b/crates/proof-ui/src/lib.rs @@ -55,6 +55,7 @@ pub trait Host { /// Accept the word from now on. fn add_word(&mut self, word: &str) -> Result<(), String>; /// A line above the review about what it can't cover (a language with no data), if any. + /// Asked at each check, not each frame. fn notice(&self) -> Option { None } diff --git a/crates/proof-ui/src/review.rs b/crates/proof-ui/src/review.rs index 31b48a9ef..b77319973 100644 --- a/crates/proof-ui/src/review.rs +++ b/crates/proof-ui/src/review.rs @@ -38,6 +38,8 @@ pub struct Review { /// The Change to field's text. pub replacement: String, message: String, + /// [`Host::notice`] at the last check: asked once, as it may read a whole document. + pub(crate) notice: Option, started: bool, } @@ -191,6 +193,7 @@ impl Review { /// Take `found`, a check of the document as it is now, as the issues left. fn update(&mut self, host: &mut dyn Host, found: Vec) -> Result<(), String> { + self.notice = host.notice(); let versions = &self.versions; self.skipped.retain(|skip| versions.get(&skip.story).is_none_or(|v| host.version(skip.story) == Some(*v))); let stories: HashSet = found.iter().chain(&self.skipped).map(|i| i.story).collect(); diff --git a/crates/proof-ui/src/ui.rs b/crates/proof-ui/src/ui.rs index 95c9b10cc..0154df999 100644 --- a/crates/proof-ui/src/ui.rs +++ b/crates/proof-ui/src/ui.rs @@ -207,7 +207,7 @@ fn body(ui: &mut Ui, review: &mut Review, host: &dyn Host, text: &dyn Text, colo ui.add_space(8.0); return primary(ui, &tr(text, "Check This Document"), true, colors).then_some(After::Start); } - if let Some(notice) = host.notice() { + if let Some(notice) = &review.notice { ui.label(RichText::new(notice).size(11.0).color(colors.dim)); ui.add_space(8.0); }