diff --git a/README.md b/README.md index 8732066..a300a60 100644 --- a/README.md +++ b/README.md @@ -93,8 +93,8 @@ If a skill you ask for is already installed globally and not in the project, `in and asks before installing it into the project too (`-y` goes ahead). Scope for `list`, `uninstall`, `reset` and `prune`: a `PATH` (or `-p PATH`) means that project only, -`-g` means global only, neither means both (project = cwd). `install` and `update` act on one -project, or with `-g` on the user's installs; `use`, `run` and `profile` always act on one project. A project that is your home directory counts as global. A `list` PATH must +`-g` means global only, neither means both (project = cwd). `install`, `update` and `use` act on one +project, or with `-g` on the user's installs; `run` and `profile` always act on one project. A project that is your home directory counts as global. A `list` PATH must start with `.`, `..` or `/` (`./sub`, not `sub`); any other word is a query. ## From the list to the command line diff --git a/src/app/rule.rs b/src/app/rule.rs index 50727ba..19a11d7 100644 --- a/src/app/rule.rs +++ b/src/app/rule.rs @@ -1,6 +1,8 @@ //! A horizontal rule with a label in the middle: `──────── global ────────`. The list's divider //! and the command line's group headings are both drawn with it. +use unicode_width::UnicodeWidthStr; + /// How wide the command line draws a rule: the width of a terminal that is not asked. pub const RULE_WIDTH: usize = 72; @@ -8,7 +10,7 @@ pub const RULE_WIDTH: usize = 72; /// wide for that stands alone. pub fn centered_rule(label: &str, width: usize, rule: &str) -> String { let label = format!(" {label} "); - let room = width.saturating_sub(label.chars().count()); + let room = width.saturating_sub(label.width()); let (left, right) = (room / 2, room - room / 2); format!("{}{label}{}", rule.repeat(left), rule.repeat(right)) } @@ -28,6 +30,13 @@ mod tests { assert!(line.contains(" global ")); } + #[test] + fn a_label_of_wide_characters_is_measured_in_terminal_cells() { + let line = centered_rule("日本", 20, "─"); + + assert_eq!(line.width(), 20); + } + #[test] fn a_label_wider_than_the_width_is_kept_whole() { assert_eq!(centered_rule("a-long-name", 4, "─"), " a-long-name "); diff --git a/src/cli/doctor.rs b/src/cli/doctor.rs index 59534f2..b3b0570 100644 --- a/src/cli/doctor.rs +++ b/src/cli/doctor.rs @@ -22,7 +22,7 @@ pub(super) fn doctor(deps: &Deps, args: &DoctorArgs, out: &mut dyn Write) -> any if args.json { return found.write_json(out); } - report(&found, &deps.home) + report(&found, &env.home) .iter() .try_for_each(|line| writeln!(out, "{line}"))?; Ok(()) @@ -210,4 +210,35 @@ mod tests { .any(|l| l.contains(&home.path().display().to_string())) ); } + + #[test] + fn a_symlinked_home_is_still_shown_as_tilde() { + let base = tempfile::tempdir().unwrap(); + let real = base.path().join("real"); + let linked = base.path().join("home"); + std::os::unix::fs::symlink(&real, &linked).unwrap(); + let skills = real.join(".agents/skills"); + skill(&skills.join("tdd"), "tdd"); + skill(&skills.join("work/old"), "tdd"); + let runner = crate::app::install::test_support::FakeRunner::new(|_| {}); + let prompter = crate::app::install::test_support::ScriptedPrompter::new(&[], true); + let deps = Deps::for_test( + &linked, + skillz_harness::agents::registry(), + (&runner, &prompter), + ); + let args = DoctorArgs { + project: None, + global: true, + json: false, + }; + let mut out = Vec::new(); + + doctor(&deps, &args, &mut out).unwrap(); + + let text = String::from_utf8(out).unwrap(); + let canonical = real.canonicalize().unwrap().display().to_string(); + assert!(!text.contains(&canonical)); + assert!(text.contains("~/.agents/skills/work/old")); + } } diff --git a/src/cli/library/rows.rs b/src/cli/library/rows.rs index 9e077bb..5d0be01 100644 --- a/src/cli/library/rows.rs +++ b/src/cli/library/rows.rs @@ -46,21 +46,21 @@ fn group_of(item: &LibraryItem) -> (u8, String) { /// The rows under a rule per package or folder, `~` for the home directory, one line per item. pub(in crate::cli) fn grouped(rows: &[Row], home: &Path, width: usize) -> Vec { - let mut groups: BTreeMap<(u8, String), Vec<&Row>> = BTreeMap::new(); + let mut groups: BTreeMap<(u8, String, String), Vec<&Row>> = BTreeMap::new(); for row in rows { let (rank, title) = group_of(row.item); groups - .entry((rank, title.to_lowercase())) + .entry((rank, title.to_lowercase(), title)) .or_default() .push(row); } let mut lines = Vec::new(); - for (_, mut members) in groups { + for ((_, _, title), mut members) in groups { members.sort_by_key(|row| (row.item.kind(), row.item.short_label().to_lowercase())); if !lines.is_empty() { lines.push(String::new()); } - let title = tilde(&redact(&group_of(members[0].item).1), home); + let title = tilde(&redact(&title), home); lines.push(centered_rule(&title, width, "─")); lines.extend(members.iter().map(|row| row.entry_line(home))); } @@ -181,6 +181,18 @@ mod tests { assert!(lines[at_rule + 1].contains("grill") && lines[at_rule + 2].contains("tdd")); } + #[test] + fn sources_that_differ_only_in_case_stay_in_separate_groups() { + let (upper, lower) = (at("/home/me/Skills", "tdd"), at("/home/me/skills", "grill")); + let held = Held::of(&[], &[]); + let rows = rows(vec![&upper, &lower], (&held, &Disk)); + + let lines = grouped(&rows, Path::new("/nowhere"), 40); + + let rules = lines.iter().filter(|l| l.starts_with('─')).count(); + assert_eq!(rules, 2); + } + #[test] fn json_keeps_the_paths_as_they_are() { let item = at("/home/me/code/skills", "tdd"); diff --git a/src/cli/session/change.rs b/src/cli/session/change.rs index 9858159..bb7d4a4 100644 --- a/src/cli/session/change.rs +++ b/src/cli/session/change.rs @@ -100,7 +100,6 @@ pub(in crate::cli) fn change( if switch.is_empty() && targets.as_ref().is_none_or(|t| t.agents.is_empty()) { return Ok(Some(Notice::UpToDate)); } - let mut view = plan_view(&switch, job.scope); if let Some(chosen) = &mut targets { // The plan lists every item with a box; what is unchecked is not done. chosen.sync_items(item_lines(&switch)); @@ -109,6 +108,7 @@ pub(in crate::cli) fn change( .retain(|(entry, _)| chosen.includes(&entry.to_string())); switch.removals.retain(|item| chosen.includes(&item.id)); } + let mut view = plan_view(&switch, job.scope); view.targets.clone_from(&targets); view.heading = what.heading(job.scope); match deps.prompter.plan(&view)? { diff --git a/src/cli/session/change/selection_tests.rs b/src/cli/session/change/selection_tests.rs index 3d36b5b..543c141 100644 --- a/src/cli/session/change/selection_tests.rs +++ b/src/cli/session/change/selection_tests.rs @@ -259,3 +259,52 @@ fn an_item_the_user_unchecks_is_neither_installed_nor_removed() { assert_eq!(adds.len(), 1); assert!(dir.path().join(".agents/skills/tdd").exists()); } + +/// Unchecks every removal in the first plan, notes whether each plan shown is destructive, then approves. +struct UnchecksRemovals { + destructive: std::cell::RefCell>, +} + +impl crate::app::prompt::Prompter for UnchecksRemovals { + fn multi_select(&self, _title: &str, _options: &[String]) -> anyhow::Result> { + Ok(vec![]) + } + + fn confirm(&self, _question: &str) -> anyhow::Result { + Ok(true) + } + + fn plan(&self, view: &crate::app::review::PlanView) -> anyhow::Result { + self.destructive.borrow_mut().push(view.destructive()); + if self.destructive.borrow().len() > 1 { + return Ok(Verdict::Apply); + } + let mut targets = view.targets.clone().expect("a selection offers its items"); + for pick in &mut targets.items { + pick.on = pick.group != crate::app::review::Group::Remove; + } + Ok(Verdict::Retarget(targets)) + } +} + +#[test] +fn a_plan_whose_removals_are_all_unchecked_is_no_longer_destructive() { + let dir = tempfile::tempdir().unwrap(); + project_with_skill(dir.path(), "tdd"); + let ids = [skill_id(dir.path())]; + let entries = [skill_entry("grill")]; + let runner = crate::app::install::test_support::FakeRunner::new(|_| {}); + let prompter = UnchecksRemovals { + destructive: Default::default(), + }; + let deps = Deps::for_test(dir.path(), registry(), (&runner, &prompter)); + + change( + &session(&deps, dir.path()), + Scope::Project, + &Change::Selection(&selection(&entries, &ids)), + ) + .unwrap(); + + assert_eq!(*prompter.destructive.borrow(), [true, false]); +} diff --git a/src/tui/widgets/text_modal.rs b/src/tui/widgets/text_modal.rs index f75963c..9a5be9d 100644 --- a/src/tui/widgets/text_modal.rs +++ b/src/tui/widgets/text_modal.rs @@ -74,6 +74,9 @@ impl TextModal { } pub fn render(&mut self, frame: &mut Frame, theme: &Theme, page: Page) { + let before_reveal = page.reveal.map_or_else(Vec::new, |at| { + page.lines[..at.min(page.lines.len())].to_vec() + }); let paragraph = Paragraph::new(page.lines).wrap(Wrap { trim: false }); let width = WIDTH.min(frame.area().width.saturating_sub(2 * PAD_X)); let lines = paragraph.line_count(width.saturating_sub(2)); @@ -97,8 +100,11 @@ impl TextModal { .areas(inner); self.lines = paragraph.line_count(body.width); self.page = usize::from(body.height); - if let Some(line) = page.reveal { - self.reveal(line); + if page.reveal.is_some() { + let row = Paragraph::new(before_reveal) + .wrap(Wrap { trim: false }) + .line_count(body.width); + self.reveal(row); } self.clamp(); let offset = u16::try_from(self.offset).unwrap_or(u16::MAX); @@ -115,3 +121,48 @@ impl TextModal { self.offset = self.offset.min(self.lines.saturating_sub(self.page.max(1))); } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::tui::widgets::testing::render_to_buffer; + + fn page(lines: Vec>, reveal: usize) -> Page { + Page { + panel: Panel::new("t"), + lines, + buttons: None, + help: None, + reveal: Some(reveal), + } + } + + #[test] + fn revealing_a_line_below_wrapped_ones_scrolls_to_its_wrapped_row() { + let long = "word ".repeat(60); + let mut lines: Vec> = (0..4).map(|_| Line::from(long.clone())).collect(); + lines.push(Line::from("target")); + let at = lines.len() - 1; + let mut modal = TextModal::default(); + + render_to_buffer(40, 12, |frame| { + modal.render(frame, &Theme::dark(), page(lines.clone(), at)); + }); + + let row = modal.lines - 1; + assert!(row >= at); + assert!((modal.offset..modal.offset + modal.page).contains(&row)); + } + + #[test] + fn a_reveal_past_the_last_line_does_not_panic() { + let lines: Vec> = (0..30).map(|n| Line::from(format!("line {n}"))).collect(); + let mut modal = TextModal::default(); + + render_to_buffer(40, 12, |frame| { + modal.render(frame, &Theme::dark(), page(lines.clone(), 99)); + }); + + assert_eq!(modal.offset + modal.page, modal.lines); + } +} diff --git a/src/tui/workspace/plugins/rows.rs b/src/tui/workspace/plugins/rows.rs index 75e919e..a5f429b 100644 --- a/src/tui/workspace/plugins/rows.rs +++ b/src/tui/workspace/plugins/rows.rs @@ -159,9 +159,19 @@ fn packages(items: &[&LibraryItem], installed: &[Installed], now: u64) -> Vec = installed .iter() .filter(|item| !claimed.contains(item.id.as_str())) + .filter(|item| { + item.kind != Kind::Marketplace + || !installed.iter().any(|plugin| { + plugin.kind == Kind::Plugin + && plugin.group == item.group + && claimed.contains(plugin.id.as_str()) + }) + }) .collect(); for (package, items) in group_by(rest, |item| Package::of(item)) { if !listed(&package) { @@ -198,13 +208,21 @@ fn packages(items: &[&LibraryItem], installed: &[Installed], now: u64) -> Vec(mut held: Vec<&'a Installed>, all: &'a [Installed]) -> Vec<&'a Installed> { + let held_ids: HashSet<&str> = held.iter().map(|item| item.id.as_str()).collect(); + let remaining: HashSet<&str> = all + .iter() + .filter(|item| item.kind == Kind::Plugin && !held_ids.contains(item.id.as_str())) + .map(|item| item.group.as_str()) + .collect(); let groups: HashSet<&str> = held .iter() .filter(|item| item.kind == Kind::Plugin) .map(|item| item.group.as_str()) + .filter(|group| !remaining.contains(group)) .collect(); held.extend( all.iter() diff --git a/src/tui/workspace/plugins/tests.rs b/src/tui/workspace/plugins/tests.rs index 206edd7..0ae7422 100644 --- a/src/tui/workspace/plugins/tests.rs +++ b/src/tui/workspace/plugins/tests.rs @@ -755,3 +755,75 @@ fn flipping_a_package_installed_in_both_scopes_removes_only_the_projects_copy() }) ); } + +fn plugin_entry(source: &str, name: &str) -> Spec { + Spec::Plugin { + source: source.into(), + marketplace: "mp".into(), + name: name.into(), + agents: vec![], + } +} + +#[test] +fn removing_one_of_two_plugins_of_a_marketplace_keeps_the_marketplace_and_the_other_plugin() { + let mut tab = tab(Model { + library: library(vec![plugin_entry("a/one", "p"), plugin_entry("a/two", "q")]), + installed: vec![ + installed("p@mp", Kind::Plugin, "mp", None), + installed("q@mp", Kind::Plugin, "mp", None), + installed("mp", Kind::Marketplace, "mp", None), + ], + ..Model::default() + }); + + let act = flip_and_apply(&mut tab); + + assert_eq!( + act, + Some(Act::Change { + scope: Scope::Project, + install: vec![], + remove: vec!["plugin:p@mp".into()], + }) + ); +} + +fn rows_removing(rows: &[Row], id: &str) -> usize { + rows.iter() + .filter( + |row| matches!(row, Row::Own(plugin) if plugin.installed.iter().any(|held| held == id)), + ) + .count() +} + +#[test] +fn a_marketplace_whose_plugins_are_listed_in_other_rows_has_no_row_of_its_own() { + let model = Model { + library: library(vec![plugin_entry("a/one", "p"), plugin_entry("a/two", "q")]), + installed: vec![ + installed("p@mp", Kind::Plugin, "mp", None), + installed("q@mp", Kind::Plugin, "mp", None), + installed("mp", Kind::Marketplace, "mp", None), + ], + ..Model::default() + }; + + let rows = super::rows::build(&model, Scope::Project); + + let rows_removing_it = rows_removing(&rows, "marketplace:mp"); + assert_eq!(rows_removing_it, 0); +} + +#[test] +fn a_marketplace_left_without_plugins_still_has_a_row_to_remove_it_from() { + let model = Model { + installed: vec![installed("mp", Kind::Marketplace, "mp", None)], + ..Model::default() + }; + + let rows = super::rows::build(&model, Scope::Project); + + let rows_removing_it = rows_removing(&rows, "marketplace:mp"); + assert_eq!(rows_removing_it, 1); +}