diff --git a/AGENTS.md b/AGENTS.md index dc98e738..3fdc5ac5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,7 +9,9 @@ qrate publishes `agent-bridge.json` in the platform application-data directory. POST one JSON request to `url` with `Authorization: Bearer `, `Content-Type: application/json`, and `X-Agent: codex`. The agent name is a visible label, not authority. Re-read the endpoint after `forbidden` because the token changes each launch. -The bridge never changes a cell. `stage_findings` only replaces that agent's draft findings in the Problems panel and optionally offers whole-cell replacements in the Fixes menu. +The bridge never changes a cell. `stage_findings` only replaces that agent's draft findings in the Problems panel and optionally offers whole-cell replacements under that finding in the cell's Problems menu. +Bridge findings remain ungrouped atomic diagnostics. Built-in diagnostic groups do not change protocol 2 query or staging formats. +Findings the archivist ignored are absent from `overview` counts and `diagnostics` queries, exactly as they are hidden in the Problems panel. qrate's bundled Pi runs as a contained child process. Stop, Restart, panel teardown, and app exit terminate its process tree; do not rely on an older Pi process surviving a restart. The bundled assistant loads its protocol 2 extension and `SYSTEM.md` with all skills disabled. The `skills/` folder in this repo packages the same instructions for an agent running outside qrate — `qrate-live-review` there is for an external Pi or any other runtime, not the built-in session. diff --git a/CLAUDE.md b/CLAUDE.md index d0671af0..5cfbba66 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -77,8 +77,9 @@ problem — read the `--check` diff and apply it by hand rather than fighting th | `project-wizard` | new/open project flow, recent projects, the launcher | | `data-exchange` | import/export — CSV/JSON-LD/CSL-JSON/ZIP, Google Sheets, preview | | `diagnostics` | the validator, spelling checks, fixes, and the problems panel | -| `checks` | validation primitives (date parsing, authority files). Only `app` depends on it — it reaches `diagnostics` at runtime by registering into the validator registry, not through a `path` dependency | +| `checks` | date and authority validators, registered by `app` through the `diagnostics` crate | | `spellcheck` | dictionary catalogue behind `diagnostics::spelling` | +| `clustering` | pure value-pair matching, value-variant diagnostics, and cell fixes | | `plugin-host` | the Luau runtime that loads and runs plugins | | `plugin-api` | the types a plugin sees — see the three-repo rule below | | `preview` | turns a linked file into pixels — the format ladder and the thumbnail cache. Native decoders (PDF, video, RAW) belong here so they never reach `table` | diff --git a/Cargo.lock b/Cargo.lock index 81af60e2..7aa6cf5b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -260,6 +260,7 @@ dependencies = [ "anyhow", "async-channel", "checks", + "clustering", "data-exchange", "diagnostics", "dirs 6.0.0", @@ -980,6 +981,15 @@ dependencies = [ "wayland-client", ] +[[package]] +name = "caseless" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8b6fd507454086c8edfd769ca6ada439193cdb209c7681712ef6275cccbfe5d8" +dependencies = [ + "unicode-normalization", +] + [[package]] name = "cbc" version = "0.1.2" @@ -1064,6 +1074,7 @@ dependencies = [ "gpui-pre", "log", "reqwest", + "serde", "serde_json", "settings", ] @@ -1104,6 +1115,19 @@ dependencies = [ "libloading", ] +[[package]] +name = "clustering" +version = "0.4.0-beta.2" +dependencies = [ + "caseless", + "diagnostics", + "gpui-pre", + "log", + "settings", + "strsim", + "unicode-normalization", +] + [[package]] name = "cocoa" version = "0.25.0" @@ -3431,6 +3455,8 @@ version = "0.15.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9229cfe53dfd69f0609a49f65461bd93001ea1ef889cd5529dd176593f5338a1" dependencies = [ + "allocator-api2", + "equivalent", "foldhash 0.1.5", ] @@ -7527,6 +7553,7 @@ dependencies = [ "reqwest", "settings", "spellbook", + "whatlang", ] [[package]] @@ -7618,6 +7645,12 @@ dependencies = [ "quote", ] +[[package]] +name = "strsim" +version = "0.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" + [[package]] name = "strum" version = "0.28.0" @@ -8042,6 +8075,7 @@ version = "0.4.0-beta.2" dependencies = [ "ai", "base64", + "clustering", "diagnostics", "gpui-base", "gpui-component", @@ -8680,6 +8714,15 @@ version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3b09c83c3c29d37506a3e260c08c03743a6bb66a9cd432c6934ab501a190571f" +[[package]] +name = "unicode-normalization" +version = "0.1.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5fd4f6878c9cb28d874b009da9e8d183b5abc80117c40bbd187a1fde336be6e8" +dependencies = [ + "tinyvec", +] + [[package]] name = "unicode-properties" version = "0.1.4" @@ -9431,6 +9474,15 @@ dependencies = [ "web-sys", ] +[[package]] +name = "whatlang" +version = "0.18.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f5e8f38b596e2a359b755342473520a99421e43658548c79489ee221b728c107" +dependencies = [ + "hashbrown 0.15.5", +] + [[package]] name = "which" version = "8.0.5" diff --git a/Cargo.toml b/Cargo.toml index dd73ed48..4cae05cd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,6 +5,7 @@ members = [ "crates/app", "crates/ai", "crates/checks", + "crates/clustering", "crates/data-exchange", "crates/diagnostics", "crates/plugin-api", diff --git a/PLAN.md b/PLAN.md new file mode 100644 index 00000000..c90b5ffa --- /dev/null +++ b/PLAN.md @@ -0,0 +1,520 @@ +# Grouped diagnostics and reusable value clustering + +## Implementation status + +The seven implementation phases are complete. The full cluster-review workspace remains future work in issue #125. + +- The Problems panel groups explicit producer metadata and retains atomic diagnostics. +- Expansion supports mouse and keyboard input. Occurrence rows retain exact-location navigation. +- Column validators use `ColumnFinding`. Notes, plugin scripts, and bridge protocol 2 keep their existing storage and wire formats. +- Spelling emits one finding per distinct observed word per cell, rather than one combined sentence per cell. +- The new `clustering` crate separates pure matching from diagnostics and fixes. +- Value-variant fixes verify the expected cell text. Each validation run clears the previous candidate snapshot. +- Group menus can fix all spelling or capitalization occurrences in one undo step. +- Connected value-variant groups can use any displayed member or save a distinct-cluster decision. +- Documentation describes scopes, counts, grouping keys, and OpenRefine credit. + +Implementation details that refine the original proposal: + +- Tab counts mean diagnostic occurrences, not unique affected locations. One cell can contain two words or several cluster members. +- Spelling keys retain the observed token and dictionary code. This avoids combining distinct displayed spellings or correction targets. +- Cluster keys retain the sorted exact displayed forms and the column. Normalization generates links, not automatic edits. +- A `begin_run` validator hook clears candidate state for deleted columns and changed projects. +- The table crate needs adapter and diagnostic-literal updates. Its production edit, navigation, and undo paths do not change. + +Validation covers pure grouping, 100 repeated spelling findings, all scopes, source and severity filters, +expansion persistence, mouse navigation, keyboard expansion, stale fixes, Unicode matching, connected clusters, +the 2,000-value comparison bound, and a real table fix followed by undo and revalidation. +UI checks use GPUI test windows, not an interactive review of a user's project. + +Final local gate: `scripts/ci.sh` passed on Windows. Formatting and workspace Clippy passed. +The workspace suite passed 375 tests, with 10 existing network or external-plugin tests ignored. + +## Purpose + +This branch will make repeated findings easier to review. It will also move value clustering out +of the spell checker. + +A spelling mistake can occur in 100 cells. The Problems panel now shows 100 independent rows. The +panel should show one problem group with an occurrence count. The user can expand the group and +jump to each affected location. + +Value variants have the same need. Several linked forms of one name are one review question, +not unrelated notes or separate pair decisions. + +This branch keeps review inline. A separate future task can add custom canonical text, manual +membership editing, row previews, and reusable mappings. + +## Completed baseline + +The branch already contains these spell-check upgrades: + +- qrate loads all installed dictionaries. +- Each value selects a dictionary only when the evidence is strong enough. +- Ambiguous and short unknown values do not default to English. +- Capitalization findings come from the `spell` source, with the same fixes and ignores as misspellings. +- Candidate proper names do not become ordinary spelling warnings. +- The opt-in `value variants` validator finds formatting, token order, diacritic, and close-spelling + differences. +- Existing fixes replace one whole cell through the undoable table edit path. + +These behaviors stay in place during the diagnostic and crate changes. + +## Root cause + +The diagnostic model stores one `Diagnostic` for one exact `Location`. This is correct for cell +markers, navigation, notes, fixes, and source replacement. + +The display model uses the same shape. `ProblemsPanel` converts each `Diagnostic` directly into one +visible row. It has no stable key that says several diagnostics describe the same problem. + +The validator boundary also limits built-in synchronous validators. `ColumnValidator::validate` +returns `(row, severity, message)`. It can report a cell in the current column, but it cannot report +a column-wide finding. + +`Location` itself already supports all required scopes: + +- dataset: no row and no column; +- column: a column and no row; +- row: a row and no column; +- cell: both a row and a column. + +The branch does not need another location representation. It needs a clear scope view, a richer +validator result, and a separate grouped panel projection. + +## Design rules + +1. Keep each diagnostic attached to its exact location. +2. Let a producer define group identity. Do not parse a human message. +3. Group only diagnostics from the same dataset, source, severity, and producer key. +4. Keep diagnostics without group metadata as independent rows. +5. Keep group aggregation in the diagnostics crate. +6. Keep matching algorithms out of the diagnostics crate. +7. Never treat text similarity as proof of entity identity. +8. Do not add bulk replacement in the first grouped-panel change. +9. Preserve source replacement and validator severity overrides. +10. Preserve one-cell fixes on expanded occurrence rows. + +## Proposed diagnostic contracts + +### Location scope + +Add a derived scope method instead of replacing `Location`: + +```rust +pub enum Scope<'a> { + Dataset, + Column(&'a str), + Row(usize), + Cell { row: usize, column: &'a str }, +} + +impl Location { + pub fn scope(&self) -> Scope<'_>; +} +``` + +This method gives the panel one exhaustive scope match. It does not migrate stored notes or change +the existing table lookup keys. + +Add constructors for new call sites: + +```rust +Location::dataset(dataset) +Location::column(dataset, column) +Location::row(dataset, row, row_id) +Location::cell(dataset, row, row_id, column) +``` + +Existing struct literals can migrate when their files change. This branch does not need a +repository-wide mechanical rewrite. + +### Diagnostic group + +Add optional producer metadata to `Diagnostic`: + +```rust +pub struct DiagnosticGroup { + pub key: SharedString, + pub summary: SharedString, +} + +pub struct Diagnostic { + pub location: Location, + pub severity: Severity, + pub source: Source, + pub message: SharedString, + pub group: Option, + pub filed: Option, +} +``` + +`key` is machine-readable and stable within one source. `summary` is the text for the collapsed +group row. + +Examples: + +| Source | Group key | Summary | +|---|---|---| +| `spell` | `en:recieve` | `“recieve” is misspelled` | +| `capitalization` | `alice→Alice` | `Use “Alice” instead of “alice”` | +| `value variants` | sorted exact cluster members | `3 values may be variants: formatting differs, accents or diacritics differ` | + +The panel groups by `(dataset, source, severity, group.key)`. This prevents an override from hiding +the severity of some occurrences inside a different tab. + +Authored notes remain ungrouped. Existing validators remain ungrouped until they supply a group. + +### Validator finding + +Replace the tuple return value with a named type: + +```rust +pub struct ColumnFinding { + pub row: Option, + pub severity: Severity, + pub message: SharedString, + pub group: Option, +} +``` + +`row: Some(row)` reports a cell. `row: None` reports the current column. + +The named type makes later fields possible without another tuple migration. It also makes the scope +of each result visible at its construction site. + +The plugin script contract does not change in this branch. `plugin-host` will translate its current +row findings into ungrouped `ColumnFinding` values. + +Row-wide and dataset-wide producers continue to publish addressed `Diagnostic` values. A +column-wise validator must not claim a row-wide result because it only owns one column snapshot. + +## Grouped panel projection + +Keep `Diagnostics::items` as the source of truth. Build a derived panel model during +`ProblemsPanel::refresh`. + +```rust +enum ProblemEntry { + Group { + id: ProblemGroupId, + severity: Severity, + summary: SharedString, + source: SharedString, + occurrences: Vec, + }, + Occurrence(Occurrence), +} +``` + +The implementation can use a flat visible list after it applies expansion state. This keeps +`uniform_list` and its fixed row height. + +### Collapsed group row + +A collapsed row will show: + +- the severity icon; +- the group summary; +- the source; +- an `N occurrences` badge; +- an expand control. + +Clicking the expand control will show the occurrence rows. The group row will not choose an +arbitrary location or apply a fix. + +### Occurrence row + +An occurrence row will keep the current behavior: + +- show `Row N · Column`, row, column, or dataset scope; +- jump to the location on click; +- offer the current one-cell spelling and fix menus; +- read the current cell text when the menu opens. + +Column, row, and dataset occurrences have no cell text. Their context menus will omit cell fixes. + +### Counts and filters + +Severity tabs keep diagnostic occurrence totals. `Warnings (100)` means 100 warning findings, +which can include more than one distinct problem at the same location. + +The visible list will show fewer top-level rows after grouping. Each group badge will state its own +occurrence count. + +Source filtering runs before tab counts and grouping. Severity filtering runs before grouping. +Each severity tab keeps its own total regardless of the active tab. + +### Expansion state + +Store expanded `ProblemGroupId` values in `ProblemsPanel`. Clear keys that no longer exist after a +diagnostic refresh. + +Derive the ID from dataset, source, severity, and producer group key. Do not use the visible summary +or an item index. + +## Reusable clustering crate + +Create `crates/clustering`. + +The crate will own: + +- Unicode token normalization; +- strict, sorted, and diacritic-insensitive keys; +- candidate blocking; +- normalized Damerau-Levenshtein comparison; +- value frequencies and source rows; +- pair evidence and stable pair keys; +- connected review clusters and stable member-set keys; +- the `value variants` validator; +- its one-cell fix provider. + +The crate will depend on: + +- `caseless`; +- `unicode-normalization`; +- `strsim`; +- `diagnostics`; +- `settings`; +- `gpui` for validator registration state and fix lookup. + +The core matching module must stay free of GPUI types. The validator adapter can convert core +results into `SharedString`, diagnostics, and fixes. + +Move these dependencies out of `spellcheck`: + +- `caseless`; +- `unicode-normalization`; +- `strsim`. + +After the move, `spellcheck` will own dictionaries, language selection, spelling, capitalization, +and custom words. It will not own general value comparison. + +The app will register `clustering::ValueVariants` independently from the spell checker. The source +name remains `value variants` so existing severity settings continue to work. + +## Value-variant grouping + +The matcher returns evidence pairs. The review adapter treats each pair as a graph link and shows +each connected component as one cluster. + +For each cluster, the clustering crate returns: + +- every exact displayed value; +- the rows for each value; +- the matching reasons; +- a stable key from the sorted member set. + +All diagnostics for a cluster use the same `DiagnosticGroup`. The resolver offers every member as +a canonical choice and applies the result as one undoable edit. The user can instead mark all +current members as distinct. Similarity never changes data without that explicit choice. + +The future cluster-review workspace can add custom canonical text, member editing, row previews, +and reusable mappings. + +## Files and crates affected + +### `crates/diagnostics` + +- Add `DiagnosticGroup`, `ColumnFinding`, and the derived `Scope`. +- Update `address` and `ColumnValidator`. +- Add a pure panel projection that groups filtered diagnostics. +- Add expansion state and grouped rows to `ProblemsPanel`. +- Keep `Diagnostics::set`, `Diagnostics::at`, and the row index atomic. +- Test grouping, counts, filters, scopes, expansion, navigation, and fixes. + +### `crates/spellcheck` + +- Return `ColumnFinding` from spelling and capitalization validators. +- Add stable group keys for repeated spelling and capitalization findings. +- Remove value-variant code and general comparison dependencies. +- Keep dictionary behavior unchanged. + +### `crates/clustering` + +- Receive the current value-variant implementation and tests. +- Separate the pure matcher from the diagnostic adapter. +- Return pair evidence, connected clusters, frequencies, rows, and stable keys. +- Publish grouped value-variant findings. + +### `crates/checks` + +- Return `ColumnFinding` from date checks. +- Convert authority results to the named type. +- Leave findings ungrouped unless the rule has a clear stable subject. + +### `crates/plugin-host` + +- Adapt plugin validator output to `ColumnFinding`. +- Keep the public Luau result format unchanged. +- Leave plugin findings ungrouped in this phase. + +### `crates/app` + +- Register the value-variant validator and fixes from `clustering`. +- Remove the spell-check ownership link. + +### Workspace and documentation + +- Add `crates/clustering` to the workspace. +- Update `Cargo.lock`. +- Update `CLAUDE.md` crate map. +- Update `docs/diagnostics.md`. +- Add OpenRefine credit to the diagnostic documentation. + +### Existing integration points + +- `table` supplies one-cell fix hooks and exact location navigation. Its adapters need the new diagnostic types. +- `workspace` already hosts `ProblemsPanel`. +- `settings` already stores the opt-in value-variant setting. +- `plugin-api` does not expose grouped diagnostic metadata yet. + +## Implementation sequence + +### 1. Pin current behavior + +- Add tests for 100 identical spelling findings. +- Add tests for connected value variants that occur in many rows. +- Add tests for dataset, column, row, and cell labels. +- Confirm that fixes query the current cell value. + +### 2. Add the diagnostic contracts + +- Add `DiagnosticGroup`. +- Add `ColumnFinding`. +- Add `Location::scope` and constructors. +- Update built-in validator implementations. +- Keep plugin output compatible through its adapter. +- Confirm that source replacement still clears stale findings. + +### 3. Add the pure group projection + +- Group only diagnostics with explicit metadata. +- Keep ungrouped diagnostics independent. +- Calculate occurrence totals and top-level group totals. +- Filter before grouping. +- Sort groups by severity, summary, and stable key. +- Sort occurrences by table location. +- Test mixed scopes and mixed severity overrides. + +### 4. Render grouped diagnostics + +- Render collapsed groups with occurrence badges. +- Expand groups inline. +- Keep the list virtualized. +- Keep navigation and context menus on occurrence rows. +- Add keyboard and accessibility labels to expansion controls. +- Remove stale expansion keys after revalidation. + +### 5. Teach spelling to group + +- Group repeated misspellings by language and normalized token. +- Group capitalization findings by observed and canonical forms. +- Keep different replacement targets in different groups. +- Confirm that 100 repeated mistakes produce one collapsed row and 100 occurrences. + +### 6. Extract value clustering + +- Create the `clustering` crate. +- Move the matcher without changing thresholds. +- Split pure matching from the GPUI adapter. +- Publish one group for each connected value cluster. +- Offer every member as a canonical group fix. +- Run the existing 2,000-value blocking test in the new crate. + +### 7. Document and validate + +- Document all four diagnostic scopes. +- Document group identity and count semantics. +- Credit OpenRefine as a design influence. +- Run focused tests after each crate change. +- Run `./scripts/ci.sh` before the branch is ready for review. +- Test expansion, filtering, navigation, fixes, undo, and revalidation in the app. + +## OpenRefine credit and license boundary + +OpenRefine influenced the proposed review flow and the strict-to-broad clustering order: + +- OpenRefine, “Cluster and edit”: + +- OpenRefine, “Clustering Methods In-depth”: + +- OpenRefine source: + + +OpenRefine documentation uses CC BY 4.0. OpenRefine source code uses the BSD 3-Clause license. + +This branch will independently implement the algorithms from their published descriptions and +standard algorithm references. It will not copy OpenRefine source code, UI assets, or documentation +text. + +Add this acknowledgment to `docs/diagnostics.md`: + +> qrate's value-clustering workflow is inspired by OpenRefine's Cluster and edit feature. qrate +> uses an independent implementation designed for its diagnostics and cataloging workflow. + +Link both feature names to the OpenRefine documentation. Do not imply that OpenRefine endorses +qrate. + +This change credits design influence and uses an independent implementation. It does not distribute OpenRefine material. +If future work copies or adapts upstream material, review its actual license and add the required notices. + +If later work copies or adapts OpenRefine code: + +1. Record the exact upstream file and commit. +2. Keep the upstream copyright notice. +3. Add the BSD 3-Clause license text to `NOTICES`. +4. Mark the adapted qrate file. +5. Keep documentation excerpts under CC BY 4.0 with title, author, source, license, and change notes. + +## Future issue: cluster review workspace + +Track the full workspace in [GitHub issue #125](https://github.com/devnull03/qrate/issues/125). + +Track this outside the current implementation: + +- a dedicated cluster review workspace; +- value frequencies and affected-row counts; +- a custom canonical value; +- member inclusion and exclusion; +- sample row and thumbnail context; +- match evidence and strategy controls; +- skip and reject judgments; +- richer decision management and export; +- bulk replacement across a user-edited cluster; +- re-cluster after an accepted change; +- value-frequency facets; +- authority reconciliation candidates; +- mapping export and reuse. + +This issue should cite OpenRefine as a design influence. It should state that qrate will use an +independent implementation. + +## Non-goals + +- Do not build the full cluster-review workspace on this branch. +- Do not add phonetic, n-gram, PPM, or authority clustering on this branch. +- Do not merge similar values automatically. +- Do not group diagnostics by message text. +- Do not replace atomic diagnostics with aggregate records. +- Do not change stored note schema. +- Do not change the plugin API. + +## Definition of done + +- Repeated equivalent findings appear as one collapsed group. +- Each group shows its occurrence count. +- A user can expand a group and jump to every exact location. +- Existing one-cell fixes work from expanded occurrences. +- Group fixes use one table edit, one validation pass, and one undo step. +- A user can save all current cluster members as distinct for one column. +- Dataset, column, row, and cell scopes have clear labels. +- Source and severity filters produce correct groups and counts. +- Spelling and capitalization emit stable group metadata. +- Value clustering lives outside `spellcheck`. +- Value-variant clusters emit stable group metadata. +- Similarity never causes an automatic data change. +- Existing spell-check and value-variant behavior stays intact. +- Tests cover grouping, scopes, filters, fixes, revalidation, and comparison bounds. +- The documentation credits OpenRefine without claiming affiliation. +- The full workspace test and CI script pass. diff --git a/crates/app/Cargo.toml b/crates/app/Cargo.toml index 32603932..99a9d8d3 100644 --- a/crates/app/Cargo.toml +++ b/crates/app/Cargo.toml @@ -43,6 +43,7 @@ preview = { path = "../preview" } settings = { path = "../settings" } # The dictionary is half a megabyte, so only the composition root links it — see `crates/spellcheck`. spellcheck = { path = "../spellcheck" } +clustering = { path = "../clustering" } table = { path = "../table" } updater = { path = "../updater", features = ["client"] } plugin-api = { path = "../plugin-api" } diff --git a/crates/app/src/app_settings/config.rs b/crates/app/src/app_settings/config.rs index ecf94507..75dc1949 100644 --- a/crates/app/src/app_settings/config.rs +++ b/crates/app/src/app_settings/config.rs @@ -51,6 +51,7 @@ fn headers(cx: &App) -> Vec { "Description", "Authority", "Spellcheck", + "Variant Review", AUTHORITY_SEVERITY, ]; fixed @@ -103,6 +104,12 @@ fn rows(cx: &App) -> Vec { "no" } .to_string(), + if settings.is_some_and(|s| s.variant_review) { + "yes" + } else { + "no" + } + .to_string(), settings .and_then(|s| s.authority.as_ref().and_then(|a| s.severity.get(a))) .cloned() @@ -159,6 +166,7 @@ mod tests { "Description", "Authority", "Spellcheck", + "Variant Review", "Authority Severity", "islandora::Vocabularies", "islandora::Severity", @@ -173,6 +181,6 @@ mod tests { /// somebody hand-writes. #[gpui::test] fn without_plugins_only_the_fixed_columns_are_written(cx: &mut TestAppContext) { - cx.update(|cx| assert_eq!(super::headers(cx).len(), 6)); + cx.update(|cx| assert_eq!(super::headers(cx).len(), 7)); } } diff --git a/crates/app/src/app_settings/mod.rs b/crates/app/src/app_settings/mod.rs index 4b389aa5..c815af7c 100644 --- a/crates/app/src/app_settings/mod.rs +++ b/crates/app/src/app_settings/mod.rs @@ -3,11 +3,13 @@ mod config; use std::collections::{HashMap, HashSet}; use std::path::PathBuf; use std::rc::Rc; +use std::time::Duration; +use diagnostics::CheckList; use gpui::prelude::FluentBuilder as _; use gpui::{ AnyElement, App, AppContext as _, Axis, Entity, Global, IntoElement, ParentElement as _, - PromptLevel, SharedString, Styled as _, Subscription, Window, div, px, + PromptLevel, SharedString, Styled as _, Subscription, Task, Window, div, px, }; use gpui_component::{ ActiveTheme as _, Disableable as _, Icon, IconName, IndexPath, Sizable as _, @@ -15,7 +17,7 @@ use gpui_component::{ combobox::{Combobox, ComboboxEvent, ComboboxState}, h_flex, input::{Input, InputEvent, InputState}, - searchable_list::{SearchableListItem, SearchableVec}, + searchable_list::SearchableListItem, setting::{SettingField, SettingGroup, SettingItem, SettingPage}, switch::Switch, tab::{Tab, TabBar}, @@ -31,6 +33,20 @@ use settings::{Setting, columns, project::CurrentProject}; pub const COLUMNS_PAGE: usize = 2; pub const PLUGINS_PAGE: usize = 7; +struct AuthorityRevalidation(#[allow(dead_code)] Task<()>); + +impl Global for AuthorityRevalidation {} + +fn revalidate_authority_setting(cx: &mut App) { + let task = cx.spawn(async move |cx| { + cx.background_executor() + .timer(Duration::from_millis(500)) + .await; + cx.update(table::revalidate_now); + }); + cx.set_global(AuthorityRevalidation(task)); +} + pub fn build_pages(cx: &App) -> Vec { let mut pages = vec![ SettingPage::new("Application") @@ -748,8 +764,7 @@ fn saving_group(cx: &App) -> SettingGroup { /// The spell-check master switch and, when on, the dictionary language. Both read through their /// own closures rather than `Setting::Switch`, because an unset value has to mean *on* — see /// `SPELLCHECK_ENABLED_KEY`. Which columns are checked lives on the Columns page instead, next to -/// the other per-column knobs. Changing either takes effect on the next launch, since the -/// dictionary is parsed once at startup. +/// the other per-column knobs. Changing any of them reloads the checker. fn spelling_group(cx: &App) -> SettingGroup { let mut group = divided_group(cx).title("Spelling").item( SettingItem::new( @@ -758,6 +773,7 @@ fn spelling_group(cx: &App) -> SettingGroup { |cx: &App| spellcheck::enabled(cx), |on: bool, cx: &mut App| { settings::set_scoped_bool(spellcheck::SPELLCHECK_ENABLED_KEY, on, cx); + crate::register_spell_checker(cx); }, ), ) @@ -772,13 +788,14 @@ fn spelling_group(cx: &App) -> SettingGroup { |cx: &App| spellcheck::ignore_capitalized(cx), |on: bool, cx: &mut App| { settings::set_scoped_bool(spellcheck::SPELLCHECK_NAMES_KEY, on, cx); + crate::register_spell_checker(cx); }, ), ) .description( "Skip capitalized words. No dictionary holds every person, place, or studio, so \ without this a catalogue of names is mostly underlines. Turn it off to catch \ - typos that begin with a capital. Takes effect on restart.", + typos that begin with a capital.", ), ); group = group.item( @@ -789,8 +806,9 @@ fn spelling_group(cx: &App) -> SettingGroup { }), ) .description( - "Canadian and American English are built in. Any other language downloads on \ - first use and is kept beside your logs. Takes effect on restart.", + "Each value is checked in whichever ticked language fits it. Canadian and \ + American English are built in; ticking another language downloads it. The first \ + one ticked sets the preferred regional spelling.", ), ); } @@ -829,7 +847,7 @@ struct Picker where I::Value: PartialEq + Clone, { - state: Entity>>, + state: Entity>>, /// What the list was last built from. `set_items` replaces the delegate wholesale, which /// throws away the active search filter — so it only runs when this actually changed. items: Vec, @@ -1082,7 +1100,7 @@ fn language_rows(cx: &App) -> Vec { /// which only knows the search-filtered view. The guard keeps this a no-op during interaction, so /// the two agree in practice. fn sync_selection + 'static>( - state: &Entity>>, + state: &Entity>>, items: &[I], want: &[SharedString], window: &mut Window, @@ -1129,7 +1147,7 @@ fn picker( on_change: impl Fn(&[SharedString], &mut App) + 'static, window: &mut Window, cx: &mut App, -) -> Entity>> +) -> Entity>> where I: SearchableListItem + PartialEq + Clone + 'static, { @@ -1143,7 +1161,7 @@ where Some(state) => state, None => { let state = cx.new(|cx| { - ComboboxState::new(SearchableVec::new(items.clone()), vec![], window, cx) + ComboboxState::new(CheckList::new(items.clone()), vec![], window, cx) .multiple(kind.multiple) .searchable(kind.searchable) }); @@ -1167,7 +1185,7 @@ where // A refresh replaces the list; rebuilding on every render would throw away the search filter. if slot(cx.global_mut::())[&id].items != items { state.update(cx, |state, cx| { - state.set_items(SearchableVec::new(items.clone()), window, cx); + state.set_items(CheckList::new(items.clone()), window, cx); }); if let Some(picker) = slot(cx.global_mut::()).get_mut(&id) { picker.items = items; @@ -1179,9 +1197,9 @@ where /// The language list, in the shape a phone's language screen uses: everything available, each row /// saying whether it is already here or a download away, and one tap doing whichever applies. /// -/// Picking a language that isn't here yet starts its download instead of switching to it. Nothing -/// undoes the highlight by hand — the selection re-syncs from the setting on the next render, and -/// the setting did not change. +/// Ticking a language that isn't here yet starts its download, and it joins the checked languages +/// once it lands. Nothing undoes the tick by hand — the selection re-syncs from the setting on the +/// next render. The first language picked is the preferred regional spelling. fn language_picker(window: &mut Window, cx: &mut App) -> AnyElement { use spellcheck::catalogue::State; @@ -1193,35 +1211,60 @@ fn language_picker(window: &mut Window, cx: &mut App) -> AnyElement { LANGUAGE.to_string(), rows.clone(), PickerKind { - multiple: false, + multiple: true, searchable: true, }, |values: &[SharedString], cx: &mut App| { - let Some(code) = values.first().cloned() else { - return; + spellcheck::retain_wanted_downloads(values, cx); + let listing = spellcheck::catalogue::listing(); + let state_of = |code: &str| { + listing + .iter() + .find(|((c, _, _), _)| *c == code) + .map(|(_, state)| *state) }; - match spellcheck::catalogue::listing() + let mut codes: Vec = spellcheck::languages(cx) .into_iter() - .find(|((c, _, _), _)| *c == code.as_ref()) - .map(|(_, state)| state) - { - Some(State::Available) => spellcheck::start_download(code, cx), - // Built in and installed both mean "here already"; only the reason differs, and - // the reason is not the user's problem. - Some(_) => settings::set_scoped_text(spellcheck::SPELLCHECK_LANGUAGE_KEY, code, cx), - None => {} + .filter(|code| values.iter().any(|v| v.as_ref() == code)) + .collect(); + for value in values { + match state_of(value) { + Some(State::Available) => { + spellcheck::start_download(value.clone(), crate::register_spell_checker, cx) + } + Some(_) if !codes.iter().any(|c| c == value.as_ref()) => { + codes.push(value.to_string()) + } + _ => {} + } + } + if codes != spellcheck::languages(cx) { + spellcheck::set_languages(&codes, cx); + crate::register_spell_checker(cx); } }, window, cx, ); - sync_selection(&state, &rows, &[spellcheck::language(cx)], window, cx); + let chosen: Vec = spellcheck::languages(cx) + .into_iter() + .map(SharedString::from) + .collect(); + sync_selection(&state, &rows, &chosen, window, cx); + let label = picker_label( + &chosen + .iter() + .filter_map(|code| rows.iter().find(|row| &row.code == code)) + .map(|row| row.name.clone()) + .collect::>(), + ); Combobox::new(&state) .small() .menu_width(px(320.)) .menu_max_h(px(360.)) .search_placeholder("Search languages…") + .render_trigger(move |_ctx, _, _| div().child(label.clone())) .into_any_element() } @@ -1242,6 +1285,17 @@ fn columns_page(cx: &App) -> SettingPage { let headers = column_items(project); + let values = divided_group(cx).title("Multi-value cells").item( + Setting::Text { + key: settings::FILTER_SUBDELIMITER_KEY, + label: "Value separator", + description: "Separate multiple values in one cell, e.g. \"|\" for \"Film|Video\". \ + Filters and validators treat each part as one logical value. Leave empty \ + to treat the complete cell as one value.", + } + .into_item(cx), + ); + let mut group = divided_group(cx).title("Filters").item( SettingItem::new( "Enable column filters", @@ -1254,16 +1308,6 @@ fn columns_page(cx: &App) -> SettingPage { ); if columns::filters_master_enabled(cx) { - group = group.item( - Setting::Text { - key: settings::FILTER_SUBDELIMITER_KEY, - label: "Sub-delimiter", - description: "Split cells that hold several values, e.g. \";\" for \"Film; Video\", \ - so the dropdown lists each value on its own. Leave empty to filter \ - whole cells.", - } - .into_item(cx), - ); let picked = headers.clone(); group = group.item( SettingItem::new( @@ -1305,9 +1349,170 @@ fn columns_page(cx: &App) -> SettingPage { .description("Columns whose text is spell-checked. New columns start checked."), ); + let variant_columns = headers.clone(); + let variants = divided_group(cx).title("Value variants").item( + SettingItem::new( + "Reviewed columns", + SettingField::element(move |_opts: &_, window: &mut _, cx: &mut _| { + column_picker( + "variant-reviewed-columns", + variant_columns.clone(), + Rc::new(|c: &ColumnItem, cx: &App| columns::get(&c.key, cx).variant_review), + Rc::new(|c: &ColumnItem, on: bool, cx: &mut App| { + columns::update(&c.key, |s| s.variant_review = on, cx); + }), + window, + cx, + ) + }), + ) + .description( + "Columns checked for inconsistent displayed forms. Similar values are suggestions for \ + review, not automatic merges.", + ), + ); + + let ignored = divided_group(cx) + .title("Ignored problems") + .description("Findings you chose to ignore. Remove one to see it in Problems again.") + .item( + SettingItem::new( + "Ignored", + SettingField::element(move |_opts: &_, _window: &mut _, cx: &mut App| { + // Only ignores that still hide something are listed; Clear all removes the rest. + let hidden = diagnostics::Diagnostics::ignored(cx); + let mut entries: Vec<( + SharedString, + String, + String, + String, + Option, + )> = Vec::new(); + for (column, settings) in columns::load(cx) { + let matching = |source: &String, key: &String| { + hidden + .iter() + .filter(|d| { + d.location.column.as_deref() == Some(column.as_str()) + && d.source.key() == *source + && d.ignore_key() == *key + }) + .collect::>() + }; + for (source, key) in &settings.ignored_diagnostics { + let hits = matching(source, key); + let Some(first) = hits.first() else { + continue; + }; + let summary = first + .group + .as_ref() + .map_or(&first.message, |group| &group.summary); + let label = format!( + "{column} · {} · {summary} ({})", + first.source.label(), + hits.len() + ); + entries.push(( + label.into(), + column.clone(), + source.clone(), + key.clone(), + None, + )); + } + for (source, key, id) in &settings.ignored_occurrences { + let Some(first) = matching(source, key) + .into_iter() + .find(|d| d.location.row_id == Some(*id)) + else { + continue; + }; + let label = format!( + "{column} · Row {} · {}", + first.location.row.map_or(0, |row| row + 1), + first.message + ); + entries.push(( + label.into(), + column.clone(), + source.clone(), + key.clone(), + Some(*id), + )); + } + } + let muted = cx.theme().muted_foreground; + v_flex() + .w_full() + .gap_1() + .when(entries.is_empty(), |list| { + list.child(div().text_sm().text_color(muted).child("Nothing ignored")) + }) + .children(entries.into_iter().enumerate().map( + |(ix, (label, column, source, key, row_id))| { + h_flex() + .w_full() + .gap_2() + .items_center() + .child( + div() + .flex_1() + .min_w_0() + .overflow_hidden() + .text_ellipsis() + .text_sm() + .child(label), + ) + .child( + Button::new(("unignore", ix)) + .xsmall() + .icon(IconName::Close) + .tooltip("Stop ignoring") + .on_click(move |_, _, cx| { + let entry = (source.clone(), key.clone()); + columns::update( + &column, + |settings| match row_id { + Some(id) => { + settings + .ignored_occurrences + .remove(&(entry.0, entry.1, id)); + } + None => { + settings + .ignored_diagnostics + .remove(&entry); + } + }, + cx, + ); + table::revalidate_now(cx); + }), + ) + }, + )) + .child( + Button::new("clear-ignored-diagnostics") + .small() + .label("Clear all") + .on_click(move |_, _, cx| { + columns::clear_ignored(cx); + table::revalidate_now(cx); + }), + ) + .into_any_element() + }), + ) + .layout(Axis::Vertical), + ); + SettingPage::new("Columns") + .group(values) .group(group) .group(spelling) + .group(variants) + .group(ignored) .group(descriptions_group(project, &headers, cx)) .group(data_types_group(headers, cx)) .group(authority_accounts_group(cx)) @@ -1537,7 +1742,7 @@ fn config_file_group(cx: &App) -> SettingGroup { fn authority_accounts_group(cx: &App) -> SettingGroup { let described = checks::authorities() .iter() - .map(|(name, describes)| format!("**{name}** — {describes}")) + .map(|(name, describes)| format!("{name} — {describes}")) .collect::>() .join("\n\n"); @@ -1545,12 +1750,13 @@ fn authority_accounts_group(cx: &App) -> SettingGroup { .title("Authority accounts") .description(SharedString::from(described)) .item( - Setting::Text { + Setting::TextWithAction { key: checks::GEONAMES_USERNAME_KEY, label: "GeoNames account", description: "GeoNames refuses anonymous requests, so its check stays off \ until this is filled in. A free account at geonames.org is \ enough — this is a username, not a password.", + on_change: revalidate_authority_setting, } .into_item(cx), ) @@ -1739,6 +1945,7 @@ fn column_picker( { let on = on.clone(); move |values: &[SharedString], cx: &mut App| { + let started = std::time::Instant::now(); // Re-read the columns rather than capturing them: this closure outlives the // project whose headers built it. let Some(current) = cx.try_global::().map(column_items) else { @@ -1753,6 +1960,7 @@ fn column_picker( // A validator reads these settings, so its published findings are stale the // moment one changes — and only a run replaces them. table::revalidate_now(cx); + log::debug!("column picker {id:?} applied in {:?}", started.elapsed()); } }, window, diff --git a/crates/app/src/main.rs b/crates/app/src/main.rs index c0c95d5a..baa56b2b 100644 --- a/crates/app/src/main.rs +++ b/crates/app/src/main.rs @@ -107,6 +107,7 @@ fn set_main_window_title(window: &mut Window, cx: &gpui::App) { } pub(crate) fn open_main_window(cx: &mut gpui::App) { + let started = std::time::Instant::now(); // Window already exists (a project switch); reload its layout instead of keeping the old one's. if let Some(handle) = WindowRegistry::focus_or_clear(MAIN_WINDOW_KIND, cx) { handle @@ -146,6 +147,7 @@ pub(crate) fn open_main_window(cx: &mut gpui::App) { cx.new(|cx| Root::new(view, window, cx)) }) { WindowRegistry::register(MAIN_WINDOW_KIND, window_handle.into(), cx); + log::info!("opened the main window in {:?}", started.elapsed()); } } @@ -261,6 +263,7 @@ impl App { impl Render for App { fn render(&mut self, window: &mut Window, cx: &mut Context) -> impl IntoElement { let dialog_layer = Root::render_dialog_layer(window, cx); + let notification_layer = Root::render_notification_layer(window, cx); div() .size_full() @@ -320,30 +323,57 @@ impl Render for App { .child(self.status_bar.clone()), ) .children(dialog_layer) + .children(notification_layer) } } -/// Register the spell checker, once its dictionary has parsed off the UI thread — half a -/// megabyte of affix rules is not something to put in front of first paint. +/// Which spell-checker load is current, so a slow earlier load cannot land over a newer choice. +#[derive(Default)] +struct SpellLoad(u64); + +impl gpui::Global for SpellLoad {} + +/// (Re)register the spell checker, once its dictionaries have parsed off the UI thread — half a +/// megabyte of affix rules is not something to put in front of first paint. Called again whenever a +/// spelling setting changes, so the new choice applies without a restart. /// /// The only place that links both the checker and the table, which is what lets `spellcheck` know /// nothing about grids and `table` nothing about dictionaries. -fn register_spell_checker(cx: &mut gpui::App) { +pub(crate) fn register_spell_checker(cx: &mut gpui::App) { + let generation = { + let load = cx.default_global::(); + load.0 += 1; + load.0 + }; if !spellcheck::enabled(cx) { log::info!("spell checking is turned off in settings"); + diagnostics::Validators::remove(&spellcheck::SPELLING_VALIDATOR_NAME.into(), cx); return; } - let language = spellcheck::language(cx); + let languages = spellcheck::languages(cx); let ignore_capitalized = spellcheck::ignore_capitalized(cx); - let load = cx - .background_executor() - .spawn(async move { spellcheck::SpellCheck::load(&language, ignore_capitalized) }); + let load = cx.background_executor().spawn(async move { + let started = std::time::Instant::now(); + let spell = spellcheck::SpellCheck::load(&languages, ignore_capitalized); + log::info!("loaded dictionaries in {:?}", started.elapsed()); + spell + }); cx.spawn(async move |cx| { - let Some(spell) = load.await else { - return; - }; + let spell = load.await; cx.update(|cx| { + if cx.global::().0 != generation { + return; + } + diagnostics::Validators::remove(&spellcheck::SPELLING_VALIDATOR_NAME.into(), cx); + let Some(spell) = spell else { + return; + }; diagnostics::Validators::register(Box::new(spell.clone()), cx); + diagnostics::GroupFixProviders::register( + spellcheck::SPELLING_VALIDATOR_NAME, + spellcheck::spelling_group_fixes, + cx, + ); cx.set_global(spell); cx.set_global(diagnostics::SpellActions { suggest: spellcheck::misspellings, @@ -360,6 +390,23 @@ fn register_spell_checker(cx: &mut gpui::App) { .detach(); } +fn register_variant_checker(cx: &mut gpui::App) { + let variants = clustering::ValueVariants::default(); + diagnostics::Validators::register(Box::new(variants.clone()), cx); + diagnostics::FixProviders::register( + clustering::VALUE_VARIANTS_NAME, + clustering::variant_fixes, + cx, + ); + diagnostics::GroupFixProviders::register( + clustering::VALUE_VARIANTS_NAME, + clustering::variant_group_fixes, + cx, + ); + cx.set_global(variants); + table::revalidate_now(cx); +} + /// Select and scroll to whatever a problem points at. Three index spaces meet here: the /// diagnostic's source row and column *name*, the delegate's filtered view row, and the table's /// display column (data col + 1, past the pinned `#`). @@ -489,6 +536,7 @@ fn main() { }); app.run(move |cx| { + let started = std::time::Instant::now(); gpui_component::init(cx); cx.register_url_scheme("qrate").detach(); @@ -518,13 +566,21 @@ fn main() { reveal: reveal_in_table, text_at: table::cell_text, set_text: table::set_cell_text, + set_texts: table::set_cell_texts, + revalidate: table::revalidate_now, }); diagnostics::init(cx); + log::debug!( + "startup: settings and theme ready at {:?}", + started.elapsed() + ); // The composition root is the only place that knows where validators come from; `table` // triggers the runs without naming any of them. plugin_host::on_command_finished(table::revalidate_now); plugin_host::reload(cx); + log::debug!("startup: plugins loaded at {:?}", started.elapsed()); + register_variant_checker(cx); register_spell_checker(cx); update_check::init(cx); diagnostics::AsyncValidators::register( @@ -535,6 +591,10 @@ fn main() { checks::init(cx); agent_bridge::init(cx); agent_runtime::init(cx); + log::debug!( + "startup: validators and bridges registered at {:?}", + started.elapsed() + ); cx.on_action(|_: &ReloadPlugins, cx| { plugin_host::reload(cx); diff --git a/crates/checks/Cargo.toml b/crates/checks/Cargo.toml index 2d5d6f6b..169978f4 100644 --- a/crates/checks/Cargo.toml +++ b/crates/checks/Cargo.toml @@ -8,6 +8,7 @@ diagnostics = { path = "../diagnostics" } settings = { path = "../settings" } gpui.workspace = true log.workspace = true +serde.workspace = true serde_json.workspace = true # Blocking like `spellcheck`'s download and `plugin-host`'s http.get, and for the same reason: # gpui's executor is not a tokio reactor, so these run on the background executor. @@ -16,3 +17,6 @@ reqwest = { version = "0.12", features = ["blocking"] } # compatible with this repo's AGPL-3.0. Hand-parsing that grammar is how you get a validator # that is subtly wrong about seasons, intervals, and unspecified digits. edtf = "0.2" + +[dev-dependencies] +gpui = { workspace = true, features = ["test-support"] } diff --git a/crates/checks/src/authority/geonames.rs b/crates/checks/src/authority/geonames.rs index 45f5f1db..5fafb418 100644 --- a/crates/checks/src/authority/geonames.rs +++ b/crates/checks/src/authority/geonames.rs @@ -43,19 +43,25 @@ impl AuthoritySource for GeoNames { ) } + fn cache_scope(&self) -> String { + format!("{NAME}:{}", self.username) + } + + fn refusal(&self, body: &str) -> Option { + serde_json::from_str::(body) + .ok()? + .get("status")? + .get("message")? + .as_str() + .map(str::to_owned) + } + fn labels(&self, body: &str) -> Option> { let parsed = serde_json::from_str::(body).ok()?; // Every refusal — bad username, spent daily credits, rate limit — comes back as a `status` // object with no `geonames` array. Reading that as "no such place" would redden a whole // column over an account problem, so it is not an answer. - let Some(hits) = parsed["geonames"].as_array() else { - if let Some(message) = parsed["status"]["message"].as_str() { - log::warn!( - "GeoNames refused the lookup, so places are not being checked: {message}" - ); - } - return None; - }; + let hits = parsed["geonames"].as_array()?; Some( hits.iter() .filter_map(|hit| hit["name"].as_str().map(str::to_owned)) @@ -118,6 +124,16 @@ mod tests { assert!(configured().unavailable().is_none()); } + #[test] + fn the_cache_scope_changes_with_the_account() { + let first = configured().cache_scope(); + let second = GeoNames { + username: "somebody-else".into(), + } + .cache_scope(); + assert_ne!(first, second); + } + #[test] fn the_username_reaches_the_url_escaped() { let odd = GeoNames { diff --git a/crates/checks/src/authority/mod.rs b/crates/checks/src/authority/mod.rs index 43ed89b5..d16afce3 100644 --- a/crates/checks/src/authority/mod.rs +++ b/crates/checks/src/authority/mod.rs @@ -39,7 +39,8 @@ use std::sync::Mutex; use std::time::{Duration, Instant}; use diagnostics::{ - ColumnSnapshot, DATASET_MAIN, Diagnostics, Fix, FixProviders, Location, Severity, Source, + ColumnFinding, ColumnSnapshot, DATASET_MAIN, DiagnosticGroup, DiagnosticHooks, Diagnostics, + Fix, FixProviders, GroupFix, Location, Severity, Source, SourceActions, }; use gpui::{App, AppContext as _, Global, SharedString, Task}; @@ -62,8 +63,11 @@ const WINDOW: Duration = Duration::from_secs(60); /// Per request. A server that has not answered in this long is not going to. const TIMEOUT: Duration = Duration::from_secs(10); +/// How long a term whose request failed waits before it is asked again. +const RETRY_AFTER: Duration = Duration::from_secs(300); + /// What an authority said about one term. -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub struct Verdict { /// Whether the authority holds this exact heading. pub known: bool, @@ -78,6 +82,12 @@ pub struct Verdict { #[derive(Default)] struct Cache { terms: HashMap>, + scopes: HashMap, + failures: HashMap, + /// Terms a request is out for, per source, so an overlapping run never asks for them twice. + pending: HashMap>, + /// Terms whose request failed, per source, with when they may be asked again. + unanswered: HashMap>, loaded: bool, /// Keeps the in-flight run alive; dropping it cancels, which is what a newer run wants. run: Option>, @@ -85,6 +95,12 @@ struct Cache { impl Global for Cache {} +#[derive(serde::Serialize, serde::Deserialize)] +struct StoredCache { + scope: String, + terms: HashMap, +} + fn cache_path(source: &str) -> Option { settings::data_dir().map(|dir| dir.join("authority").join(format!("{source}.json"))) } @@ -96,6 +112,7 @@ fn ensure_loaded(cx: &mut App) { return; } let mut terms = HashMap::new(); + let mut scopes = HashMap::new(); for name in source::NAMES { let Some(path) = cache_path(name) else { continue; @@ -103,32 +120,38 @@ fn ensure_loaded(cx: &mut App) { let Ok(text) = std::fs::read_to_string(&path) else { continue; }; - match serde_json::from_str::)>>(&text) { - Ok(stored) => { - terms.insert( - name.to_string(), - stored - .into_iter() - .map(|(term, (known, suggestions))| (term, Verdict { known, suggestions })) - .collect(), - ); - } - Err(err) => log::warn!("discarding the unreadable {name} cache: {err}"), + if let Ok(stored) = serde_json::from_str::(&text) { + scopes.insert(name.to_string(), stored.scope); + terms.insert(name.to_string(), stored.terms); + } else if let Ok(stored) = + serde_json::from_str::)>>(&text) + { + scopes.insert(name.to_string(), name.to_string()); + terms.insert( + name.to_string(), + stored + .into_iter() + .map(|(term, (known, suggestions))| (term, Verdict { known, suggestions })) + .collect(), + ); + } else { + log::warn!("discarding the unreadable {name} cache"); } } let cache = cx.default_global::(); cache.terms = terms; + cache.scopes = scopes; cache.loaded = true; } -fn write_cache(source: &str, verdicts: &HashMap) { +fn write_cache(source: &str, scope: &str, verdicts: &HashMap) { let Some(path) = cache_path(source) else { return; }; - let stored: HashMap<&String, (bool, &Vec)> = verdicts - .iter() - .map(|(term, v)| (term, (v.known, &v.suggestions))) - .collect(); + let stored = StoredCache { + scope: scope.to_owned(), + terms: verdicts.clone(), + }; let written = path .parent() .map_or(Ok(()), std::fs::create_dir_all) @@ -145,9 +168,8 @@ fn write_cache(source: &str, verdicts: &HashMap) { /// One source's share of a run. struct Job { source: Box, + scope: String, columns: Vec, - /// Terms nothing is cached for, already capped at [`PER_RUN`]. - ask: Vec, } /// Split a cell into the values it actually holds. Archival subject and name columns are commonly @@ -208,79 +230,132 @@ pub fn check(columns: &[ColumnSnapshot], cx: &mut App) { continue; } - let cached = cx - .default_global::() - .terms - .entry(name.into()) - .or_default(); - let mut seen = HashSet::new(); - let ask: Vec = mine - .iter() - .flat_map(|c| c.values.iter()) - .flat_map(|cell| values_in(cell, &subdelimiter)) - .filter(|value| seen.insert(value.to_lowercase())) - .filter(|value| !cached.contains_key(&value.to_lowercase())) - .take(PER_RUN) - .collect(); - + let scope = source.cache_scope(); + let cache = cx.default_global::(); + if cache.scopes.get(name) != Some(&scope) { + cache.scopes.insert(name.into(), scope.clone()); + cache.terms.remove(name); + cache.failures.remove(name); + cache.unanswered.remove(name); + } jobs.push(Job { source, + scope, columns: mine, - ask, }); } if jobs.is_empty() { return; } + // Everything already cached is published before any request goes out, so a reopened project + // shows what it knew immediately instead of after a round trip. + publish(&jobs, &subdelimiter, cx); let task = cx.spawn(async move |cx| { - // Everything already cached is published before the first request goes out, so a reopened - // project shows what it knew immediately instead of after a round trip. - cx.update(|cx| publish(&jobs, &subdelimiter, cx)); - - let asked: Vec<(String, Vec)> = jobs - .iter() - .filter(|job| !job.ask.is_empty()) - .map(|job| (job.source.name().to_string(), job.ask.clone())) - .collect(); + cx.background_executor().timer(DEBOUNCE).await; + let asked = cx.update(|cx| claim(&jobs, &subdelimiter, cx)); if asked.is_empty() { return; } + // Detached: a newer run cancels the debounce, never answers a request already paid for. + cx.spawn(async move |cx| { + let fetched = cx + .background_spawn(async move { + let sources = source::all(&config); + asked + .into_iter() + .filter_map(|(name, scope, terms)| { + let source = sources.iter().find(|s| s.name() == name)?; + let result = lookup_all(source.as_ref(), &terms); + Some((name, scope, terms, result)) + }) + .collect::>() + }) + .await; + cx.update(|cx| store(fetched, cx)); + }) + .detach(); + }); + cx.default_global::().run = Some(task); +} - cx.background_executor().timer(DEBOUNCE).await; - let fetched = cx - .background_spawn(async move { - let sources = source::all(&config); - asked - .into_iter() - .filter_map(|(name, terms)| { - let source = sources.iter().find(|s| s.name() == name)?; - Some((name.clone(), lookup_all(source.as_ref(), &terms))) - }) - .collect::>() - }) - .await; +/// Each source's next [`PER_RUN`] distinct terms that nothing is cached, pending, or waiting to +/// retry for, marked pending as they are handed out. +fn claim(jobs: &[Job], subdelimiter: &str, cx: &mut App) -> Vec<(String, String, Vec)> { + let cache = cx.default_global::(); + let now = Instant::now(); + jobs.iter() + .filter_map(|job| { + let name = job.source.name(); + if cache.scopes.get(name) != Some(&job.scope) || cache.failures.contains_key(name) { + return None; + } + let cached = cache.terms.get(name); + let unanswered = cache.unanswered.get(name); + let pending = cache.pending.entry(name.into()).or_default(); + let mut seen = HashSet::new(); + let ask: Vec = job + .columns + .iter() + .flat_map(|c| c.values.iter()) + .flat_map(|cell| values_in(cell, subdelimiter)) + .filter(|value| { + let key = value.to_lowercase(); + !cached.is_some_and(|cached| cached.contains_key(&key)) + && unanswered + .and_then(|unanswered| unanswered.get(&key)) + .is_none_or(|retry_at| now >= *retry_at) + && !pending.contains(&key) + && seen.insert(key) + }) + .take(PER_RUN) + .collect(); + pending.extend(ask.iter().map(|term| term.to_lowercase())); + (!ask.is_empty()).then(|| (name.to_owned(), job.scope.clone(), ask)) + }) + .collect() +} - cx.update(|cx| { - for (name, verdicts) in fetched { - if verdicts.is_empty() { - continue; - } - let cache = cx.default_global::(); - let for_source = cache.terms.entry(name.clone()).or_default(); - for (term, verdict) in verdicts { - for_source.insert(term, verdict); - } - let snapshot = for_source.clone(); - cx.background_executor() - .spawn(async move { write_cache(&name, &snapshot) }) - .detach(); +/// Cache what came back, then revalidate so the answers publish against the sheet as it is now — +/// which also claims the next batch, until every term has an answer. +fn store(fetched: Vec<(String, String, Vec, LookupBatch)>, cx: &mut App) { + let mut answered = false; + for (name, scope, asked, result) in fetched { + let cache = cx.default_global::(); + if let Some(pending) = cache.pending.get_mut(&name) { + for term in &asked { + pending.remove(&term.to_lowercase()); } - publish(&jobs, &subdelimiter, cx); - }); - }); - cx.default_global::().run = Some(task); + } + if cache.scopes.get(&name) != Some(&scope) { + continue; + } + let retry_at = Instant::now() + RETRY_AFTER; + cache + .unanswered + .entry(name.clone()) + .or_default() + .extend(result.unanswered.into_iter().map(|term| (term, retry_at))); + if let Some(failure) = result.failure { + log::warn!("{name} checks paused: {failure}"); + cache.failures.insert(name.clone(), failure); + answered = true; + } + if result.verdicts.is_empty() { + continue; + } + answered = true; + let for_source = cache.terms.entry(name.clone()).or_default(); + for_source.extend(result.verdicts); + let snapshot = for_source.clone(); + cx.background_executor() + .spawn(async move { write_cache(&name, &scope, &snapshot) }) + .detach(); + } + if answered && let Some(hooks) = cx.try_global::().copied() { + (hooks.revalidate)(cx); + } } /// Turn what the cache knows into findings, for every job. A term nothing is cached for is not @@ -289,42 +364,58 @@ fn publish(jobs: &[Job], subdelimiter: &str, cx: &mut App) { for job in jobs { let name = job.source.name(); let empty = HashMap::new(); - let cached = cx - .try_global::() - .and_then(|c| c.terms.get(name)) + let cache = cx.try_global::(); + let cached = cache + .and_then(|cache| cache.terms.get(name)) .unwrap_or(&empty); + let failure = cache.and_then(|cache| cache.failures.get(name)); let items: Vec<_> = job .columns .iter() .flat_map(|column| { - let found: Vec<(usize, Severity, SharedString)> = column + let mut found: Vec<_> = column .values .iter() .enumerate() - .filter_map(|(row, cell)| { - let rejected: Vec = values_in(cell, subdelimiter) + .flat_map(|(row, cell)| { + let mut seen = HashSet::new(); + values_in(cell, subdelimiter) .into_iter() - .filter(|value| { - cached - .get(&value.to_lowercase()) - .is_some_and(|verdict| !verdict.known) + .filter_map(move |value| { + let key = value.to_lowercase(); + if !seen.insert(key.clone()) { + return None; + } + cached.get(&key).filter(|verdict| !verdict.known)?; + let message: SharedString = job.source.rejection(&value).into(); + Some(ColumnFinding { + row: Some(row), + severity: Severity::Error, + message: message.clone(), + group: Some(DiagnosticGroup { + key: key.into(), + summary: message, + subject: Some(value.into()), + }), + }) }) - .collect(); - (!rejected.is_empty()).then(|| { - ( - row, - Severity::Error, - rejected - .iter() - .map(|value| job.source.rejection(value)) - .collect::>() - .join("; ") - .into(), - ) - }) }) .collect(); + if let Some(failure) = failure { + let message: SharedString = + format!("{name} could not check this column: {failure}").into(); + found.push(ColumnFinding { + row: None, + severity: Severity::Warning, + message: message.clone(), + group: Some(DiagnosticGroup { + key: "authority unavailable".into(), + summary: message, + subject: None, + }), + }); + } diagnostics::address(name.into(), column, found) }) .collect(); @@ -332,48 +423,111 @@ fn publish(jobs: &[Job], subdelimiter: &str, cx: &mut App) { } } +struct LookupBatch { + verdicts: Vec<(String, Verdict)>, + /// Lowercased terms the server did not answer, to wait [`RETRY_AFTER`] before asking again. + unanswered: Vec, + failure: Option, +} + +enum Lookup { + Verdict(Verdict), + Retry, + Refused(String), +} + /// Ask `source` about each term in turn, skipping the rest once the request budget is spent. -fn lookup_all(source: &dyn AuthoritySource, terms: &[String]) -> Vec<(String, Verdict)> { - terms - .iter() - .map_while(|term| { - if !take_token() { - log::warn!( - "{}: request budget spent, so the rest of this pass waits for the next one", - source.name() - ); - return None; +fn lookup_all(source: &dyn AuthoritySource, terms: &[String]) -> LookupBatch { + let mut batch = LookupBatch { + verdicts: Vec::new(), + unanswered: Vec::new(), + failure: None, + }; + for term in terms { + if !take_token() { + log::warn!( + "{}: request budget spent, so the rest of this pass waits for the next one", + source.name() + ); + break; + } + match lookup_result(source, term) { + Lookup::Verdict(verdict) => batch.verdicts.push((term.to_lowercase(), verdict)), + Lookup::Retry => batch.unanswered.push(term.to_lowercase()), + Lookup::Refused(failure) => { + batch.failure = Some(failure); + break; } - Some(lookup(source, term).map(|verdict| (term.to_lowercase(), verdict))) - }) - .flatten() - .collect() + } + } + batch } /// One term. `None` when the authority could not be reached or did not answer with anything /// readable — that must never be cached, and must never become a finding, because "the server is /// down" reported as "this heading is wrong" is the one mistake this crate must not make. -fn lookup(source: &dyn AuthoritySource, term: &str) -> Option { - let response = client() - .get(source.lookup_url(term)) - .send() - .and_then(|r| r.error_for_status()) - .and_then(reqwest::blocking::Response::text); - let body = match response { - Ok(body) => body, +fn lookup_result(source: &dyn AuthoritySource, term: &str) -> Lookup { + let response = match client().get(source.lookup_url(term)).send() { + Ok(response) => response, Err(err) => { log::warn!("{}: could not check “{term}”: {err}", source.name()); - return None; + return Lookup::Retry; + } + }; + let status = response.status(); + let body = match response.text() { + Ok(body) => body, + Err(err) => { + log::warn!( + "{}: could not read the answer for “{term}”: {err}", + source.name() + ); + return Lookup::Retry; } }; + interpret(source, term, status, &body) +} + +#[cfg(test)] +fn lookup(source: &dyn AuthoritySource, term: &str) -> Option { + match lookup_result(source, term) { + Lookup::Verdict(verdict) => Some(verdict), + Lookup::Retry | Lookup::Refused(_) => None, + } +} + +fn interpret( + source: &dyn AuthoritySource, + term: &str, + status: reqwest::StatusCode, + body: &str, +) -> Lookup { + let refusal = source.refusal(body); + if !status.is_success() { + let reason = refusal.unwrap_or_else(|| format!("HTTP {status}")); + if matches!(status.as_u16(), 401 | 403) { + return Lookup::Refused(reason); + } + log::warn!( + "{}: could not check “{term}”: HTTP {status}: {reason}", + source.name() + ); + return Lookup::Retry; + } + if let Some(reason) = refusal { + return Lookup::Refused(reason); + } // An empty list is the authority saying "no such heading" and must be cached as one. Only an // answer we could not read at all is dropped. - let mut labels = source.labels(&body)?; + let Some(mut labels) = source.labels(body) else { + return Lookup::Retry; + }; let known = labels.iter().any(|label| label.eq_ignore_ascii_case(term)); // One heading can appear several times, once per authority record that carries it. - labels.dedup(); - Some(Verdict { + let mut seen = HashSet::new(); + labels.retain(|label| seen.insert(label.to_lowercase())); + Lookup::Verdict(Verdict { known, suggestions: if known { Vec::new() } else { labels }, }) @@ -416,44 +570,160 @@ fn take_token() -> bool { /// /// Reads the cache only — a menu opens on a click and cannot wait for a round trip. A term whose /// verdict has not arrived offers nothing, and the menu simply doesn't appear. -fn offer(_: &Location, text: &str, cx: &App) -> Vec { +fn offer(location: &Location, text: &str, subject: Option<&str>, cx: &App) -> Vec { let Some(cache) = cx.try_global::() else { return Vec::new(); }; + let Some(source) = location + .column + .as_deref() + .and_then(|column| settings::columns::get(column, cx).authority) + else { + return Vec::new(); + }; + let Some(verdicts) = cache.terms.get(&source) else { + return Vec::new(); + }; let subdelimiter = settings::effective_text(settings::FILTER_SUBDELIMITER_KEY, cx).to_string(); - cache - .terms - .values() - .flat_map(|verdicts| { - values_in(text, &subdelimiter) - .into_iter() - .filter_map(|value| Some((value.clone(), verdicts.get(&value.to_lowercase())?))) - .filter(|(_, verdict)| !verdict.known) - .flat_map(|(value, verdict)| { - verdict.suggestions.iter().map(move |suggestion| Fix { - label: format!("{value} → {suggestion}").into(), - // The whole cell, with just the rejected value swapped, so a multi-valued - // cell keeps the values that were fine. - replacement: text.replace(&value, suggestion).into(), - }) - }) - .collect::>() + values_in(text, &subdelimiter) + .into_iter() + .filter(|value| subject.is_none_or(|subject| value == subject)) + .filter_map(|value| Some((value.clone(), verdicts.get(&value.to_lowercase())?))) + .filter(|(_, verdict)| !verdict.known) + .flat_map(|(value, verdict)| { + verdict.suggestions.iter().map(move |suggestion| Fix { + label: format!("{value} → {suggestion}").into(), + // The whole cell, with just the rejected value swapped, so a multi-valued + // cell keeps the values that were fine. + replacement: text.replace(&value, suggestion).into(), + }) }) .collect() } +/// "Try again" once a source has been refused, otherwise "Refresh": both ask the server afresh. +fn actions(name: &str, cx: &App) -> Vec { + let failed = cx + .try_global::() + .is_some_and(|cache| cache.failures.contains_key(name)); + let label = if failed { + format!("Try {name} again") + } else { + format!("Refresh {name} results") + }; + let name = name.to_owned(); + vec![GroupFix::action(label, move |cx| { + refresh(&name, failed, cx) + })] +} + +/// Forget a refusal, or every verdict, and check again. +fn refresh(name: &str, only_failure: bool, cx: &mut App) { + ensure_loaded(cx); + let cache = cx.default_global::(); + cache.failures.remove(name); + cache.unanswered.remove(name); + if !only_failure { + cache.terms.remove(name); + if let Some(path) = cache_path(name) + && let Err(err) = std::fs::remove_file(&path) + && err.kind() != std::io::ErrorKind::NotFound + { + log::warn!( + "could not clear the saved {name} answers at {}, so a restart may show old results: {err}", + path.display() + ); + } + } + if let Some(hooks) = cx.try_global::().copied() { + (hooks.revalidate)(cx); + } +} + /// Wire the authority check in. Reached through [`crate::init`]. pub fn init(cx: &mut App) { for name in source::NAMES { FixProviders::register(name, offer, cx); + SourceActions::register(name, actions, cx); } diagnostics::AsyncValidators::register("authority", check, cx); } #[cfg(test)] mod tests { - use crate::authority::{Verdict, values_in}; + use crate::authority::geonames::GeoNames; + use crate::authority::{Lookup, Verdict, interpret, values_in}; + + #[gpui::test] + fn a_repeated_value_is_asked_once_and_never_while_pending(cx: &mut gpui::TestAppContext) { + use crate::authority::{Cache, Job, claim}; + let job = || Job { + source: Box::new(GeoNames { + username: "archivist".into(), + }), + scope: "GeoNames:archivist".into(), + columns: vec![diagnostics::ColumnSnapshot { + name: "Place".into(), + data_type: "Text".into(), + settings: Default::default(), + values: ["Vancouver", "vancouver", "Vancouver; Paris", "Surrey"] + .map(Into::into) + .to_vec(), + subdelimiter: ";".into(), + row_ids: std::sync::Arc::new([]), + }], + }; + cx.update(|cx| { + let cache = cx.default_global::(); + cache.loaded = true; + cache + .scopes + .insert("GeoNames".into(), "GeoNames:archivist".into()); + cache.terms.entry("GeoNames".into()).or_default().insert( + "surrey".into(), + Verdict { + known: true, + suggestions: Vec::new(), + }, + ); + let asked = claim(&[job()], ";", cx); + assert_eq!(asked.len(), 1); + assert_eq!(asked[0].2, ["Vancouver", "Paris"]); + assert!( + claim(&[job()], ";", cx).is_empty(), + "an overlapping run finds every term already pending" + ); + }); + } + + #[gpui::test] + fn trying_a_refused_source_again_keeps_its_answers(cx: &mut gpui::TestAppContext) { + cx.update(|cx| { + let cache = cx.default_global::(); + cache.loaded = true; + cache + .failures + .insert("GeoNames".into(), "credits spent".into()); + cache.terms.entry("GeoNames".into()).or_default().insert( + "paris".into(), + Verdict { + known: true, + suggestions: Vec::new(), + }, + ); + let offered = crate::authority::actions("GeoNames", cx); + assert_eq!(offered[0].label, "Try GeoNames again"); + offered[0].apply(cx); + let cache = cx.global::(); + assert!(cache.failures.is_empty()); + assert!(cache.terms["GeoNames"].contains_key("paris")); + assert_eq!( + crate::authority::actions("GeoNames", cx)[0].label, + "Refresh GeoNames results" + ); + }); + } #[test] fn a_multi_valued_cell_splits_into_its_values() { @@ -484,6 +754,45 @@ mod tests { assert_eq!(verdict.suggestions.len(), 1); } + #[test] + fn a_geonames_refusal_stops_the_source_and_keeps_its_message() { + let source = GeoNames { + username: "invalid".into(), + }; + let refused = r#"{"status":{"message":"user does not exist","value":10}}"#; + assert!(matches!( + interpret( + &source, + "Vancouver", + reqwest::StatusCode::UNAUTHORIZED, + refused + ), + Lookup::Refused(message) if message == "user does not exist" + )); + } + + #[test] + fn suggestions_are_deduplicated_without_changing_rank_order() { + let source = GeoNames { + username: "valid".into(), + }; + let body = r#"{"geonames":[ + {"name":"Vancouver Island"}, + {"name":"Vancouver"}, + {"name":"Vancouver Island"} + ]}"#; + let Lookup::Verdict(verdict) = + interpret(&source, "Vancouvr", reqwest::StatusCode::OK, body) + else { + panic!("the response must produce a verdict"); + }; + assert_eq!( + verdict.suggestions, + ["Vancouver Island", "Vancouver"], + "the API's best-first order is preserved" + ); + } + /// The budget is a backstop against a runaway loop, so what matters is that it stops rather /// than exactly where — a run that spends it all still gets [`super::BUDGET`] requests. #[test] diff --git a/crates/checks/src/authority/source.rs b/crates/checks/src/authority/source.rs index a84250ca..4ce4eca3 100644 --- a/crates/checks/src/authority/source.rs +++ b/crates/checks/src/authority/source.rs @@ -31,6 +31,16 @@ pub trait AuthoritySource: Send + Sync + 'static { /// Where to ask about `term`. fn lookup_url(&self, term: &str) -> String; + /// Cache identity for configuration that can change what this source answers. + fn cache_scope(&self) -> String { + self.name().to_string() + } + + /// A source-level refusal that makes more requests in this pass pointless. + fn refusal(&self, _body: &str) -> Option { + None + } + /// The headings the response offers, best first. /// /// `None` means the answer could not be read at all — a truncated body, an error page, a diff --git a/crates/checks/src/dates.rs b/crates/checks/src/dates.rs index 6a990b9f..9344768d 100644 --- a/crates/checks/src/dates.rs +++ b/crates/checks/src/dates.rs @@ -6,7 +6,10 @@ //! value this rejects is usually prose — `circa 1987`, `May 1987`, `1987-5-3` — which is exactly //! what an ingest would choke on. -use diagnostics::{ColumnInfo, ColumnValidator, Fix, FixProviders, Location, Severity}; +use diagnostics::{ + ColumnFinding, ColumnInfo, ColumnValidator, ColumnValues, DiagnosticGroup, Fix, FixProviders, + Location, Severity, +}; use gpui::{App, SharedString}; use settings::columns::ColumnType; @@ -62,22 +65,29 @@ impl ColumnValidator for DateCheck { fn validate( &self, column: &ColumnInfo, - values: &[SharedString], - ) -> Vec<(usize, Severity, SharedString)> { + values: ColumnValues<'_>, + ) -> Vec { if ColumnType::from_declared(column.data_type) != ColumnType::Date { return Vec::new(); } values .iter() - .enumerate() - .filter(|(_, value)| !value.trim().is_empty()) - .filter(|(_, value)| !valid(value.trim())) - .map(|(row, value)| { - ( - row, - Severity::Error, - format!("“{}” is not an EDTF date", value.trim()).into(), - ) + .flat_map(|cell| { + cell.parts() + .filter(|value| !valid(value)) + .map(move |value| { + let message: SharedString = format!("“{value}” is not an EDTF date").into(); + ColumnFinding { + row: Some(cell.row), + severity: Severity::Error, + message: message.clone(), + group: Some(DiagnosticGroup { + key: value.to_owned().into(), + summary: message, + subject: Some(value.to_owned().into()), + }), + } + }) }) .collect() } @@ -117,15 +127,16 @@ fn rewrites(value: &str) -> Vec { offered } -fn offer(_: &Location, text: &str, _: &App) -> Vec { - if valid(text.trim()) { +fn offer(_: &Location, text: &str, subject: Option<&str>, _: &App) -> Vec { + let value = subject.unwrap_or(text); + if valid(value.trim()) { return Vec::new(); } - rewrites(text) + rewrites(value) .into_iter() .map(|fixed| Fix { label: format!("Use {fixed}").into(), - replacement: fixed.into(), + replacement: text.replacen(value, &fixed, 1).into(), }) .collect() } @@ -137,7 +148,35 @@ pub fn init(cx: &mut App) { #[cfg(test)] mod tests { - use crate::dates::{rewrites, valid}; + use crate::dates::{DateCheck, offer, rewrites, valid}; + use diagnostics::{ColumnInfo, ColumnValidator, ColumnValues, DATASET_MAIN, Location}; + use gpui::SharedString; + use settings::columns::ColumnSettings; + + #[gpui::test] + fn the_same_bad_date_groups_and_fixes_only_its_own_value(cx: &mut gpui::TestAppContext) { + let settings = ColumnSettings::default(); + let column = ColumnInfo { + name: "Date", + data_type: "Date", + settings: &settings, + }; + let raw: Vec = vec!["circa 1987".into(), "1990 | circa 1987".into()]; + let found = DateCheck.validate(&column, ColumnValues::new(&raw, "|")); + let groups: Vec<_> = found.iter().map(|f| f.group.clone().unwrap()).collect(); + assert_eq!(groups.len(), 2); + assert_eq!(groups[0], groups[1]); + assert_eq!(groups[0].subject.as_deref(), Some("circa 1987")); + + let location = Location { + dataset: DATASET_MAIN.into(), + row: Some(1), + row_id: None, + column: Some("Date".into()), + }; + let fixes = cx.update(|cx| offer(&location, &raw[1], Some("circa 1987"), cx)); + assert_eq!(fixes[0].replacement.as_ref(), "1990 | 1987~"); + } /// The shapes an archive actually needs EDTF for — uncertainty and imprecision — must pass, /// or the validator is just a stricter date field nobody wants. diff --git a/crates/clustering/Cargo.toml b/crates/clustering/Cargo.toml new file mode 100644 index 00000000..fee73deb --- /dev/null +++ b/crates/clustering/Cargo.toml @@ -0,0 +1,16 @@ +[package] +name = "clustering" +version.workspace = true +edition.workspace = true + +[dependencies] +diagnostics = { path = "../diagnostics" } +settings = { path = "../settings" } +gpui.workspace = true +log.workspace = true +unicode-normalization = "0.1" +caseless = "0.2" +strsim = "0.11" + +[dev-dependencies] +gpui = { workspace = true, features = ["test-support"] } diff --git a/crates/clustering/src/core.rs b/crates/clustering/src/core.rs new file mode 100644 index 00000000..de92ff10 --- /dev/null +++ b/crates/clustering/src/core.rs @@ -0,0 +1,354 @@ +//! Pair evidence joined into review clusters. A cluster is advisory until the user resolves it. +use caseless::default_case_fold_str; +use std::collections::{BTreeMap, BTreeSet}; +use unicode_normalization::{UnicodeNormalization, char::is_combining_mark}; + +#[derive(Clone, Debug)] +pub struct Value { + pub displayed: String, + pub rows: Vec, + strict: Vec, + sorted: Vec, + loose: Vec, +} + +impl Value { + fn new(displayed: String, mut rows: Vec) -> Self { + rows.sort_unstable(); + rows.dedup(); + let strict = tokens(&displayed, false); + let mut sorted = strict.clone(); + sorted.sort(); + let loose = tokens(&displayed, true); + Self { + displayed, + rows, + strict, + sorted, + loose, + } + } +} + +#[derive(Clone, Debug)] +pub struct Pair { + pub left: Value, + pub right: Value, + pub reason: &'static str, + pub key: String, +} + +pub struct Comparison { + pub pairs: Vec, + pub distinct_values: usize, + pub candidate_pairs: usize, +} + +#[derive(Clone, Debug)] +pub struct Cluster { + pub members: Vec, + pub reasons: Vec<&'static str>, + pub key: String, +} + +fn tokens(text: &str, strip_marks: bool) -> Vec { + let normalized: String = text.nfkc().collect(); + let folded = default_case_fold_str(&normalized); + let comparable = if strip_marks { + folded.nfkd().filter(|c| !is_combining_mark(*c)).collect() + } else { + folded + }; + comparable + .split(|c: char| !c.is_alphanumeric()) + .filter(|token| !token.is_empty()) + .map(str::to_owned) + .collect() +} + +fn one_close_token(left: &[String], right: &[String]) -> bool { + if left.len() != right.len() || left.is_empty() { + return false; + } + let changed: Vec<_> = left.iter().zip(right).filter(|(a, b)| a != b).collect(); + changed.len() == 1 + && left.len() >= 2 + && changed[0].0.chars().all(char::is_alphabetic) + && changed[0].1.chars().all(char::is_alphabetic) + && strsim::normalized_damerau_levenshtein(changed[0].0, changed[0].1) >= 0.8 +} + +fn abbreviated_or_extra(left: &[String], right: &[String]) -> Option<&'static str> { + let (short, long) = if left.len() <= right.len() { + (left, right) + } else { + (right, left) + }; + // ponytail: one extra word at most, so "New York" never swallows "New York Times Magazine". + if short.len() < 2 || long.len() > short.len() + 1 { + return None; + } + let mut rest: Vec<&str> = long.iter().map(String::as_str).collect(); + let mut unmatched = Vec::new(); + for token in short { + match rest.iter().position(|other| other == token) { + Some(ix) => { + rest.swap_remove(ix); + } + None => unmatched.push(token.as_str()), + } + } + if unmatched.len() == short.len() { + return None; + } + let initial = |a: &str, b: &str| { + a.chars().count() == 1 + && a.chars().all(char::is_alphabetic) + && b.len() > a.len() + && b.starts_with(a) + }; + let abbreviated = !unmatched.is_empty(); + for token in unmatched { + let ix = rest + .iter() + .position(|other| initial(token, other) || initial(other, token))?; + rest.swap_remove(ix); + } + match rest.as_slice() { + [] if abbreviated => Some("a word is abbreviated"), + [extra] if extra.chars().all(char::is_alphabetic) => Some(if abbreviated { + "a word is abbreviated" + } else { + "one value has an extra word" + }), + _ => None, + } +} + +fn reason(left: &Value, right: &Value) -> Option<&'static str> { + if left.strict.is_empty() || right.strict.is_empty() { + return None; + } + if left.strict == right.strict { + return Some("formatting differs"); + } + if left.sorted == right.sorted { + return Some("the same words appear in a different order"); + } + if left.loose == right.loose { + return Some("accents or diacritics differ"); + } + if one_close_token(&left.strict, &right.strict) + || one_close_token( + &left.strict.iter().rev().cloned().collect::>(), + &right.strict, + ) + { + return Some("one word has a close spelling"); + } + abbreviated_or_extra(&left.strict, &right.strict) +} + +fn candidate_pairs(values: &[Value]) -> BTreeSet<(usize, usize)> { + let mut exact: BTreeMap, Vec> = BTreeMap::new(); + let mut ordered: BTreeMap, Vec> = BTreeMap::new(); + let mut loose: BTreeMap, Vec> = BTreeMap::new(); + let mut shared: BTreeMap> = BTreeMap::new(); + for (ix, value) in values.iter().enumerate() { + if value.strict.is_empty() { + continue; + } + exact.entry(value.strict.clone()).or_default().push(ix); + ordered.entry(value.sorted.clone()).or_default().push(ix); + loose.entry(value.loose.clone()).or_default().push(ix); + for token in value.strict.iter().collect::>() { + shared.entry(token.clone()).or_default().push(ix); + } + } + let mut pairs = BTreeSet::new(); + for group in exact + .into_values() + .chain(ordered.into_values()) + .chain(loose.into_values()) + .chain(shared.into_values().filter(|group| group.len() <= 100)) + { + for (offset, &left) in group.iter().enumerate() { + for &right in &group[offset + 1..] { + pairs.insert((left.min(right), left.max(right))); + } + } + } + pairs +} + +pub fn compare<'a>(values: impl IntoIterator) -> Vec { + let comparison = compare_indexed(values.into_iter().enumerate()); + comparison.pairs +} + +pub fn compare_indexed<'a>(values: impl IntoIterator) -> Comparison { + let mut grouped: BTreeMap> = BTreeMap::new(); + for (row, value) in values { + if !value.trim().is_empty() { + grouped.entry(value.to_owned()).or_default().push(row); + } + } + let distinct: Vec<_> = grouped + .into_iter() + .map(|(text, rows)| Value::new(text, rows)) + .collect(); + let candidates = candidate_pairs(&distinct); + let candidate_count = candidates.len(); + let pairs = candidates + .into_iter() + .filter_map(|(left, right)| { + let (left, right) = (&distinct[left], &distinct[right]); + Some(Pair { + reason: reason(left, right)?, + key: format!("{:?}", (&left.displayed, &right.displayed)), + left: left.clone(), + right: right.clone(), + }) + }) + .collect(); + Comparison { + pairs, + distinct_values: distinct.len(), + candidate_pairs: candidate_count, + } +} + +pub fn clusters(pairs: Vec) -> Vec { + let mut values = BTreeMap::new(); + let mut links: BTreeMap> = BTreeMap::new(); + for pair in pairs { + let (left, right) = (pair.left.displayed.clone(), pair.right.displayed.clone()); + values.entry(left.clone()).or_insert(pair.left); + values.entry(right.clone()).or_insert(pair.right); + links + .entry(left.clone()) + .or_default() + .push((right.clone(), pair.reason)); + links.entry(right).or_default().push((left, pair.reason)); + } + + let mut unseen: BTreeSet<_> = values.keys().cloned().collect(); + let mut clusters = Vec::new(); + while let Some(start) = unseen.pop_first() { + let mut names = BTreeSet::from([start.clone()]); + let mut reasons = BTreeSet::new(); + let mut stack = vec![start]; + while let Some(name) = stack.pop() { + for (neighbor, reason) in links.get(&name).into_iter().flatten() { + reasons.insert(*reason); + if unseen.remove(neighbor) { + names.insert(neighbor.clone()); + stack.push(neighbor.clone()); + } + } + } + let members = names + .iter() + .filter_map(|name| values.remove(name)) + .collect(); + clusters.push(Cluster { + key: format!("{names:?}"), + members, + reasons: reasons.into_iter().collect(), + }); + } + clusters +} + +#[cfg(test)] +mod tests { + use super::{Value, candidate_pairs, clusters, compare}; + #[test] + fn evidence_and_frequencies_preserve_exact_text() { + let pairs = compare([ + "Agnès Varda", + "Varda, Agnès", + "Agnes Varda", + "Agnès Varad", + "Werner Herzog", + ]); + for why in ["different order", "diacritics differ", "close spelling"] { + assert!(pairs.iter().any(|p| p.reason.contains(why))); + } + assert!( + pairs + .iter() + .all(|p| !p.left.displayed.contains("Werner") + && !p.right.displayed.contains("Werner")) + ); + assert!(compare(["Agnès Varda", "Agnès Varda"]).is_empty()); + let pair = compare(["Alice", " Alice ", "Alice"]).remove(0); + assert_eq!(pair.left.displayed, " Alice "); + assert_eq!(pair.right.rows, [0, 2]); + assert_eq!(pair.key, compare(["Alice", " Alice "])[0].key); + } + #[test] + fn abbreviations_and_extra_words_join_one_cluster() { + let found = clusters(compare([ + "Bains, Satwinder", + "Satwinder, Bains", + "Dr. Satwinder Bains", + "Satinder Bains", + "Satwinder B.", + "Satwinder Singh Bains Kaur", + "Satwinder", + ])); + assert_eq!(found.len(), 1); + assert_eq!(found[0].members.len(), 5); + assert!(compare(["New York", "New York Times Magazine"]).is_empty()); + assert!(compare(["Bains", "Dr. Bains"]).is_empty()); + } + + #[test] + fn blocking_bounds_two_thousand_values() { + let values: Vec<_> = (0..2000) + .map(|ix| Value::new(format!("group{} value{}", ix / 100, ix % 100), vec![ix])) + .collect(); + assert!(candidate_pairs(&values).len() <= 120_000); + } + + #[test] + fn unicode_normalization_does_not_change_displayed_values() { + for (left, right) in [ + ("Straße", "STRASSE"), + ("Ａｌｉｃｅ", "Alice"), + ("Cafe\u{301}", "Café"), + ] { + let pairs = compare([left, right]); + assert_eq!(pairs.len(), 1); + assert_eq!(pairs[0].reason, "formatting differs"); + assert!([left, right].contains(&pairs[0].left.displayed.as_str())); + assert!([left, right].contains(&pairs[0].right.displayed.as_str())); + } + assert!(compare(["Film 12345", "Film 12346", "...", "---"]).is_empty()); + } + + #[test] + fn pair_evidence_keeps_the_observed_edges() { + let pairs = compare(["Alice abcde", "Alice abcdf", "Alice abcdg"]); + assert_eq!(pairs.len(), 3); + let pairs = compare(["Alice abcde", "Alice abcdf", "Alice abccf"]); + assert_eq!(pairs.len(), 2); + assert!(pairs.iter().all(|p| !(p.left.displayed == "Alice abccf" && p.right.displayed == "Alice abcde"))); + } + + #[test] + fn connected_pair_evidence_forms_one_review_cluster() { + let clusters = clusters(compare(["Alice abcde", "Alice abcdf", "Alice abccf"])); + assert_eq!(clusters.len(), 1); + assert_eq!( + clusters[0] + .members + .iter() + .map(|member| member.displayed.as_str()) + .collect::>(), + ["Alice abccf", "Alice abcde", "Alice abcdf"] + ); + assert_eq!(clusters[0].reasons, ["one word has a close spelling"]); + } +} diff --git a/crates/clustering/src/lib.rs b/crates/clustering/src/lib.rs new file mode 100644 index 00000000..0141b8bf --- /dev/null +++ b/crates/clustering/src/lib.rs @@ -0,0 +1,520 @@ +//! Advisory value comparisons, independently registered from spelling. +pub mod core; + +use diagnostics::{ + ColumnFinding, ColumnInfo, ColumnValidator, ColumnValues, DiagnosticGroup, DiagnosticHooks, + Fix, GroupFix, GroupMember, Location, Severity, +}; +use gpui::{App, Global, SharedString}; +use settings::columns::ColumnType; +use std::collections::BTreeMap; +use std::sync::{Arc, RwLock}; +use std::time::{Duration, Instant}; + +pub const VALUE_VARIANTS_NAME: &str = "value variants"; +const SLOW_CLUSTERING: Duration = Duration::from_millis(250); +type Candidates = BTreeMap<(String, usize), (SharedString, Vec<(SharedString, SharedString)>)>; + +#[derive(Clone, Default)] +pub struct ValueVariants { + candidates: Arc>, +} +impl Global for ValueVariants {} + +impl ColumnValidator for ValueVariants { + fn name(&self) -> SharedString { + VALUE_VARIANTS_NAME.into() + } + + fn begin_run(&self) { + if let Ok(mut candidates) = self.candidates.write() { + candidates.clear(); + } + } + + fn validate(&self, column: &ColumnInfo, values: ColumnValues<'_>) -> Vec { + let started = Instant::now(); + let Ok(mut candidates) = self.candidates.write() else { + return Vec::new(); + }; + candidates.retain(|(name, _), _| name != column.name); + if !column.settings.variant_review + || !ColumnType::from_declared(column.data_type).is_prose() + { + return Vec::new(); + } + let mut findings = Vec::new(); + let mut logical_values = 0; + let logical = values + .iter() + .flat_map(|cell| cell.parts().map(move |value| (cell.row, value))) + .inspect(|_| logical_values += 1); + let comparison = core::compare_indexed(logical); + let pairs: Vec<_> = comparison + .pairs + .into_iter() + .filter(|pair| { + !column + .settings + .distinct_variants + .contains(&ordered_pair(&pair.left.displayed, &pair.right.displayed)) + }) + .collect(); + let pair_count = pairs.len(); + let clusters = core::clusters(pairs); + let cluster_count = clusters.len(); + for cluster in clusters { + let reasons = cluster.reasons.join(", "); + let group = DiagnosticGroup { + key: format!("{:?}", (column.name, &cluster.key)).into(), + summary: format!( + "{} values may be variants: {reasons}", + cluster.members.len() + ) + .into(), + subject: None, + }; + for value in &cluster.members { + for &row in &value.rows { + for alternative in &cluster.members { + if alternative.displayed == value.displayed { + continue; + } + let Some(replacement) = + values.replace_part(row, &value.displayed, &alternative.displayed) + else { + continue; + }; + candidates + .entry((column.name.to_owned(), row)) + .or_insert_with(|| (values.raw()[row].clone(), Vec::new())) + .1 + .push((value.displayed.clone().into(), replacement)); + } + findings.push(ColumnFinding { + row: Some(row), + severity: Severity::Warning, + message: value.displayed.clone().into(), + group: Some(DiagnosticGroup { + subject: Some(value.displayed.clone().into()), + ..group.clone() + }), + }); + } + } + } + let elapsed = started.elapsed(); + log::debug!( + "clustered column {:?}: {} rows, {logical_values} logical values, {} distinct values, {} candidate pairs, {pair_count} matches, {cluster_count} clusters, {} findings in {elapsed:?}", + column.name, + values.raw().len(), + comparison.distinct_values, + comparison.candidate_pairs, + findings.len(), + ); + if elapsed >= SLOW_CLUSTERING { + log::warn!( + "slow value clustering in column {:?}: {} rows, {logical_values} logical values, {} distinct values, {} candidate pairs, {pair_count} matches, {cluster_count} clusters, {} findings in {elapsed:?}", + column.name, + values.raw().len(), + comparison.distinct_values, + comparison.candidate_pairs, + findings.len(), + ); + } + findings + } +} + +fn ordered_pair(left: &str, right: &str) -> (String, String) { + if left <= right { + (left.to_owned(), right.to_owned()) + } else { + (right.to_owned(), left.to_owned()) + } +} + +pub fn variant_fixes(location: &Location, text: &str, subject: Option<&str>, cx: &App) -> Vec { + let (Some(row), Some(column), Some(variants)) = ( + location.row, + location.column.as_deref(), + cx.try_global::(), + ) else { + return Vec::new(); + }; + if location.dataset != diagnostics::DATASET_MAIN { + return Vec::new(); + } + let Ok(candidates) = variants.candidates.read() else { + return Vec::new(); + }; + let Some((expected, alternatives)) = candidates.get(&(column.to_owned(), row)) else { + return Vec::new(); + }; + if expected.as_ref() != text { + return Vec::new(); + } + alternatives + .iter() + .filter(|(observed, _)| subject.is_none_or(|subject| observed == subject)) + .filter(|(_, replacement)| replacement.as_ref() != text) + .map(|(_, replacement)| Fix { + label: format!("Use “{replacement}”").into(), + replacement: replacement.clone(), + }) + .collect() +} + +pub fn variant_group_fixes(members: &[GroupMember], cx: &App) -> Vec { + let mut forms: Vec<_> = members + .iter() + .map(|member| member.message.to_string()) + .collect(); + forms.sort(); + forms.dedup(); + if forms.len() < 2 { + return Vec::new(); + } + let delimiter = settings::effective_text(settings::FILTER_SUBDELIMITER_KEY, cx); + let mut fixes = forms + .iter() + .map(|target| { + let (target, members, delimiter) = + (target.clone(), members.to_vec(), delimiter.clone()); + GroupFix::action(format!("Change all to “{target}”"), move |cx| { + let Some(replacements) = replacements_to(&members, &target, &delimiter) else { + return; + }; + if let Some(hooks) = cx.try_global::().copied() { + (hooks.set_texts)(replacements, cx); + } + }) + }) + .collect::>(); + let Some(column) = members + .first() + .and_then(|member| member.location.column.clone()) + else { + return fixes; + }; + fixes.push(GroupFix::action( + "These are distinct — ignore this cluster", + move |cx| { + let distinct = forms + .iter() + .enumerate() + .flat_map(|(offset, left)| { + forms[offset + 1..] + .iter() + .map(move |right| ordered_pair(left, right)) + }) + .collect::>(); + settings::columns::update( + &column, + |settings| { + settings.distinct_variants.extend(distinct); + }, + cx, + ); + if let Some(hooks) = cx.try_global::().copied() { + (hooks.revalidate)(cx); + } + }, + )); + fixes +} + +fn replacements_to( + members: &[GroupMember], + target: &str, + delimiter: &str, +) -> Option> { + type CellKey = (SharedString, Option, Option); + let mut cells: BTreeMap = BTreeMap::new(); + for member in members + .iter() + .filter(|member| member.message.as_ref() != target) + { + let key = ( + member.location.dataset.clone(), + member.location.row, + member.location.column.clone(), + ); + let (_, text) = cells + .entry(key) + .or_insert_with(|| (member.location.clone(), member.text.clone())); + let raw = [text.clone()]; + *text = ColumnValues::new(&raw, delimiter).replace_part(0, &member.message, target)?; + } + Some(cells.into_values().collect()) +} + +#[cfg(test)] +mod tests { + use crate::{ValueVariants, ordered_pair, replacements_to, variant_fixes, variant_group_fixes}; + use diagnostics::{ + ColumnInfo, ColumnValidator, ColumnValues, DATASET_MAIN, GroupMember, Location, Severity, + }; + use gpui::TestAppContext; + use settings::columns::ColumnSettings; + + #[gpui::test] + fn opt_in_grouping_and_stale_fixes(cx: &mut TestAppContext) { + let variants = ValueVariants::default(); + let values = [ + "Agnès Varda".into(), + "Varda, Agnès".into(), + "Agnès Varda".into(), + ]; + let mut settings = ColumnSettings::default(); + assert!( + variants + .validate( + &ColumnInfo { + name: "Creator", + data_type: "Text", + settings: &settings, + }, + ColumnValues::new(&values, ""), + ) + .is_empty() + ); + settings.variant_review = true; + let found = variants.validate( + &ColumnInfo { + name: "Creator", + data_type: "Text", + settings: &settings, + }, + ColumnValues::new(&values, ""), + ); + assert_eq!(found.len(), 3); + let first_group = found[0].group.as_ref().unwrap(); + assert!(found.iter().all(|finding| { + finding.group.as_ref().is_some_and(|group| { + group.key == first_group.key && group.summary == first_group.summary + }) + })); + assert_eq!( + found + .iter() + .map(|finding| finding.message.as_ref()) + .collect::>(), + ["Agnès Varda", "Agnès Varda", "Varda, Agnès"] + ); + cx.update(|cx| { + cx.set_global(variants); + let location = Location::cell(DATASET_MAIN, 0, None, "Creator"); + assert_eq!( + variant_fixes(&location, "Agnès Varda", None, cx)[0].replacement, + "Varda, Agnès" + ); + assert!(variant_fixes(&location, "changed", None, cx).is_empty()); + cx.global::().begin_run(); + assert!(variant_fixes(&location, "Agnès Varda", None, cx).is_empty()); + }); + } + + #[test] + fn subdelimited_cells_compare_each_value_and_keep_whole_cell_fixes() { + let variants = ValueVariants::default(); + let settings = ColumnSettings { + variant_review: true, + ..Default::default() + }; + let values = [ + "Busson, Carl W.|Dhillon, Baltej Singh".into(), + "Carl W. Busson|Baltej Singh Dhillon".into(), + ]; + let found = variants.validate( + &ColumnInfo { + name: "Creator", + data_type: "Text", + settings: &settings, + }, + ColumnValues::new(&values, "|"), + ); + assert_eq!(found.len(), 4); + assert!(found.iter().all(|finding| { + finding.severity == Severity::Warning && !finding.message.contains('|') + })); + let candidates = variants.candidates.read().unwrap(); + let (expected, replacements) = &candidates[&("Creator".to_owned(), 0)]; + assert_eq!(expected, "Busson, Carl W.|Dhillon, Baltej Singh"); + assert!( + replacements + .iter() + .any(|(_, replacement)| replacement == "Carl W. Busson|Dhillon, Baltej Singh") + ); + assert!( + replacements + .iter() + .any(|(_, replacement)| replacement == "Busson, Carl W.|Baltej Singh Dhillon") + ); + } + + #[test] + fn confirmed_distinct_pairs_are_not_reported() { + let variants = ValueVariants::default(); + let mut settings = ColumnSettings { + variant_review: true, + ..Default::default() + }; + settings + .distinct_variants + .insert(ordered_pair("Agnès Varda", "Agnes Varda")); + let values = ["Agnès Varda".into(), "Agnes Varda".into()]; + assert!( + variants + .validate( + &ColumnInfo { + name: "Creator", + data_type: "Text", + settings: &settings, + }, + ColumnValues::new(&values, ""), + ) + .is_empty() + ); + } + + #[test] + fn connected_matches_share_one_multi_value_group() { + let variants = ValueVariants::default(); + let settings = ColumnSettings { + variant_review: true, + ..Default::default() + }; + let values = [ + "Alice abcde".into(), + "Alice abcdf".into(), + "Alice abccf".into(), + ]; + let found = variants.validate( + &ColumnInfo { + name: "Creator", + data_type: "Text", + settings: &settings, + }, + ColumnValues::new(&values, ""), + ); + assert_eq!(found.len(), 3); + let group = found[0].group.as_ref().unwrap(); + assert!(group.summary.starts_with("3 values may be variants")); + assert!(found.iter().all(|finding| { + finding + .group + .as_ref() + .is_some_and(|candidate| candidate.key == group.key) + })); + } + + #[test] + fn resolving_a_cluster_replaces_every_member_in_one_multi_value_cell() { + let location = Location::cell(DATASET_MAIN, 0, None, "Creator"); + let members = ["Alice abcde", "Alice abcdf"].map(|value| GroupMember { + location: location.clone(), + text: "Alice abcde|Alice abcdf".into(), + message: value.into(), + subject: Some(value.into()), + }); + let replacements = replacements_to(&members, "Alice abccf", "|").unwrap(); + assert_eq!(replacements.len(), 1); + assert_eq!(replacements[0].1, "Alice abccf|Alice abccf"); + } + + #[gpui::test] + fn group_resolver_offers_every_form_and_distinct(cx: &mut TestAppContext) { + #[derive(Default)] + struct Revalidations(usize); + impl gpui::Global for Revalidations {} + #[derive(Default)] + struct Applied(Vec<(Location, gpui::SharedString)>); + impl gpui::Global for Applied {} + + cx.update(|cx| { + let mut settings = settings::AppSettings::default(); + settings.values.insert( + settings::FILTER_SUBDELIMITER_KEY.into(), + settings::Val::Text("|".into()), + ); + cx.set_global(settings); + cx.set_global(Revalidations::default()); + cx.set_global(Applied::default()); + cx.set_global(settings::project::CurrentProject { + file: std::env::temp_dir().join("qrate-clustering-ignore.qrate"), + data: settings::project::ProjectData { + name: "Test".into(), + columns: Vec::new(), + headers: vec!["Creator".into()], + rows: Vec::new(), + row_ids: Vec::new(), + values: Default::default(), + }, + }); + cx.set_global(diagnostics::DiagnosticHooks { + reveal: |_, _| {}, + text_at: |_, _| None, + set_text: |_, _, _| {}, + set_texts: |replacements, cx| { + use gpui::BorrowAppContext as _; + cx.update_global::(|applied, _| applied.0 = replacements); + }, + revalidate: |cx| { + use gpui::BorrowAppContext as _; + cx.update_global::(|count, _| count.0 += 1); + }, + }); + let members = [ + GroupMember { + location: Location::cell(DATASET_MAIN, 0, None, "Creator"), + text: "Agnès Varda|Director".into(), + message: "Agnès Varda".into(), + subject: Some("Agnès Varda".into()), + }, + GroupMember { + location: Location::cell(DATASET_MAIN, 1, None, "Creator"), + text: "Agnes Varda|Producer".into(), + message: "Agnes Varda".into(), + subject: Some("Agnes Varda".into()), + }, + GroupMember { + location: Location::cell(DATASET_MAIN, 2, None, "Creator"), + text: "Varda, Agnès|Editor".into(), + message: "Varda, Agnès".into(), + subject: Some("Varda, Agnès".into()), + }, + ]; + let fixes = variant_group_fixes(&members, cx); + assert_eq!( + fixes + .iter() + .map(|fix| fix.label.as_ref()) + .collect::>(), + [ + "Change all to “Agnes Varda”", + "Change all to “Agnès Varda”", + "Change all to “Varda, Agnès”", + "These are distinct — ignore this cluster", + ] + ); + fixes[0].apply(cx); + assert_eq!(cx.global::().0.len(), 2); + assert_eq!( + cx.global::().0[0].1, + "Agnes Varda|Director", + "the other logical value remains unchanged" + ); + fixes[3].apply(cx); + let ignored = &settings::columns::get("Creator", cx).distinct_variants; + for pair in [ + ordered_pair("Agnès Varda", "Agnes Varda"), + ordered_pair("Agnès Varda", "Varda, Agnès"), + ordered_pair("Agnes Varda", "Varda, Agnès"), + ] { + assert!(ignored.contains(&pair)); + } + assert_eq!(cx.global::().0, 1); + }); + } +} diff --git a/crates/diagnostics/src/fixes.rs b/crates/diagnostics/src/fixes.rs index 003e8a74..cb2e8ef1 100644 --- a/crates/diagnostics/src/fixes.rs +++ b/crates/diagnostics/src/fixes.rs @@ -1,20 +1,25 @@ //! Corrections a diagnostic's producer can offer, and the menu that shows them. //! -//! The generalisation of what [`crate::spelling`] does for one producer. A [`Diagnostic`] still +//! The generalisation of the spelling menu to every producer. A [`Diagnostic`] still //! carries nothing but a sentence — a `fixes` field would make every producer serialise its //! corrections into the store on every run, most of which nobody ever right-clicks. Instead a //! producer registers a function here and is asked when a menu opens, against the cell's text //! *now* rather than the text that produced the finding. //! -//! Keyed by the same string [`Source::label`] returns, so a producer's findings and its fixes -//! meet without either side holding a handle to the other. +//! Keyed by the stable producer identity in [`Source::key`], so display-name changes cannot +//! disconnect a producer's findings from its fixes. use std::collections::BTreeMap; +use std::rc::Rc; use gpui::{App, Context, Global, SharedString, Window}; +use gpui_component::WindowExt as _; +use gpui_component::button::Button; use gpui_component::menu::{PopupMenu, PopupMenuItem}; +use gpui_component::notification::Notification; +use settings::project::RowId; -use crate::{Diagnostics, Location}; +use crate::{Diagnostic, DiagnosticHooks, Diagnostics, Location, SpellActions}; /// One offered correction: what to call it, and what the cell becomes if it is taken. #[derive(Clone, PartialEq, Eq, Debug)] @@ -26,7 +31,50 @@ pub struct Fix { } /// What a producer is asked when a menu opens: where the finding is, and what the cell says now. -pub type OfferFixes = fn(&Location, &str, &App) -> Vec; +pub type OfferFixes = fn(&Location, &str, Option<&str>, &App) -> Vec; + +#[derive(Clone)] +pub struct GroupMember { + pub location: Location, + pub text: SharedString, + pub message: SharedString, + pub subject: Option, +} + +#[derive(Clone)] +pub struct GroupFix { + pub label: SharedString, + apply: Rc, +} + +impl GroupFix { + pub fn replacements( + label: impl Into, + replacements: Vec<(Location, SharedString)>, + ) -> Self { + Self { + label: label.into(), + apply: Rc::new(move |cx| { + if let Some(hooks) = cx.try_global::().copied() { + (hooks.set_texts)(replacements.clone(), cx); + } + }), + } + } + + pub fn action(label: impl Into, apply: impl Fn(&mut App) + 'static) -> Self { + Self { + label: label.into(), + apply: Rc::new(apply), + } + } + + pub fn apply(&self, cx: &mut App) { + (self.apply)(cx); + } +} + +pub type OfferGroupFixes = fn(&[GroupMember], &App) -> Vec; /// Which producers can offer corrections, by source name. #[derive(Default)] @@ -42,10 +90,31 @@ impl FixProviders { } } +#[derive(Default)] +pub struct GroupFixProviders(BTreeMap); + +impl Global for GroupFixProviders {} + +impl GroupFixProviders { + pub fn register(source: &str, offer: OfferGroupFixes, cx: &mut App) { + cx.default_global::().0.insert(source.into(), offer); + } +} + /// Every correction offered for the findings at `location`, in the order their sources sort. /// /// Public because the cell renderer wants to know whether *any* exist without building a menu. pub fn at(location: &Location, text: &str, cx: &App) -> Vec { + at_subject(location, text, None, None, cx) +} + +fn at_subject( + location: &Location, + text: &str, + source: Option<&str>, + subject: Option<&str>, + cx: &App, +) -> Vec { let Some(providers) = cx.try_global::() else { return Vec::new(); }; @@ -55,54 +124,260 @@ pub fn at(location: &Location, text: &str, cx: &App) -> Vec { location.column.as_deref(), cx, ) - .map(|d| d.source.label()) + .map(|d| d.source.key()) + .filter(|candidate| source.is_none_or(|source| candidate == source)) .collect(); // One finding per source is enough to ask it; a column flagged twice by the same rule must // not offer its corrections twice. sources.sort(); sources.dedup(); - sources + let mut fixes: Vec<_> = sources .iter() .filter_map(|source| providers.0.get(source)) - .flat_map(|offer| offer(location, text, cx)) - .collect() + .flat_map(|offer| offer(location, text, subject, cx)) + .collect(); + let mut seen = std::collections::BTreeSet::new(); + fixes.retain(|fix| seen.insert((fix.label.clone(), fix.replacement.clone()))); + fixes } -/// Add a `Fixes` submenu for the findings at `location`, or return `menu` untouched when nothing -/// here offers one. -/// -/// `apply` takes the whole replacement text, the same contract [`crate::spelling::menu`] uses, so -/// a caller needs nothing but its own way of storing a string. -pub fn menu( - location: &Location, - text: &SharedString, +/// Actions about a producer itself rather than any one finding, such as asking a server again. +pub type OfferSourceActions = fn(&str, &App) -> Vec; + +#[derive(Default)] +pub struct SourceActions(BTreeMap); + +impl Global for SourceActions {} + +impl SourceActions { + pub fn register(source: &str, offer: OfferSourceActions, cx: &mut App) { + cx.default_global::().0.insert(source.into(), offer); + } +} + +fn with_source_actions(source: &str, menu: PopupMenu, cx: &App) -> PopupMenu { + let Some(offer) = cx + .try_global::() + .and_then(|actions| actions.0.get(source)) + .copied() + else { + return menu; + }; + offer(source, cx).into_iter().fold(menu, |menu, action| { + menu.item( + PopupMenuItem::new(action.label.clone()).on_click(move |_, _, cx| action.apply(cx)), + ) + }) +} + +pub fn group_menu( + source: &SharedString, + members: &[GroupMember], menu: PopupMenu, window: &mut Window, cx: &mut Context, - apply: impl Fn(SharedString, &mut App) + Clone + 'static, ) -> PopupMenu { - let found = at(location, text, cx); - if found.is_empty() { + let menu = with_source_actions(source, menu, cx); + let mut fixes = cx + .try_global::() + .and_then(|providers| providers.0.get(source)) + .map(|offer| offer(members, cx)) + .unwrap_or_default(); + let mut seen = std::collections::BTreeSet::new(); + fixes.retain(|fix| seen.insert(fix.label.clone())); + if fixes.is_empty() { return menu; } + menu.submenu("Resolve group", window, cx, move |sub, _window, _cx| { + fixes.iter().fold(sub, |sub, fix| { + let fix = fix.clone(); + sub.item(PopupMenuItem::new(fix.label.clone()).on_click(move |_, _, cx| fix.apply(cx))) + }) + }) +} - menu.submenu("Fixes", window, cx, move |sub, _window, _cx| { - found.iter().fold(sub, |sub, fix| { - let (apply, replacement) = (apply.clone(), fix.replacement.clone()); - sub.item( - PopupMenuItem::new(fix.label.clone()) +/// Menu text is often a cell value, which a popup menu never ellipsizes on its own. +pub fn menu_label(text: &str) -> SharedString { + const MAX: usize = 48; + match text.char_indices().nth(MAX) { + Some((cut, _)) => format!("{}…", text[..cut].trim_end()).into(), + None => text.to_owned().into(), + } +} + +/// Everything one finding offers, appended flat to `menu`: its own corrections, then the ways to +/// dismiss it. `text` is the cell as it reads now, `None` when there is no cell to correct. +/// +/// `apply` takes the whole replacement text, so a caller needs nothing but its own way of storing +/// a string. +pub fn finding_menu( + finding: &Diagnostic, + text: Option<&SharedString>, + menu: PopupMenu, + cx: &mut Context, + apply: impl Fn(SharedString, &mut App) + Clone + 'static, +) -> PopupMenu { + let source = finding.source.key(); + let subject = finding.group.as_ref().and_then(|g| g.subject.clone()); + let mut menu = menu; + if let Some(text) = text { + let fix = |menu: PopupMenu, label: SharedString, replacement: SharedString| { + let apply = apply.clone(); + menu.item( + PopupMenuItem::new(menu_label(&label)) .on_click(move |_, _, cx| apply(replacement.clone(), cx)), ) - }) - }) + }; + if source == "spell" + && let Some(actions) = cx.try_global::().copied() + { + let mut found = (actions.suggest)(text, cx); + found.retain(|(word, _)| subject.as_ref().is_none_or(|s| s == word)); + for (word, suggestions) in &found { + for suggestion in suggestions { + let fixed = text.replace(word.as_ref(), suggestion.as_ref()); + menu = fix(menu, format!("{word} → {suggestion}").into(), fixed.into()); + } + } + menu = menu.separator(); + for (word, _) in found { + menu = menu.item( + PopupMenuItem::new(menu_label(&format!("Add “{word}” to dictionary"))) + .on_click(move |_, _, cx| (actions.add_word)(&word, cx)), + ); + } + } + let offered = at_subject( + &finding.location, + text, + Some(&source), + subject.as_deref(), + cx, + ); + for found in offered { + menu = fix(menu, found.label, found.replacement); + } + } + let menu = with_source_actions(&source, menu, cx); + let Some(column) = finding.location.column.clone() else { + return menu; + }; + let menu = menu.separator(); + let ignore_item = |menu: PopupMenu, label: SharedString, occurrence: bool| { + let finding = finding.clone(); + menu.item(PopupMenuItem::new(label).on_click(move |_, window, cx| { + ignore(std::slice::from_ref(&finding), occurrence, window, cx) + })) + }; + let menu = if finding.location.row_id.is_some() { + ignore_item(menu, "Ignore this occurrence".into(), true) + } else { + menu + }; + // Value variants already persist "these are distinct", which is the precise column-wide ignore. + if source == "value variants" { + return menu; + } + ignore_item( + menu, + menu_label(&format!("Ignore in column “{column}”")), + false, + ) +} + +/// Hide `findings` from now on: each one's single occurrence, or its key throughout its column. +pub fn ignore(findings: &[Diagnostic], occurrence: bool, window: &mut Window, cx: &mut App) { + edit_ignores(findings, cx, |settings, source, key, row_id| match row_id { + Some(id) if occurrence => { + settings.ignored_occurrences.insert((source, key, id)); + } + _ if !occurrence => { + settings.ignored_diagnostics.insert((source, key)); + } + _ => {} + }); + + let Some(first) = findings.first() else { + return; + }; + let column = first.location.column.clone().unwrap_or_default(); + let message = if occurrence { + "Ignored this occurrence".to_string() + } else { + let what = first + .group + .as_ref() + .map_or(&first.message, |group| &group.summary); + format!("Ignored “{what}” in {column}") + }; + let undo = findings.to_vec(); + // One toast type, so a newer ignore replaces the older one's Undo instead of stacking them. + struct IgnoreToast; + window.push_notification( + Notification::new() + .id::() + .message(menu_label(&message)) + .action(move |_, _, cx| { + let undo = undo.clone(); + let note = cx.entity(); + Button::new("undo-ignore") + .label("Undo") + .on_click(move |_, window, cx| { + unignore(&undo, cx); + note.update(cx, |note, cx| note.dismiss(window, cx)); + }) + }) + // `action` switches autohide off; an undo offer should still go away on its own. + .autohide(true), + cx, + ); +} + +/// Remove every ignore that hides `findings`, column-wide and single-occurrence alike. +pub fn unignore(findings: &[Diagnostic], cx: &mut App) { + edit_ignores(findings, cx, |settings, source, key, row_id| { + if let Some(id) = row_id { + settings + .ignored_occurrences + .remove(&(source.clone(), key.clone(), id)); + } + settings.ignored_diagnostics.remove(&(source, key)); + }); +} + +// ponytail: one settings write per finding; batch into a single map store if big groups feel slow. +fn edit_ignores( + findings: &[Diagnostic], + cx: &mut App, + edit: impl Fn(&mut settings::columns::ColumnSettings, String, String, Option), +) { + for finding in findings { + let Some(column) = finding.location.column.as_deref() else { + continue; + }; + let (source, key) = ( + finding.source.key().to_string(), + finding.ignore_key().to_string(), + ); + settings::columns::update( + column, + |settings| edit(settings, source, key, finding.location.row_id), + cx, + ); + } + if let Some(hooks) = cx.try_global::().copied() { + (hooks.revalidate)(cx); + } } #[cfg(test)] mod tests { // Never `use super::*` here — see the note in `lib.rs`'s test module. - use crate::fixes::{Fix, FixProviders, at}; - use crate::{DATASET_MAIN, Diagnostic, Diagnostics, Location, Severity, Source}; + use crate::fixes::{Fix, FixProviders, GroupFix, at, at_subject}; + use crate::{ + DATASET_MAIN, Diagnostic, DiagnosticHooks, Diagnostics, Location, Severity, Source, + }; use gpui::{App, SharedString, TestAppContext}; fn location(column: &str) -> Location { @@ -123,13 +398,14 @@ mod tests { severity: Severity::Error, source: Source::Validator(source.into()), message: "not a known term".into(), + group: None, filed: None, }], cx, ); } - fn offer(_: &Location, text: &str, _: &App) -> Vec { + fn offer(_: &Location, text: &str, _: Option<&str>, _: &App) -> Vec { vec![Fix { label: format!("Use “{text}s”").into(), replacement: format!("{text}s").into(), @@ -150,6 +426,62 @@ mod tests { }); } + #[gpui::test] + fn display_names_do_not_disconnect_fix_providers(cx: &mut TestAppContext) { + cx.update(|cx| { + publish("spell", "Title", cx); + publish("LCSH", "Title", cx); + FixProviders::register("spell", offer, cx); + FixProviders::register("LCSH", offer, cx); + + let found = at(&location("Title"), "alice", cx); + assert_eq!(found.len(), 1, "identical fixes are deduplicated"); + assert_eq!(found[0].replacement, SharedString::from("alices")); + assert_eq!( + at_subject( + &location("Title"), + "alice", + Some("spell"), + Some("alice"), + cx, + ) + .len(), + 1, + "a diagnostic row asks only its own producer" + ); + }); + } + + #[gpui::test] + fn a_group_replacement_uses_the_bulk_edit_hook(cx: &mut TestAppContext) { + #[derive(Default)] + struct Applied(Vec<(Location, SharedString)>); + impl gpui::Global for Applied {} + + cx.update(|cx| { + cx.set_global(Applied::default()); + cx.set_global(DiagnosticHooks { + reveal: |_, _| {}, + text_at: |_, _| None, + set_text: |_, _, _| {}, + set_texts: |replacements, cx| { + use gpui::BorrowAppContext as _; + cx.update_global::(|applied, _| applied.0 = replacements); + }, + revalidate: |_| {}, + }); + let replacements = vec![ + (location("Title"), "Alice".into()), + ( + Location::cell(DATASET_MAIN, 1, None, "Title"), + "Alice".into(), + ), + ]; + GroupFix::replacements("Use Alice", replacements.clone()).apply(cx); + assert_eq!(cx.global::().0, replacements); + }); + } + /// A finding from a producer that registered nothing must not manufacture a menu, and neither /// must a location with no findings at all. #[gpui::test] @@ -178,6 +510,7 @@ mod tests { severity: Severity::Error, source: Source::Validator("LCSH".into()), message: "not a known term".into(), + group: None, filed: None, }, Diagnostic { @@ -185,6 +518,7 @@ mod tests { severity: Severity::Warning, source: Source::Validator("LCSH".into()), message: "also deprecated".into(), + group: None, filed: None, }, ]; @@ -198,4 +532,14 @@ mod tests { assert_eq!(at(&location("Subject"), "Photograph", cx).len(), 1); }); } + + #[test] + fn menu_labels_cut_long_text_on_a_char_boundary() { + use crate::fixes::menu_label; + assert_eq!(menu_label("short"), "short"); + let long = "l’enfant ".repeat(10); + let label = menu_label(&long); + assert!(label.ends_with('…')); + assert_eq!(label.chars().count(), 49, "48 kept plus the ellipsis"); + } } diff --git a/crates/diagnostics/src/lib.rs b/crates/diagnostics/src/lib.rs index ccd02561..9d7df010 100644 --- a/crates/diagnostics/src/lib.rs +++ b/crates/diagnostics/src/lib.rs @@ -7,13 +7,12 @@ pub mod fixes; mod panel; -pub mod spelling; mod validator; -pub use fixes::{Fix, FixProviders}; -pub use panel::ProblemsPanel; +pub use fixes::{Fix, FixProviders, GroupFix, GroupFixProviders, GroupMember, SourceActions}; +pub use panel::{CheckList, ProblemsPanel}; pub use validator::{ - AsyncValidators, ColumnInfo, ColumnSnapshot, ColumnValidator, Misspelling, SpellActions, - Validators, address, + AsyncValidators, CellValue, ColumnFinding, ColumnInfo, ColumnSnapshot, ColumnValidator, + ColumnValues, Misspelling, SpellActions, Validators, address, }; use std::collections::HashMap; @@ -41,6 +40,65 @@ pub struct Location { pub column: Option, } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Scope<'a> { + Dataset, + Column(&'a str), + Row(usize), + Cell { row: usize, column: &'a str }, +} + +impl Location { + pub fn scope(&self) -> Scope<'_> { + match (self.row, self.column.as_deref()) { + (None, None) => Scope::Dataset, + (None, Some(column)) => Scope::Column(column), + (Some(row), None) => Scope::Row(row), + (Some(row), Some(column)) => Scope::Cell { row, column }, + } + } + + pub fn dataset(dataset: impl Into) -> Self { + Self { + dataset: dataset.into(), + row: None, + row_id: None, + column: None, + } + } + + pub fn column(dataset: impl Into, column: impl Into) -> Self { + Self { + column: Some(column.into()), + ..Self::dataset(dataset) + } + } + + pub fn row( + dataset: impl Into, + row: usize, + row_id: Option, + ) -> Self { + Self { + row: Some(row), + row_id, + ..Self::dataset(dataset) + } + } + + pub fn cell( + dataset: impl Into, + row: usize, + row_id: Option, + column: impl Into, + ) -> Self { + Self { + column: Some(column.into()), + ..Self::row(dataset, row, row_id) + } + } +} + /// How loud a problem is. Closed set — a hand-authored mark is just a [`Severity::Note`] from /// [`Source::Note`], so marks and validator output share one list and one colour scale. /// Ordered worst-first so sorting by it floats errors to the top. @@ -77,7 +135,7 @@ pub enum Source { /// A note attached to the data, whether typed here or carried in from the imported /// spreadsheet. Persists, and carries whatever provenance it arrived with — see [`Filed`]. Note, - /// A named rule, validator, plugin, or language server — the string is what the panel shows. + /// A named rule, validator, plugin, or language server — the string is its stable identity. /// Never persisted: computed output is recomputed on open, and stored copies go stale. Validator(SharedString), } @@ -87,13 +145,27 @@ pub enum Source { pub const SOURCE_NOTE: &str = "note"; impl Source { - /// What the panel shows in a diagnostic's source column. - pub fn label(&self) -> SharedString { + /// Stable producer identity used for invalidation, settings, and fix-provider lookup. + pub fn key(&self) -> SharedString { match self { Source::Note => SOURCE_NOTE.into(), Source::Validator(name) => name.clone(), } } + + /// What the panel shows in a diagnostic's source column. + pub fn label(&self) -> SharedString { + match self { + Source::Note => "User notes".into(), + Source::Validator(name) => match name.as_ref() { + "spell" => "Spelling".into(), + "files" => "Missing files".into(), + "date" => "Date format".into(), + "value variants" => "Value variants".into(), + _ => name.clone(), + }, + } + } } #[derive(Clone, PartialEq, Eq, Debug)] @@ -102,11 +174,49 @@ pub struct Diagnostic { pub severity: Severity, pub source: Source, pub message: SharedString, + pub group: Option, /// Who filed this and when, for authored notes. Always `None` on a computed finding — a /// validator's output is recomputed on open, so it has no history to carry. pub filed: Option, } +impl Diagnostic { + /// What an ignore is keyed by: the producer's stable group key, or the message without one. + pub fn ignore_key(&self) -> SharedString { + self.group + .as_ref() + .map_or_else(|| self.message.clone(), |group| group.key.clone()) + } + + /// Whether a column-wide or single-occurrence ignore in `columns` covers this finding. + pub fn is_ignored(&self, columns: &settings::columns::ColumnSettingsMap) -> bool { + let (Source::Validator(source), Some(column)) = + (&self.source, self.location.column.as_deref()) + else { + return false; + }; + let Some(settings) = columns.get(column) else { + return false; + }; + let (source, key) = (source.to_string(), self.ignore_key().to_string()); + self.location.row_id.is_some_and(|id| { + settings + .ignored_occurrences + .contains(&(source.clone(), key.clone(), id)) + }) || settings.ignored_diagnostics.contains(&(source, key)) + } +} + +/// Producer-owned identity; summaries are presentation, never grouping keys. +#[derive(Clone, PartialEq, Eq, Debug)] +pub struct DiagnosticGroup { + pub key: SharedString, + pub summary: SharedString, + /// The exact value this occurrence concerns, for fix menus that must not inspect other values + /// in the same cell. It does not participate in group identity. + pub subject: Option, +} + /// A note's provenance. Free text rather than a parsed date: a catalogue inherits notes from /// whatever kept them before, and refusing to store "March 1998" because it is not ISO-8601 loses /// the note to keep the field tidy. @@ -148,6 +258,10 @@ pub fn severity_color(severity: Severity, cx: &App) -> Hsla { #[derive(Default)] pub struct Diagnostics { items: Vec, + /// Computed findings an ignore covers. Kept apart so every reader of `items` stays unaware. + ignored: Vec, + /// `dataset_main` stable row ids in source order, from the last validation run. + row_ids: Vec, /// `items` indices grouped by row. Publishing happens on an edit; [`Self::at`] runs three /// times per rendered cell per frame, so the whole point is to turn that scan into a lookup /// over one row's handful of entries. Keyed by row alone because `Option` is `Copy` — @@ -165,15 +279,46 @@ impl Global for Diagnostics {} impl Diagnostics { /// Publish `source`'s complete diagnostics for `dataset`, replacing whatever it published /// before (LSP `publishDiagnostics`). A re-run that finds nothing clears its own stale - /// entries, so resolving a problem is just republishing without it. - pub fn set(source: &Source, dataset: &str, items: Vec, cx: &mut App) { + /// entries, so resolving a problem is just republishing without it. Computed notes become + /// warnings because the Notes tab belongs to authored notes. + pub fn set(source: &Source, dataset: &str, mut items: Vec, cx: &mut App) { + let columns = settings::columns::load(cx); let this = cx.default_global::(); - this.items - .retain(|d| &d.source != source || d.location.dataset != dataset); + for diagnostic in &mut items { + if diagnostic.source == Source::Note { + continue; + } + if diagnostic.severity == Severity::Note { + diagnostic.severity = Severity::Warning; + } + if dataset == DATASET_MAIN && diagnostic.location.row_id.is_none() { + diagnostic.location.row_id = diagnostic + .location + .row + .and_then(|row| this.row_ids.get(row).copied()); + } + } + let (ignored, items): (Vec<_>, Vec<_>) = + items.into_iter().partition(|d| d.is_ignored(&columns)); + let stale = |d: &Diagnostic| &d.source == source && d.location.dataset == dataset; + this.items.retain(|d| !stale(d)); this.items.extend(items); + this.ignored.retain(|d| !stale(d)); + this.ignored.extend(ignored); this.reindex(); } + /// Record the stable row ids a validation run addressed, so its findings can carry them. + pub fn set_row_ids(row_ids: &[settings::project::RowId], cx: &mut App) { + cx.default_global::().row_ids = row_ids.to_vec(); + } + + /// Computed findings currently hidden by an ignore, for the panel's "Show ignored" view. + pub fn ignored(cx: &App) -> &[Diagnostic] { + cx.try_global::() + .map_or(&[], |d| d.ignored.as_slice()) + } + fn reindex(&mut self) { self.by_row.clear(); for (ix, diagnostic) in self.items.iter().enumerate() { @@ -303,6 +448,7 @@ impl Diagnostics { severity: Severity::Note, source: Source::Note, message, + group: None, filed, }); this.reindex(); @@ -352,6 +498,7 @@ impl Diagnostics { severity: Severity::Note, source: Source::Note, message, + group: None, filed, }); } @@ -475,6 +622,7 @@ fn load_project_notes(cx: &mut App) { severity: Severity::from_key(&n.severity), source: Source::Note, message: n.message.clone().into(), + group: None, filed: match (&n.created_at, &n.author) { (None, None) => None, (date, author) => Some(Filed { @@ -499,6 +647,10 @@ pub struct DiagnosticHooks { /// Write text back to a location and revalidate. The other half of [`Self::text_at`], and the /// reason a panel row can offer the same corrections a cell does. pub set_text: fn(&Location, SharedString, &mut App), + /// Apply a group resolution as one undoable edit and one validation pass. + pub set_texts: fn(Vec<(Location, SharedString)>, &mut App), + /// Re-run validation after a resolution changes validator settings instead of cell text. + pub revalidate: fn(&mut App), } impl Global for DiagnosticHooks {} @@ -596,6 +748,7 @@ mod tests { severity, source, message: msg.into(), + group: None, filed: None, } } @@ -809,6 +962,7 @@ mod tests { severity: Severity::Error, source: v.clone(), message: "typo".into(), + group: None, filed: None, }], cx, @@ -934,4 +1088,77 @@ mod tests { assert_eq!(Diagnostics::counts(cx), (1, 1)); }); } + + #[gpui::test] + fn computed_notes_are_warnings_so_the_notes_tab_stays_user_owned(cx: &mut TestAppContext) { + cx.update(|cx| { + let source = Source::Validator("plugin".into()); + Diagnostics::set( + &source, + DATASET_MAIN, + vec![diag( + Severity::Note, + source.clone(), + DATASET_MAIN, + "review this", + )], + cx, + ); + assert_eq!(Diagnostics::all(cx)[0].severity, Severity::Warning); + }); + } + + /// A column ignore hides the key throughout its column, an occurrence ignore only its own row, and + /// neither reaches another producer or column. + #[test] + fn ignores_match_by_producer_key_column_and_row() { + let mut finding = diag( + Severity::Warning, + Source::Validator("spell".into()), + DATASET_MAIN, + "misspelled: teh", + ); + finding.location.row_id = Some(7); + let mut columns = settings::columns::ColumnSettingsMap::new(); + assert!(!finding.is_ignored(&columns)); + + let title = columns.entry("Title".into()).or_default(); + title + .ignored_occurrences + .insert(("spell".into(), "misspelled: teh".into(), 8)); + assert!(!finding.is_ignored(&columns), "another row's occurrence"); + columns + .get_mut("Title") + .unwrap() + .ignored_occurrences + .insert(("spell".into(), "misspelled: teh".into(), 7)); + assert!(finding.is_ignored(&columns)); + + let mut other = finding.clone(); + other.source = Source::Validator("files".into()); + assert!(!other.is_ignored(&columns), "another producer"); + other.location.column = Some("Subject".into()); + columns + .entry("Subject".into()) + .or_default() + .ignored_diagnostics + .insert(("files".into(), "misspelled: teh".into())); + assert!(other.is_ignored(&columns), "column-wide, any row"); + } + + #[gpui::test] + fn published_findings_carry_the_stable_row_id(cx: &mut TestAppContext) { + cx.update(|cx| { + Diagnostics::set_row_ids(&[40, 41], cx); + let source = Source::Validator("v".into()); + Diagnostics::set( + &source, + DATASET_MAIN, + vec![diag(Severity::Error, source.clone(), DATASET_MAIN, "bad")], + cx, + ); + assert_eq!(Diagnostics::all(cx)[0].location.row_id, Some(40)); + assert!(Diagnostics::ignored(cx).is_empty()); + }); + } } diff --git a/crates/diagnostics/src/panel.rs b/crates/diagnostics/src/panel.rs index 84829eff..a2ce6ba6 100644 --- a/crates/diagnostics/src/panel.rs +++ b/crates/diagnostics/src/panel.rs @@ -1,14 +1,21 @@ +use std::collections::{BTreeMap, BTreeSet}; use std::rc::Rc; use gpui::prelude::FluentBuilder as _; use gpui::*; use gpui_component::button::{Button, ButtonVariants as _}; +use gpui_component::combobox::{Combobox, ComboboxEvent, ComboboxState}; use gpui_component::dock::{BasePanel, Panel, PanelEvent}; -use gpui_component::menu::{ContextMenuExt as _, DropdownMenu as _, PopupMenu, PopupMenuItem}; +use gpui_component::menu::{ContextMenuExt as _, PopupMenuItem}; +use gpui_component::searchable_list::{SearchableListDelegate, SearchableListItem}; use gpui_component::tab::{Tab, TabBar}; -use gpui_component::{ActiveTheme as _, Icon, IconName, Sizable as _, h_flex, v_flex}; +use gpui_component::{ + ActiveTheme as _, Icon, IconName, IndexPath, Selectable as _, Sizable as _, h_flex, v_flex, +}; -use crate::{DiagnosticHooks, Diagnostics, Location, Severity, severity_color}; +use crate::{ + Diagnostic, DiagnosticHooks, Diagnostics, Location, Scope, Severity, Source, severity_color, +}; /// Which severities the active tab admits. `Info` folds in with `Note`: two quiet buckets is a /// distinction without a difference for someone cataloguing a collection. @@ -33,39 +40,319 @@ impl Filter { } } - fn admits(self, severity: Severity) -> bool { + fn admits(self, diagnostic: &Diagnostic) -> bool { match self { - Filter::All => true, - Filter::Errors => severity == Severity::Error, - Filter::Warnings => severity == Severity::Warning, - Filter::Notes => severity == Severity::Note, + Filter::All => diagnostic.source != Source::Note, + Filter::Errors => { + diagnostic.source != Source::Note && diagnostic.severity == Severity::Error + } + Filter::Warnings => { + diagnostic.source != Source::Note && diagnostic.severity == Severity::Warning + } + Filter::Notes => diagnostic.source == Source::Note, } } } +#[derive(Clone, PartialEq)] +struct SourceItem(SharedString); + +/// The app's multi-select list — the grid filter, this panel's source filter, and the settings +/// pickers — searchable, with the check leading each row the way a menu's does. +pub struct CheckList { + items: Vec, + matched: Vec, +} + +impl CheckList { + pub fn new(items: Vec) -> Self { + Self { + matched: items.clone(), + items, + } + } +} + +impl SearchableListDelegate for CheckList { + type Item = I; + + fn items_count(&self, _: usize) -> usize { + self.matched.len() + } + + fn item(&self, ix: IndexPath) -> Option<&Self::Item> { + self.matched.get(ix.row) + } + + fn position(&self, value: &V) -> Option + where + Self::Item: SearchableListItem, + V: PartialEq, + { + self.matched + .iter() + .position(|item| item.value() == value) + .map(IndexPath::new) + } + + fn perform_search(&mut self, query: &str, _: &mut Window, _: &mut App) -> Task<()> { + self.matched = self + .items + .iter() + .filter(|item| item.matches(query)) + .cloned() + .collect(); + Task::ready(()) + } + + fn render_item( + &self, + _: IndexPath, + item: &Self::Item, + checked: bool, + window: &mut Window, + cx: &mut App, + ) -> Option { + Some( + h_flex() + .w_full() + .items_start() + .gap_1() + .child( + Icon::new(IconName::Check) + .xsmall() + .mt(px(2.)) + .when(!checked, |icon| icon.invisible()), + ) + .child( + div() + .flex_1() + .min_w_0() + .h_6() + .overflow_hidden() + .child(item.render(window, cx)), + ) + .into_any_element(), + ) + } +} + +impl SearchableListItem for SourceItem { + type Value = SharedString; + + fn title(&self) -> SharedString { + self.0.clone() + } + + fn value(&self) -> &Self::Value { + &self.0 + } + + fn render(&self, _: &mut Window, _: &mut App) -> impl IntoElement { + div().child(self.0.clone()) + } +} + +struct SourceFilterSub(#[allow(dead_code)] Subscription); + +#[derive(Clone)] +struct RowMember { + location: Location, + message: SharedString, + subject: Option, +} + /// One list entry, resolved out of the store once per change rather than once per frame. Colours /// are not cached here because they come from the theme, which can change without the store doing. struct Row { icon: IconName, severity: Severity, - /// Which cell, row, or column this points at, already phrased for display. - scope: SharedString, - /// A row- or column-wide note, which reads differently from a cell coordinate. - wide: bool, + /// The row and column cells, each empty when the finding does not narrow to one. + row_label: SharedString, + column: SharedString, message: SharedString, source: SharedString, + source_key: SharedString, + finding: Diagnostic, + ignored: bool, location: Location, + group: Option, + count: usize, + members: Vec, + depth: usize, +} + +fn group_id(d: &Diagnostic) -> Option { + if d.source == Source::Note { + return None; + } + d.group.as_ref().map(|group| { + format!( + "{:?}", + (&d.location.dataset, &d.source, d.severity, &group.key) + ) + }) +} + +/// Joins a group id to one observed form. `group_id` is `Debug` output, which escapes it. +const FORM_SEPARATOR: char = '\u{1f}'; + +fn subject(d: &Diagnostic) -> Option { + d.group.as_ref().and_then(|group| group.subject.clone()) +} + +fn occurrence(d: &Diagnostic, depth: usize) -> Row { + let (row_label, column) = match d.location.scope() { + Scope::Cell { row, column } => { + (format!("Row {}", row + 1).into(), column.to_owned().into()) + } + Scope::Row(row) => (format!("Row {}", row + 1).into(), SharedString::default()), + Scope::Column(column) => (SharedString::default(), column.to_owned().into()), + Scope::Dataset => (SharedString::default(), d.location.dataset.clone()), + }; + Row { + icon: match d.severity { + Severity::Error => IconName::CircleX, + Severity::Warning => IconName::TriangleAlert, + Severity::Note => IconName::Info, + }, + severity: d.severity, + row_label, + column, + message: d.message.clone(), + source: match d.source { + Source::Note => SharedString::default(), + _ => d.source.label(), + }, + source_key: d.source.key(), + finding: d.clone(), + ignored: false, + location: d.location.clone(), + group: None, + count: 1, + members: Vec::new(), + depth, + } +} + +/// A group header over `found`, or the lone finding itself. Each observed form nests one level +/// down by the same rule, so a form seen once is a plain row. +fn push_group( + rows: &mut Vec, + found: &[&Diagnostic], + cluster: &[RowMember], + id: String, + message: SharedString, + depth: usize, + expanded: &BTreeSet, +) { + if let [single] = found { + rows.push(occurrence(single, depth)); + return; + } + let mut columns: Vec<_> = found.iter().map(|d| &d.location.column).collect(); + columns.sort(); + columns.dedup(); + let open = expanded.contains(&id); + rows.push(Row { + column: match columns.as_slice() { + [Some(column)] => column.clone(), + _ => format!("{} columns", columns.len()).into(), + }, + row_label: SharedString::default(), + message, + group: Some(id.clone()), + count: found.len(), + members: cluster.to_vec(), + ..occurrence(found[0], depth) + }); + if !open { + return; + } + let forms: Vec<_> = found.chunk_by(|a, b| subject(a) == subject(b)).collect(); + for form in &forms { + if forms.len() == 1 { + rows.extend(form.iter().map(|d| occurrence(d, depth + 1))); + } else { + let form_id = format!("{id}{FORM_SEPARATOR}{:?}", subject(form[0])); + let message = form[0].message.clone(); + push_group(rows, form, cluster, form_id, message, depth + 1, expanded); + } + } +} + +/// Atomic findings stay in the store; only this flat, virtualized view collapses them. +fn project(mut items: Vec<&Diagnostic>, expanded: &BTreeSet) -> Vec { + items.sort_by(|a, b| { + ( + a.severity, + &a.location.dataset, + a.location.row, + &a.location.column, + ) + .cmp(&( + b.severity, + &b.location.dataset, + b.location.row, + &b.location.column, + )) + }); + let mut groups: BTreeMap> = BTreeMap::new(); + for d in &items { + if let Some(id) = group_id(d) { + groups.entry(id).or_default().push(d); + } + } + let mut emitted = BTreeSet::new(); + items.retain(|d| group_id(d).is_none_or(|id| emitted.insert(id))); + items.sort_by_cached_key(|d| { + ( + d.severity, + if group_id(d).is_some() { + d.group.as_ref().expect("group metadata").summary.clone() + } else { + d.message.clone() + }, + group_id(d), + ) + }); + let mut rows = Vec::new(); + for d in items { + let Some(id) = group_id(d) else { + rows.push(occurrence(d, 0)); + continue; + }; + let Some(mut members) = groups.remove(&id) else { + continue; + }; + members.sort_by_cached_key(|d| { + ( + subject(d), + d.message.clone(), + d.location.row, + d.location.column.clone(), + ) + }); + let cluster: Vec<_> = members + .iter() + .map(|member| RowMember { + location: member.location.clone(), + message: member.message.clone(), + subject: subject(member), + }) + .collect(); + let summary = d.group.as_ref().expect("group metadata").summary.clone(); + push_group(&mut rows, &members, &cluster, id, summary, 0, expanded); + } + rows } /// Bottom dock: every open problem, filtered by severity and source, click to jump to it. pub struct ProblemsPanel { focus_handle: FocusHandle, filter: Filter, - /// Which producer's findings to show, or all of them. Severity says how loud a problem is; - /// this says who is talking — with a speller, a files check, an authority, and any number of - /// plugins all publishing into one list, "only show me the spelling" is the difference - /// between a list and a pile. - source: Option, + /// Sources unchecked in the multi-select filter. Empty means all diagnostic sources. + excluded_sources: BTreeSet, /// Distinct sources currently publishing, in display order. Only what is actually here, so /// the menu never offers a filter that would empty the list. sources: Rc>, @@ -75,6 +362,9 @@ pub struct ProblemsPanel { rows: Rc>, /// Per-tab totals, which count every severity regardless of the active filter. counts: [usize; 4], + expanded: BTreeSet, + show_ignored: bool, + ignored_count: usize, /// Refreshes on any store change. One `observe_global` and no re-binding — unlike /// `TableStateHandle`, the `Diagnostics` global is plain data that is never rebuilt. _sub: Subscription, @@ -85,10 +375,13 @@ impl ProblemsPanel { let mut this = Self { focus_handle: cx.focus_handle(), filter: Filter::All, - source: None, + excluded_sources: BTreeSet::new(), sources: Rc::default(), rows: Rc::default(), counts: [0; 4], + expanded: BTreeSet::new(), + show_ignored: false, + ignored_count: 0, _sub: cx.observe_global::(|this, cx| { this.refresh(cx); cx.notify(); @@ -102,105 +395,126 @@ impl ProblemsPanel { /// and the tab counts have to agree — a tab reading "Errors (12)" over a list of two is worse /// than no count at all. fn admits_source(&self, d: &crate::Diagnostic) -> bool { - self.source - .as_ref() - .is_none_or(|wanted| &d.source.label() == wanted) - } - - /// The "which producer" dropdown. Hidden until something is publishing, so a project with - /// only spelling findings doesn't grow a menu with one entry in it. - fn source_menu(&self, cx: &mut Context) -> impl IntoElement { - let (sources, selected, panel) = (self.sources.clone(), self.source.clone(), cx.entity()); - let label = selected - .clone() - .unwrap_or_else(|| SharedString::from("All sources")); - - div().pr_1().when(sources.len() > 1, |el| { - el.child( - Button::new("problems-source") - .ghost() - .small() - .label(label) - .dropdown_menu(move |menu, _window, _cx| { - let pick = - |menu: PopupMenu, - name: Option, - panel: &Entity| { - let (panel, chosen) = (panel.clone(), name.clone()); - menu.item( - PopupMenuItem::new( - name.unwrap_or_else(|| SharedString::from("All sources")), - ) - .on_click( - move |_, _, cx| { - panel.update(cx, |this, cx| { - this.source = chosen.clone(); - this.refresh(cx); - cx.notify(); - }); - }, - ), - ) - }; - let menu = pick(menu, None, &panel).separator(); - sources - .iter() - .fold(menu, |menu, name| pick(menu, Some(name.clone()), &panel)) - }), + !self.excluded_sources.contains(&d.source.label()) + } + + fn source_menu(&self, window: &mut Window, cx: &mut Context) -> impl IntoElement { + let sources = self.sources.clone(); + let excluded = self.excluded_sources.clone(); + let panel = cx.entity(); + let state = window.use_keyed_state("problems-source-filter", cx, |window, cx| { + ComboboxState::new( + CheckList::::new(Vec::new()), + Vec::new(), + window, + cx, ) + .multiple(true) + .searchable(false) + }); + window.use_keyed_state("problems-source-sub", cx, |_window, cx| { + SourceFilterSub(cx.subscribe(&state, move |_, _, event, cx| { + let ComboboxEvent::Change(kept) = event else { + return; + }; + panel.update(cx, |this, cx| { + this.excluded_sources = this + .sources + .iter() + .filter(|source| !kept.contains(source)) + .cloned() + .collect(); + this.refresh(cx); + cx.notify(); + }); + })) + }); + let cached = window.use_keyed_state("problems-source-items", cx, |_, _| Vec::new()); + if cached.read(cx).as_slice() != sources.as_slice() { + state.update(cx, |state, cx| { + state.set_items( + CheckList::new(sources.iter().cloned().map(SourceItem).collect()), + window, + cx, + ); + }); + cached.update(cx, |cached, _| *cached = sources.to_vec()); + } + let kept: Vec<_> = sources + .iter() + .filter(|source| !excluded.contains(*source)) + .cloned() + .collect(); + let selected = state.read(cx).selected_values(); + if selected.len() != kept.len() || !kept.iter().all(|source| selected.contains(source)) { + state.update(cx, |state, cx| { + state.set_selected_indices( + sources + .iter() + .enumerate() + .filter_map(|(ix, source)| { + (!excluded.contains(source)).then_some(IndexPath::new(ix)) + }) + .collect::>(), + window, + cx, + ); + }); + } + div().w_48().pr_1().when(sources.len() > 1, |element| { + element.child(Combobox::new(&state).placeholder("Filter sources").small()) }) } fn refresh(&mut self, cx: &App) { + let started = std::time::Instant::now(); let mut sources: Vec = Diagnostics::all(cx) .iter() + .filter(|d| d.source != Source::Note) .map(|d| d.source.label()) .collect(); sources.sort(); sources.dedup(); + self.excluded_sources + .retain(|source| sources.contains(source)); self.sources = Rc::new(sources); self.counts = Filter::ALL.map(|f| { Diagnostics::all(cx) .iter() - .filter(|d| f.admits(d.severity) && self.admits_source(d)) + .filter(|d| f.admits(d) && self.admits_source(d)) .count() }); - let mut rows: Vec = Diagnostics::all(cx) + let ignored = Diagnostics::ignored(cx); + self.ignored_count = ignored.len(); + self.show_ignored &= self.ignored_count > 0; + let live: BTreeSet<_> = Diagnostics::all(cx) .iter() - .filter(|d| self.filter.admits(d.severity) && self.admits_source(d)) - .map(|d| { - let (scope, wide) = match (d.location.row, d.location.column.as_ref()) { - (Some(r), Some(c)) => (format!("Row {} · {c}", r + 1).into(), false), - (Some(r), None) => (format!("Row {}", r + 1).into(), true), - (None, Some(c)) => (c.clone(), true), - (None, None) => (d.location.dataset.clone(), true), - }; - Row { - severity: d.severity, - icon: match d.severity { - Severity::Error => IconName::CircleX, - Severity::Warning => IconName::TriangleAlert, - Severity::Note => IconName::Info, - }, - scope, - wide, - message: d.message.clone(), - source: d.source.label(), - location: d.location.clone(), - } - }) + .chain(ignored) + .filter_map(group_id) .collect(); - // Errors first, then reading order down the table. A whole-row/column note sorts ahead - // of the cells it covers because `None` orders before `Some`. - rows.sort_by(|a, b| { - (a.severity, &a.location.row, &a.location.column).cmp(&( - b.severity, - &b.location.row, - &b.location.column, - )) - }); + self.expanded + .retain(|id| live.contains(id.split(FORM_SEPARATOR).next().unwrap_or_default())); + let admitted = |d: &&Diagnostic| self.filter.admits(d) && self.admits_source(d); + let visible = Diagnostics::all(cx).iter().filter(admitted).collect(); + let mut rows = project(visible, &self.expanded); + if self.show_ignored { + rows.extend( + project(ignored.iter().filter(admitted).collect(), &self.expanded) + .into_iter() + .map(|row| Row { + ignored: true, + ..row + }), + ); + } + log::debug!( + "problems panel: {} rows from {} findings in {:?}", + rows.len(), + Diagnostics::all(cx).len(), + started.elapsed() + ); self.rows = Rc::new(rows); } } @@ -235,12 +549,15 @@ impl Panel for ProblemsPanel { } impl Render for ProblemsPanel { - fn render(&mut self, _w: &mut Window, cx: &mut Context) -> impl IntoElement { + fn render(&mut self, window: &mut Window, cx: &mut Context) -> impl IntoElement { let rows = self.rows.clone(); + let panel_handle = cx.entity(); + let expanded = self.expanded.clone(); let muted = cx.theme().muted_foreground; v_flex() .size_full() + .tab_group() // The dock focuses this panel's handle when its tab is clicked. Without an element // tracking it, focus lands nowhere: the dispatch path collapses to the window root and // the window-wide shortcuts stop reaching their handlers. @@ -275,7 +592,28 @@ impl Render for ProblemsPanel { cx.notify(); })), ) - .child(self.source_menu(cx)), + .when(self.filter != Filter::Notes, |bar| { + bar.child( + h_flex() + .items_center() + .gap_1() + .when(self.ignored_count > 0, |bar| { + bar.child( + Button::new("problems-show-ignored") + .ghost() + .small() + .selected(self.show_ignored) + .label(format!("Show ignored ({})", self.ignored_count)) + .on_click(cx.listener(|this, _, _, cx| { + this.show_ignored = !this.show_ignored; + this.refresh(cx); + cx.notify(); + })), + ) + }) + .child(self.source_menu(window, cx)), + ) + }), ) .when(rows.is_empty(), |panel| { panel.child(div().p_3().text_color(muted).child("No problems")) @@ -289,33 +627,107 @@ impl Render for ProblemsPanel { let (hover_bg, chip_bg) = (cx.theme().secondary_hover, cx.theme().secondary); let muted = cx.theme().muted_foreground; + let (guide, nested_bg) = + (cx.theme().border, cx.theme().muted.opacity(0.35)); rows[range.clone()] .iter() .zip(range) .map(|(r, ix)| { let location = r.location.clone(); + let group = r.group.clone(); + let group_members = r.members.clone(); + let source_key = r.source_key.clone(); + let finding = r.finding.clone(); + let ignored = r.ignored; + let is_group = group.is_some(); + let is_expanded = + group.as_ref().is_some_and(|id| expanded.contains(id)); + let panel_handle = panel_handle.clone(); h_flex() .id(ix) .w_full() + .h_8() + .items_center() .gap_2() .px_2() - .py_1() + .when(r.depth > 0, |row| row.bg(nested_bg)) .cursor_pointer() + .when(ignored, |row| row.opacity(0.6)) .hover(|row| row.bg(hover_bg)) .child( Icon::new(r.icon.clone()) .small() .text_color(severity_color(r.severity, cx)), ) - // A row- or column-wide note gets a chip so it doesn't read - // as a cell coordinate that just lost half its address. + .child( + h_flex() + .w(px(104.)) + .flex_shrink_0() + .h_full() + .children((0..r.depth).map(|_| { + div() + .w(px(12.)) + .h_full() + .border_l_1() + .border_color(guide) + })) + .text_sm() + .text_color(muted) + .when(!is_group, |cell| cell.child(r.row_label.clone())) + .when_some(r.group.clone(), |cell, id| { + let panel_handle = panel_handle.clone(); + cell.child( + Button::new(("expand-problem", ix)) + .text() + .small() + .icon(if is_expanded { + IconName::ChevronDown + } else { + IconName::ChevronRight + }) + .label(format!("({})", r.count)) + .accessibility_label(format!( + "{} {}", + if is_expanded { + "Collapse" + } else { + "Expand" + }, + r.message + )) + .on_click(move |_, _, cx| { + cx.stop_propagation(); + panel_handle.update(cx, |this, cx| { + if !this.expanded.remove(&id) { + this.expanded + .insert(id.clone()); + } + this.refresh(cx); + cx.notify(); + }); + }), + ) + }), + ) .child( div() + .w(px(160.)) .flex_shrink_0() .text_sm() .text_color(muted) - .when(r.wide, |s| s.px_1().rounded_sm().bg(chip_bg)) - .child(r.scope.clone()), + .overflow_hidden() + .text_ellipsis() + .child(r.column.clone()) + .id(("problem-column", ix)) + .tooltip({ + let text = r.column.clone(); + move |window, cx| { + gpui_component::tooltip::Tooltip::new( + text.clone(), + ) + .build(window, cx) + } + }), ) .child( div() @@ -324,8 +736,31 @@ impl Render for ProblemsPanel { .text_sm() .overflow_hidden() .text_ellipsis() - .child(r.message.clone()), + .child(r.message.clone()) + // ponytail: always offered, since gpui cannot tell us whether the text was cut + .id(("problem-message", ix)) + .tooltip({ + let text = r.message.clone(); + move |window, cx| { + gpui_component::tooltip::Tooltip::new( + text.clone(), + ) + .build(window, cx) + } + }), ) + .when(ignored, |row| { + row.child( + div() + .flex_shrink_0() + .px_1() + .rounded_sm() + .bg(chip_bg) + .text_xs() + .text_color(muted) + .child("Ignored"), + ) + }) .child( div() .flex_shrink_0() @@ -336,6 +771,16 @@ impl Render for ProblemsPanel { .on_click({ let location = location.clone(); move |_, _, cx| { + if let Some(id) = &group { + panel_handle.update(cx, |this, cx| { + if !this.expanded.remove(id) { + this.expanded.insert(id.clone()); + } + this.refresh(cx); + cx.notify(); + }); + return; + } if let Some(hooks) = cx.try_global::().copied() { @@ -352,29 +797,119 @@ impl Render for ProblemsPanel { else { return menu; }; - let Some(text) = (hooks.text_at)(&location, cx) else { - return menu; + let findings: Vec = if is_group { + group_members + .iter() + .map(|member| Diagnostic { + location: member.location.clone(), + message: member.message.clone(), + ..finding.clone() + }) + .collect() + } else { + vec![finding.clone()] }; - let menu = { - let location = location.clone(); - crate::spelling::menu( - &text, + if ignored { + return menu.item( + PopupMenuItem::new("Unignore").on_click( + move |_, _, cx| { + crate::fixes::unignore(&findings, cx) + }, + ), + ); + } + if !is_group { + let text = + matches!(location.scope(), Scope::Cell { .. }) + .then(|| (hooks.text_at)(&location, cx)) + .flatten(); + let applied = location.clone(); + return crate::fixes::finding_menu( + &finding, + text.as_ref(), menu, - window, cx, move |fixed, cx| { - (hooks.set_text)(&location, fixed, cx) + (hooks.set_text)(&applied, fixed, cx) }, + ); + } + let menu = { + let members = group_members + .iter() + .filter_map(|member| { + Some(crate::GroupMember { + location: member.location.clone(), + text: (hooks.text_at)( + &member.location, + cx, + )?, + message: member.message.clone(), + subject: member.subject.clone(), + }) + }) + .collect::>(); + crate::fixes::group_menu( + &source_key, + &members, + menu, + window, + cx, ) }; - let applied = location.clone(); - crate::fixes::menu( - &location, - &text, - menu, + // Value variants already persist "these are distinct", the precise form of ignore. + if source_key == "value variants" { + return menu; + } + let mut columns: BTreeMap> = + BTreeMap::new(); + for member in findings { + if let Some(column) = member.location.column.clone() { + columns.entry(column).or_default().push(member); + } + } + let firsts: Vec = columns + .values() + .map(|found| found[0].clone()) + .collect(); + let total: usize = columns.values().map(Vec::len).sum(); + let menu = menu.item( + PopupMenuItem::new(format!("Ignore all ({total})")) + .on_click(move |_, window, cx| { + crate::fixes::ignore(&firsts, false, window, cx) + }), + ); + if columns.len() < 2 { + return menu; + } + menu.submenu( + "Ignore in column", window, cx, - move |fixed, cx| (hooks.set_text)(&applied, fixed, cx), + move |sub, _, _| { + columns.iter().fold( + sub.max_w(px(360.)), + |sub, (column, members)| { + let members = members.clone(); + sub.item( + PopupMenuItem::new( + crate::fixes::menu_label(&format!( + "“{column}” ({})", + members.len() + )), + ) + .on_click(move |_, window, cx| { + crate::fixes::ignore( + &members[..1], + false, + window, + cx, + ) + }), + ) + }, + ) + }, ) }) }) @@ -394,6 +929,239 @@ mod tests { use crate::{DATASET_MAIN, Diagnostic, Diagnostics, Location, ProblemsPanel, Severity, Source}; use gpui::TestAppContext; + fn repeated() -> Vec { + (0..100) + .map(|row| Diagnostic { + location: Location::cell(DATASET_MAIN, row, None, "Title"), + severity: Severity::Warning, + source: Source::Validator("spell".into()), + message: "misspelled: recieve".into(), + group: Some(crate::DiagnosticGroup { + key: "en:recieve".into(), + summary: "misspelled: recieve".into(), + subject: Some("recieve".into()), + }), + filed: None, + }) + .collect() + } + + #[test] + fn groups_are_explicit_scoped_and_expand_to_exact_locations() { + let mut items = repeated(); + let closed = super::project(items.iter().collect(), &Default::default()); + assert_eq!(closed.len(), 1); + assert_eq!((closed[0].count, closed[0].column.as_ref()), (100, "Title")); + let expanded = [closed[0].group.clone().unwrap()].into_iter().collect(); + let open = super::project(items.iter().collect(), &expanded); + assert_eq!(open.len(), 101); + for (row, occurrence) in open[1..].iter().enumerate() { + assert_eq!(occurrence.location.row, Some(row)); + assert_eq!(occurrence.depth, 1); + assert!(occurrence.group.is_none()); + } + items[0].severity = Severity::Error; + items[1].source = Source::Validator("other".into()); + items[2].location.dataset = "other".into(); + items[3].group = None; + items[4].source = Source::Note; + assert_eq!( + super::project(items.iter().collect(), &Default::default()).len(), + 6 + ); + } + + #[test] + fn one_occurrence_is_a_plain_finding() { + let items = repeated(); + let rows = super::project(vec![&items[0]], &Default::default()); + assert_eq!(rows.len(), 1); + assert!(rows[0].group.is_none()); + assert_eq!( + (rows[0].row_label.as_ref(), rows[0].column.as_ref()), + ("Row 1", "Title") + ); + } + + #[test] + fn expanded_variants_nest_forms_and_a_single_form_is_a_plain_row() { + let mut items = repeated(); + items.truncate(5); + for (row, item) in items.iter_mut().enumerate() { + let form = if row < 3 { + "Akbar, Mohamed" + } else if row == 3 { + "Akbar, Mohammed" + } else { + "Akbar, Muhammad" + }; + item.message = form.into(); + item.group.as_mut().unwrap().subject = Some(form.into()); + } + items[4].location.column = Some("Creator".into()); + let closed = super::project(items.iter().collect(), &Default::default()); + assert_eq!( + (closed[0].count, closed[0].column.as_ref()), + (5, "2 columns") + ); + let group = closed[0].group.clone().unwrap(); + let summarize = |rows: &[super::Row]| { + rows.iter() + .map(|row| { + ( + row.depth, + row.group.is_some(), + row.count, + row.row_label.to_string(), + row.message.to_string(), + ) + }) + .collect::>() + }; + let open = super::project(items.iter().collect(), &[group.clone()].into()); + assert_eq!( + summarize(&open[1..]), + [ + (1, true, 3, "".into(), "Akbar, Mohamed".into()), + (1, false, 1, "Row 4".into(), "Akbar, Mohammed".into()), + (1, false, 1, "Row 5".into(), "Akbar, Muhammad".into()), + ] + ); + assert_eq!( + open[1].members.len(), + 5, + "a form still resolves the whole cluster" + ); + let form = open[1].group.clone().unwrap(); + let nested = super::project(items.iter().collect(), &[group, form].into()); + assert_eq!( + summarize(&nested[2..5]), + [ + (2, false, 1, "Row 1".into(), "Akbar, Mohamed".into()), + (2, false, 1, "Row 2".into(), "Akbar, Mohamed".into()), + (2, false, 1, "Row 3".into(), "Akbar, Mohamed".into()), + ] + ); + } + + #[gpui::test] + fn clicking_a_group_expands_without_navigation(cx: &mut TestAppContext) { + #[derive(Default)] + struct Revealed(Vec); + impl gpui::Global for Revealed {} + cx.update(|cx| { + gpui_component::init(cx); + cx.set_global(Revealed::default()); + cx.set_global(crate::DiagnosticHooks { + reveal: |location, cx| { + use gpui::BorrowAppContext as _; + cx.update_global::(|seen, _| seen.0.push(location.clone())); + }, + text_at: |_, _| None, + set_text: |_, _, _| panic!("navigation must not edit"), + set_texts: |_, _| panic!("navigation must not edit"), + revalidate: |_| panic!("navigation must not revalidate"), + }); + Diagnostics::set( + &Source::Validator("spell".into()), + DATASET_MAIN, + repeated(), + cx, + ); + }); + let (panel, cx) = cx.add_window_view(ProblemsPanel::new); + cx.run_until_parked(); + cx.update(|window, cx| { + _ = window.draw(cx); + }); + cx.simulate_click( + gpui::point(gpui::px(400.), gpui::px(48.)), + Default::default(), + ); + cx.run_until_parked(); + panel.read_with(cx, |panel, cx| { + assert_eq!(panel.rows.len(), 101); + assert!(cx.global::().0.is_empty()); + }); + cx.update(|window, cx| { + _ = window.draw(cx); + }); + cx.simulate_click( + gpui::point(gpui::px(400.), gpui::px(80.)), + Default::default(), + ); + cx.run_until_parked(); + panel.read_with(cx, |_, cx| { + assert_eq!( + cx.global::().0, + [Location::cell(DATASET_MAIN, 0, None, "Title")] + ); + }); + cx.simulate_click( + gpui::point(gpui::px(100.), gpui::px(48.)), + Default::default(), + ); + cx.run_until_parked(); + panel.read_with(cx, |panel, _| assert_eq!(panel.rows.len(), 1)); + let focus = panel.read_with(cx, |panel, _| panel.focus_handle.clone()); + cx.update(|window, cx| { + _ = window.draw(cx); + window.focus(&focus, cx); + window.focus_prev(cx); + _ = window.draw(cx); + }); + let keystroke = gpui::Keystroke::parse("enter").unwrap(); + cx.simulate_event(gpui::KeyDownEvent { + keystroke: keystroke.clone(), + is_held: false, + prefer_character_input: false, + }); + cx.simulate_event(gpui::KeyUpEvent { keystroke }); + cx.run_until_parked(); + panel.read_with(cx, |panel, cx| { + assert_eq!( + panel.rows.len(), + 101, + "the focused expansion button supports Enter" + ); + assert_eq!(cx.global::().0.len(), 1); + }); + } + + #[gpui::test] + fn group_refresh_preserves_counts_and_prunes_expansion(cx: &mut TestAppContext) { + cx.update(|cx| { + gpui_component::init(cx); + Diagnostics::set( + &Source::Validator("spell".into()), + DATASET_MAIN, + repeated(), + cx, + ); + }); + let (panel, cx) = cx.add_window_view(ProblemsPanel::new); + panel.update(cx, |this, cx| { + assert_eq!(this.counts, [100, 0, 100, 0]); + assert_eq!(this.rows.len(), 1); + this.expanded.insert(this.rows[0].group.clone().unwrap()); + this.refresh(cx); + assert_eq!(this.rows.len(), 101); + this.filter = Filter::Errors; + this.refresh(cx); + assert!(this.rows.is_empty()); + assert_eq!(this.counts[2], 100); + assert_eq!(this.expanded.len(), 1); + Diagnostics::set( + &Source::Validator("spell".into()), + DATASET_MAIN, + Vec::new(), + cx, + ); + this.refresh(cx); + assert!(this.expanded.is_empty()); + }); + } + #[gpui::test] fn renders_the_empty_state_without_a_store(cx: &mut TestAppContext) { cx.update(gpui_component::init); @@ -429,6 +1197,7 @@ mod tests { severity, source: Source::Note, message: "m".into(), + group: None, filed: None, }) .collect(), @@ -439,16 +1208,42 @@ mod tests { } #[test] - fn tabs_partition_every_severity() { + fn notes_are_separate_from_computed_diagnostics() { for severity in [Severity::Error, Severity::Warning, Severity::Note] { - let admitting: Vec<_> = Filter::ALL - .iter() - .filter(|f| f.admits(severity)) - .map(|f| f.label()) - .collect(); - // Always "All" plus exactly one specific tab — nothing is unreachable or double-listed. - assert_eq!(admitting.len(), 2, "{severity:?} lands in {admitting:?}"); + let computed = Diagnostic { + location: Location::dataset(DATASET_MAIN), + severity, + source: Source::Validator("test".into()), + message: "computed".into(), + group: None, + filed: None, + }; + assert!(Filter::All.admits(&computed)); + assert!(!Filter::Notes.admits(&computed)); + + let note = Diagnostic { + source: Source::Note, + ..computed + }; + assert!(!Filter::All.admits(¬e)); + assert!(Filter::Notes.admits(¬e)); + } + } + + #[test] + fn source_labels_are_descriptive_and_notes_are_not_diagnostic_sources() { + for (key, label) in [ + ("spell", "Spelling"), + ("files", "Missing files"), + ("date", "Date format"), + ("value variants", "Value variants"), + ] { + let source = Source::Validator(key.into()); + assert_eq!(source.key(), key); + assert_eq!(source.label(), label); } + assert_eq!(Source::Note.key(), "note"); + assert_eq!(Source::Note.label(), "User notes"); } /// Picking a source narrows the list *and* the tab counts: a tab reading "Errors (2)" over a @@ -473,26 +1268,42 @@ mod tests { severity: Severity::Error, source: Source::Validator(source.into()), message: "m".into(), + group: None, filed: None, }) .collect(), cx, ); } + Diagnostics::set( + &Source::Note, + DATASET_MAIN, + vec![Diagnostic { + location: at(3), + severity: Severity::Note, + source: Source::Note, + message: "user note".into(), + group: None, + filed: None, + }], + cx, + ); }); let (panel, cx) = cx.add_window_view(ProblemsPanel::new); panel.update(cx, |this, cx| { assert_eq!(this.sources.len(), 2, "both producers are offered"); assert_eq!(this.rows.len(), 3); - assert_eq!(this.counts[0], 3, "the All tab counts everything"); + assert_eq!(this.counts[0], 3, "the All tab excludes user notes"); + assert_eq!(this.counts[3], 1, "the Notes tab counts user notes"); - this.source = Some("files".into()); + this.excluded_sources.insert("Spelling".into()); this.refresh(cx); - assert_eq!(this.rows.len(), 1, "only the files finding survives"); + assert_eq!(this.rows.len(), 1, "the files finding survives"); assert_eq!(this.counts[0], 1, "and the tab count agrees"); + assert_eq!(this.counts[3], 1, "source filters do not hide user notes"); - this.source = None; + this.excluded_sources.clear(); this.refresh(cx); assert_eq!(this.rows.len(), 3, "clearing the filter brings them back"); }); diff --git a/crates/diagnostics/src/spelling.rs b/crates/diagnostics/src/spelling.rs deleted file mode 100644 index fbbbada5..00000000 --- a/crates/diagnostics/src/spelling.rs +++ /dev/null @@ -1,58 +0,0 @@ -//! The "did you mean" menu, shared by everything that can show one. -//! -//! Lives here rather than in `table` because the Problems panel offers the same fixes as a cell -//! does, and two copies of a menu is two places to fix it. Neither caller links a dictionary — -//! the words come from [`SpellActions`], and what to do with a correction is the caller's closure. - -use gpui::{App, Context, SharedString, Window}; -use gpui_component::menu::{PopupMenu, PopupMenuItem}; - -use crate::SpellActions; - -/// Add a `Spelling` submenu to `menu` for the misspellings in `text`, or return `menu` untouched -/// when spell checking is off or the text is clean. -/// -/// `apply` receives the whole corrected text rather than the word pair: replacing the word is the -/// same everywhere, so doing it here leaves each caller with nothing but its own way of storing a -/// string. Every occurrence is replaced, because a word misspelled twice is misspelled twice and -/// a menu item carries no offset. -pub fn menu( - text: &SharedString, - menu: PopupMenu, - window: &mut Window, - cx: &mut Context, - apply: impl Fn(SharedString, &mut App) + Clone + 'static, -) -> PopupMenu { - let Some(actions) = cx.try_global::().copied() else { - return menu; - }; - let found = (actions.suggest)(text, cx); - if found.is_empty() { - return menu; - } - let text = text.clone(); - - menu.submenu("Spelling", window, cx, move |sub, _window, _cx| { - let sub = found.iter().fold(sub, |sub, (word, suggestions)| { - suggestions.iter().fold(sub, |sub, suggestion| { - let (apply, text) = (apply.clone(), text.clone()); - let (word, suggestion) = (word.clone(), suggestion.clone()); - sub.item( - PopupMenuItem::new(format!("{word} → {suggestion}")).on_click( - move |_, _, cx| { - let fixed = text.replace(word.as_ref(), suggestion.as_ref()); - apply(fixed.into(), cx); - }, - ), - ) - }) - }); - found.iter().fold(sub.separator(), |sub, (word, _)| { - let word = word.clone(); - sub.item( - PopupMenuItem::new(format!("Add “{word}” to dictionary")) - .on_click(move |_, _, cx| (actions.add_word)(&word, cx)), - ) - }) - }) -} diff --git a/crates/diagnostics/src/validator.rs b/crates/diagnostics/src/validator.rs index a4ad421a..a1943674 100644 --- a/crates/diagnostics/src/validator.rs +++ b/crates/diagnostics/src/validator.rs @@ -6,17 +6,20 @@ //! whatever it published before. Neither offers "add one diagnostic", and neither does //! [`Validators::run`] — a re-run is the only invalidation there is. //! -//! A validator never builds a [`Location`] or a [`Source`]. It reports `(row, severity, message)` +//! A column validator never builds a [`Location`] or a [`Source`]. It reports [`ColumnFinding`] values //! against one column and the registry addresses it, the same split as an LSP server reporting //! ranges while the client owns the URI. That is what lets a validator live in its own crate //! knowing nothing about datasets, projects, or the table. use std::collections::BTreeMap; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; -use gpui::{App, BorrowAppContext as _, Global, SharedString}; +use gpui::{App, AppContext as _, Global, SharedString}; use settings::columns::ColumnSettings; -use crate::{DATASET_MAIN, Diagnostic, Diagnostics, Location, Severity, Source}; +use crate::{DATASET_MAIN, Diagnostic, DiagnosticGroup, Diagnostics, Location, Severity, Source}; /// One column's whole input, owned. A validator that runs later — off the UI thread, after this /// run's borrows are gone — needs the data to outlive the call, which [`ColumnInfo`] cannot do. @@ -26,6 +29,8 @@ pub struct ColumnSnapshot { pub data_type: SharedString, pub settings: ColumnSettings, pub values: Vec, + pub subdelimiter: SharedString, + pub row_ids: std::sync::Arc<[settings::project::RowId]>, } impl ColumnSnapshot { @@ -49,23 +54,135 @@ pub struct ColumnInfo<'a> { pub settings: &'a ColumnSettings, } +#[derive(Clone, Copy)] +pub struct ColumnValues<'a> { + raw: &'a [SharedString], + subdelimiter: &'a str, +} + +impl<'a> ColumnValues<'a> { + pub fn new(raw: &'a [SharedString], subdelimiter: &'a str) -> Self { + Self { raw, subdelimiter } + } + + pub fn raw(self) -> &'a [SharedString] { + self.raw + } + + pub fn iter(self) -> impl Iterator> { + self.raw + .iter() + .enumerate() + .map(move |(row, raw)| CellValue { + row, + raw, + subdelimiter: self.subdelimiter, + }) + } + + pub fn replace_part( + self, + row: usize, + observed: &str, + replacement: &str, + ) -> Option { + let raw = self.raw.get(row)?.as_ref(); + for (offset, _) in raw.match_indices(observed) { + let end = offset + observed.len(); + let bounded = if self.subdelimiter.is_empty() { + raw.trim() == observed + } else { + (raw[..offset].trim_end().ends_with(self.subdelimiter) + || raw[..offset].trim().is_empty()) + && (raw[end..].trim_start().starts_with(self.subdelimiter) + || raw[end..].trim().is_empty()) + }; + if bounded { + let mut result = raw.to_owned(); + result.replace_range(offset..end, replacement); + return Some(result.into()); + } + } + None + } +} + +#[derive(Clone, Copy)] +pub struct CellValue<'a> { + pub row: usize, + pub raw: &'a str, + subdelimiter: &'a str, +} + +impl<'a> CellValue<'a> { + pub fn parts(self) -> ValueParts<'a> { + if self.subdelimiter.is_empty() { + ValueParts::Whole( + (!self.raw.trim().is_empty()) + .then_some(self.raw.trim()) + .into_iter(), + ) + } else { + ValueParts::Split(self.raw.split(self.subdelimiter)) + } + } +} + +pub enum ValueParts<'a> { + Whole(std::option::IntoIter<&'a str>), + Split(std::str::Split<'a, &'a str>), +} + +impl<'a> Iterator for ValueParts<'a> { + type Item = &'a str; + + fn next(&mut self) -> Option { + match self { + ValueParts::Whole(values) => values.next(), + ValueParts::Split(values) => values.find_map(|value| { + let value = value.trim(); + (!value.is_empty()).then_some(value) + }), + } + } +} + +/// `None` addresses the whole column; `Some` addresses a source-row cell. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ColumnFinding { + pub row: Option, + pub severity: Severity, + pub message: SharedString, + pub group: Option, +} + +impl From<(usize, Severity, SharedString)> for ColumnFinding { + fn from((row, severity, message): (usize, Severity, SharedString)) -> Self { + Self { + row: Some(row), + severity, + message, + group: None, + } + } +} + /// One column-wise check. A `dyn` trait rather than an enum so a validator's dependencies — a /// dictionary, a pattern set, an embedded Lua VM — stay out of every other crate's graph. The /// plugin host implements it once, per loaded script, which is how a Lua file becomes a producer /// indistinguishable from a compiled-in one. -pub trait ColumnValidator: 'static { +pub trait ColumnValidator: Send + Sync + 'static { /// What the Problems panel shows in the source column, and the key its output is replaced by. /// Must be stable across runs and unique across validators. fn name(&self) -> SharedString; + /// Clear snapshot-bound state, including columns removed since the previous run. + fn begin_run(&self) {} + /// Check one column top to bottom. `values` is every row's text for this column, in source-row /// order, so the returned index *is* the row. Returning nothing means the column is clean — /// which is also how a validator that does not apply here opts out. - fn validate( - &self, - column: &ColumnInfo, - values: &[SharedString], - ) -> Vec<(usize, Severity, SharedString)>; + fn validate(&self, column: &ColumnInfo, values: ColumnValues<'_>) -> Vec; } /// Producers that cannot answer while the run is on the stack — the plugin host running its VMs, @@ -109,16 +226,28 @@ pub struct SpellActions { impl Global for SpellActions {} +const SLOW_PUBLISH: Duration = Duration::from_millis(16); + /// Every registered validator. Filled by `app` at startup, which is the only place that knows /// where validators come from. #[derive(Default)] -pub struct Validators(Vec>); +pub struct Validators { + registered: Vec>, + /// Bumped by every run and removal, so a pass that finishes after a newer one started — or + /// after its validator was dropped — publishes nothing. + generation: Arc, + /// Held for a whole background pass: validators keep per-run state (`begin_run`), which two + /// interleaved passes would mix. + running: Arc>, +} impl Global for Validators {} impl Validators { pub fn register(validator: Box, cx: &mut App) { - cx.default_global::().0.push(validator); + cx.default_global::() + .registered + .push(Arc::from(validator)); } /// Drop a validator and clear what it published. Publishing an empty set is the only @@ -131,7 +260,9 @@ impl Validators { Vec::new(), cx, ); - cx.default_global::().0.retain(|v| &v.name() != name); + let this = cx.default_global::(); + this.generation.fetch_add(1, Ordering::SeqCst); + this.registered.retain(|v| &v.name() != name); } /// Run every validator over every column and publish the results. @@ -142,18 +273,35 @@ impl Validators { /// flagged, so the replace-by-source rule makes the run self-invalidating: a fixed cell /// disappears because the next run simply doesn't report it. /// - pub fn run(columns: &[(SharedString, SharedString)], rows: &[Vec], cx: &mut App) { - let sync = cx.try_global::().is_some_and(|v| !v.0.is_empty()); + /// Only the snapshot is taken on the UI thread. The checking runs on the background executor + /// and lands a moment later; a newer run supersedes one that has not landed yet. + pub fn run( + columns: &[(SharedString, SharedString)], + rows: &[Vec], + row_ids: &[settings::project::RowId], + cx: &mut App, + ) { + Diagnostics::set_row_ids(row_ids, cx); + let row_ids: Arc<[_]> = row_ids.into(); + let validators = cx + .try_global::() + .map_or_else(Vec::new, |v| v.registered.clone()); // Copied out because running one hands `cx` back mutably, and a fn pointer is cheap. let deferred: Vec<_> = cx .try_global::() - .map(|v| v.0.values().copied().collect()) + .map(|v| v.0.iter().map(|(name, run)| (name.clone(), *run)).collect()) .unwrap_or_default(); - if !sync && deferred.is_empty() { + if validators.is_empty() && deferred.is_empty() { return; } + let started = Instant::now(); let settings = settings::columns::load(cx); + let subdelimiter = if cx.has_global::() { + settings::effective_text(settings::FILTER_SUBDELIMITER_KEY, cx) + } else { + SharedString::default() + }; let project = cx.try_global::(); let blank = ColumnSettings::default(); // Transposed once, not once per validator: every validator wants the same column-major @@ -171,38 +319,100 @@ impl Validators { .iter() .map(|r| r.get(ix).cloned().unwrap_or_default()) .collect(), + subdelimiter: subdelimiter.clone(), + row_ids: row_ids.clone(), }) .collect(); - if sync { - cx.update_global::(|this, cx| { - for validator in &this.0 { - let items = snapshot + for (name, run) in deferred { + let step = Instant::now(); + run(&snapshot, cx); + log::debug!("deferred validator {name:?} queued in {:?}", step.elapsed()); + } + log::debug!( + "validation snapshot: {} columns × {} rows taken in {:?}", + columns.len(), + rows.len(), + started.elapsed() + ); + if validators.is_empty() { + return; + } + + let this = cx.default_global::(); + let generation = this.generation.fetch_add(1, Ordering::SeqCst) + 1; + let latest = this.generation.clone(); + let running = this.running.clone(); + let current = move || latest.load(Ordering::SeqCst) == generation; + cx.spawn(async move |cx| { + let still_current = current.clone(); + let found = cx + .background_spawn(async move { + let _pass = running + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + if !still_current() { + return None; + } + let started = Instant::now(); + let found: Vec<_> = validators .iter() - .flat_map(|column| { - address( + .map(|validator| { + let step = Instant::now(); + validator.begin_run(); + let items: Vec<_> = snapshot + .iter() + .flat_map(|column| { + address( + validator.name(), + column, + validator.validate( + &column.info(), + ColumnValues::new(&column.values, &column.subdelimiter), + ), + ) + }) + .collect(); + log::debug!( + "validator {:?}: {} findings in {:?}", validator.name(), - column, - validator.validate(&column.info(), &column.values), - ) + items.len(), + step.elapsed() + ); + (validator.name(), items) }) .collect(); - Diagnostics::set( - &Source::Validator(validator.name()), - DATASET_MAIN, - items, - cx, + log::debug!( + "validation checked off the UI thread in {:?}", + started.elapsed() ); + Some(found) + }) + .await; + let Some(found) = found else { + return; + }; + cx.update(|cx| { + if !current() { + return; + } + let started = Instant::now(); + for (name, items) in found { + Diagnostics::set(&Source::Validator(name), DATASET_MAIN, items, cx); + } + let elapsed = started.elapsed(); + if elapsed >= SLOW_PUBLISH { + log::warn!("publishing validation blocked the UI thread for {elapsed:?}"); + } else { + log::debug!("validation published in {elapsed:?}"); } }); - } - for run in deferred { - run(&snapshot, cx); - } + }) + .detach(); } } -/// Turn one validator's `(row, severity, message)` reports into addressed diagnostics. A validator +/// Turn one validator's cell or column reports into addressed diagnostics. A validator /// never builds a [`Location`] or a [`Source`]; this is where that split is honoured, and it is /// public so a deferred producer addresses its findings identically. /// @@ -212,7 +422,7 @@ impl Validators { pub fn address( validator: SharedString, column: &ColumnSnapshot, - found: Vec<(usize, Severity, SharedString)>, + found: Vec, ) -> Vec { let override_to = column .settings @@ -221,16 +431,17 @@ pub fn address( .map(|key| Severity::from_key(key)); found .into_iter() - .map(|(row, severity, message)| Diagnostic { + .map(|finding| Diagnostic { location: Location { dataset: DATASET_MAIN.into(), - row: Some(row), - row_id: None, + row: finding.row, + row_id: finding.row.and_then(|row| column.row_ids.get(row).copied()), column: Some(column.name.clone()), }, - severity: override_to.unwrap_or(severity), + severity: override_to.unwrap_or(finding.severity), source: Source::Validator(validator.clone()), - message, + message: finding.message, + group: finding.group, filed: None, }) .collect() @@ -245,6 +456,28 @@ mod tests { use gpui::{SharedString, TestAppContext}; use std::sync::atomic::{AtomicUsize, Ordering}; + #[test] + fn column_values_borrow_raw_cells_and_split_without_per_cell_storage() { + let raw = [ + SharedString::from("Busson, Carl W.| Dhillon, Baltej Singh "), + SharedString::from(""), + ]; + let values = crate::ColumnValues::new(&raw, "|"); + let cells: Vec<_> = values.iter().collect(); + assert_eq!(cells.len(), 2); + assert_eq!(cells[0].row, 0); + assert_eq!( + cells[0].parts().collect::>(), + ["Busson, Carl W.", "Dhillon, Baltej Singh"] + ); + assert!(cells[1].parts().next().is_none()); + assert_eq!( + values.replace_part(0, "Busson, Carl W.", "Carl W. Busson"), + Some("Carl W. Busson| Dhillon, Baltej Singh ".into()) + ); + assert!(values.replace_part(0, "Carl", "wrong boundary").is_none()); + } + /// Flags any cell equal to `bad`, so a test can steer exactly how many items a run produces. struct Flag { name: &'static str, @@ -259,18 +492,18 @@ mod tests { fn validate( &self, column: &ColumnInfo, - values: &[SharedString], - ) -> Vec<(usize, Severity, SharedString)> { + values: crate::ColumnValues<'_>, + ) -> Vec { values .iter() - .enumerate() - .filter(|(_, v)| v.as_ref() == self.bad) - .map(|(row, _)| { + .filter(|value| value.raw == self.bad) + .map(|value| { ( - row, + value.row, Severity::Error, format!("{} in {}", self.bad, column.name).into(), ) + .into() }) .collect() } @@ -289,19 +522,32 @@ mod tests { ) } + fn register(cx: &mut TestAppContext, validators: &[(&'static str, &'static str)]) { + cx.update(|cx| { + for &(name, bad) in validators { + Validators::register(Box::new(Flag { name, bad }), cx); + } + }); + } + + /// A run only takes the snapshot; the findings land once the background pass is drained. + fn run(cx: &mut TestAppContext, rows: &[Vec]) { + cx.update(|cx| Validators::run(&grid().0, rows, &[], cx)); + cx.run_until_parked(); + } + + fn fixed() -> Vec> { + vec![ + vec!["ok".into(), "ok".into()], + vec!["ok".into(), "ok".into()], + ] + } + #[gpui::test] fn a_run_addresses_and_publishes_what_validators_report(cx: &mut TestAppContext) { + register(cx, &[("flag", "bad")]); + run(cx, &grid().1); cx.update(|cx| { - let (columns, rows) = grid(); - Validators::register( - Box::new(Flag { - name: "flag", - bad: "bad", - }), - cx, - ); - Validators::run(&columns, &rows, cx); - let all = Diagnostics::all(cx); assert_eq!(all.len(), 2); // The validator returned only a row index; the registry supplied dataset, column, and @@ -318,32 +564,14 @@ mod tests { #[gpui::test] fn a_re_run_replaces_only_its_own_validators_output(cx: &mut TestAppContext) { - cx.update(|cx| { - let (columns, rows) = grid(); - Validators::register( - Box::new(Flag { - name: "flag", - bad: "bad", - }), - cx, - ); - Validators::register( - Box::new(Flag { - name: "other", - bad: "ok", - }), - cx, - ); - Validators::run(&columns, &rows, cx); - assert_eq!(Diagnostics::all(cx).len(), 4); + register(cx, &[("flag", "bad"), ("other", "ok")]); + run(cx, &grid().1); + cx.update(|cx| assert_eq!(Diagnostics::all(cx).len(), 4)); - // The user fixes both "bad" cells. `flag` now finds nothing, and publishing nothing is - // what clears its stale entries; `other` is republished independently. - let fixed = vec![ - vec!["ok".into(), "ok".into()], - vec!["ok".into(), "ok".into()], - ]; - Validators::run(&columns, &fixed, cx); + // The user fixes both "bad" cells. `flag` now finds nothing, and publishing nothing is + // what clears its stale entries; `other` is republished independently. + run(cx, &fixed()); + cx.update(|cx| { let all = Diagnostics::all(cx); assert_eq!(all.len(), 4, "`other` now matches all four cells"); assert!( @@ -356,25 +584,10 @@ mod tests { #[gpui::test] fn removing_a_validator_clears_its_findings_and_leaves_the_rest(cx: &mut TestAppContext) { + register(cx, &[("flag", "bad"), ("other", "ok")]); + run(cx, &grid().1); cx.update(|cx| { - let (columns, rows) = grid(); - Validators::register( - Box::new(Flag { - name: "flag", - bad: "bad", - }), - cx, - ); - Validators::register( - Box::new(Flag { - name: "other", - bad: "ok", - }), - cx, - ); - Validators::run(&columns, &rows, cx); assert_eq!(Diagnostics::all(cx).len(), 4); - Validators::remove(&"flag".into(), cx); let all = Diagnostics::all(cx); assert_eq!(all.len(), 2, "removal clears without waiting for a run"); @@ -382,18 +595,42 @@ mod tests { all.iter() .all(|d| d.source == Source::Validator("other".into())) ); + }); - // And it stays gone: the next run has nothing left to republish it. - Validators::run(&columns, &rows, cx); - assert_eq!(Diagnostics::all(cx).len(), 2); + // And it stays gone: the next run has nothing left to republish it. + run(cx, &grid().1); + cx.update(|cx| assert_eq!(Diagnostics::all(cx).len(), 2)); + } + + /// Two runs queued before either lands: only the newer snapshot may publish, or a slow pass + /// over old data would overwrite the answer for the data the user is looking at. + #[gpui::test] + fn a_superseded_run_publishes_nothing(cx: &mut TestAppContext) { + register(cx, &[("flag", "bad")]); + cx.update(|cx| { + Validators::run(&grid().0, &grid().1, &[], cx); + Validators::run(&grid().0, &fixed(), &[], cx); }); + cx.run_until_parked(); + cx.update(|cx| assert!(Diagnostics::all(cx).is_empty())); + } + + #[gpui::test] + fn a_validator_removed_mid_run_is_not_resurrected(cx: &mut TestAppContext) { + register(cx, &[("flag", "bad")]); + cx.update(|cx| { + Validators::run(&grid().0, &grid().1, &[], cx); + Validators::remove(&"flag".into(), cx); + }); + cx.run_until_parked(); + cx.update(|cx| assert!(Diagnostics::all(cx).is_empty())); } #[gpui::test] fn a_run_without_registered_validators_touches_nothing(cx: &mut TestAppContext) { cx.update(|cx| { let (columns, rows) = grid(); - Validators::run(&columns, &rows, cx); + Validators::run(&columns, &rows, &[], cx); assert!(Diagnostics::all(cx).is_empty()); }); } @@ -410,7 +647,7 @@ mod tests { cx.update(|cx| { let (columns, rows) = grid(); crate::AsyncValidators::register("test", record, cx); - Validators::run(&columns, &rows, cx); + Validators::run(&columns, &rows, &[], cx); assert_eq!(SEEN.load(Ordering::SeqCst), 2, "both columns crossed over"); }); } @@ -427,6 +664,8 @@ mod tests { data_type: "Text".into(), settings, values: vec!["Aderman, Ray".into()], + subdelimiter: SharedString::default(), + row_ids: [1].into(), } } @@ -434,16 +673,62 @@ mod tests { /// the column's setting is what decides how loud that lands. #[test] fn a_columns_override_replaces_the_severity_the_check_reported() { - let found = vec![(0, Severity::Error, SharedString::from("not in LCSH"))]; + let found = vec![(0, Severity::Error, SharedString::from("not in LCSH")).into()]; let addressed = crate::address("LCSH".into(), &snapshot(&[("LCSH", "warning")]), found); assert_eq!(addressed[0].severity, Severity::Warning); } + #[test] + fn column_findings_preserve_scope_and_group_under_overrides() { + let group = crate::DiagnosticGroup { + key: "missing".into(), + summary: "Missing values".into(), + subject: None, + }; + let found = vec![super::ColumnFinding { + row: None, + severity: Severity::Error, + message: "Column is empty".into(), + group: Some(group.clone()), + }]; + let addressed = crate::address( + "required".into(), + &snapshot(&[("required", "warning")]), + found, + ); + assert_eq!( + addressed[0].location.scope(), + crate::Scope::Column("Photographer") + ); + assert_eq!(addressed[0].severity, Severity::Warning); + assert_eq!(addressed[0].group, Some(group)); + assert_eq!(crate::Location::dataset("d").scope(), crate::Scope::Dataset); + assert_eq!( + crate::Location::row("d", 4, None).scope(), + crate::Scope::Row(4) + ); + assert_eq!( + crate::Location::cell("d", 4, None, "c").scope(), + crate::Scope::Cell { + row: 4, + column: "c" + } + ); + } + + #[test] + fn addressed_rows_use_their_snapshot_ids() { + let found = vec![(0, Severity::Error, SharedString::from("bad value")).into()]; + let addressed = crate::address("test".into(), &snapshot(&[]), found); + assert_eq!(addressed[0].location.row_id, Some(1)); + } + /// An override names one producer, so it must not quiet the others checking the same column. #[test] fn an_override_for_one_producer_leaves_the_rest_alone() { let column = snapshot(&[("LCSH", "warning")]); - let found = vec![(0, Severity::Error, SharedString::from("no such file"))]; + let found: Vec = + vec![(0, Severity::Error, SharedString::from("no such file")).into()]; let addressed = crate::address("files".into(), &column, found.clone()); assert_eq!(addressed[0].severity, Severity::Error); // And a column with nothing overridden is untouched either way. @@ -464,12 +749,12 @@ mod tests { let (columns, rows) = grid(); crate::AsyncValidators::register("plugins", count, cx); crate::AsyncValidators::register("plugins", count, cx); - Validators::run(&columns, &rows, cx); + Validators::run(&columns, &rows, &[], cx); assert_eq!(RUNS.load(Ordering::SeqCst), 1, "registered twice, ran once"); // A different name is a different producer and does get its own run. crate::AsyncValidators::register("files", count, cx); - Validators::run(&columns, &rows, cx); + Validators::run(&columns, &rows, &[], cx); assert_eq!(RUNS.load(Ordering::SeqCst), 3); }); } diff --git a/crates/plugin-host/src/lib.rs b/crates/plugin-host/src/lib.rs index 4ba85c58..c2958bd5 100644 --- a/crates/plugin-host/src/lib.rs +++ b/crates/plugin-host/src/lib.rs @@ -328,7 +328,13 @@ fn validate_async(columns: &[ColumnSnapshot], cx: &mut App) { let items = columns .iter() .flat_map(|column| { - let found = plugin.validate(&column.info(), &column.values); + let found = plugin.validate( + &column.info(), + diagnostics::ColumnValues::new( + &column.values, + &column.subdelimiter, + ), + ); diagnostics::address(name.clone(), column, found) }) .collect(); diff --git a/crates/plugin-host/src/plugin.rs b/crates/plugin-host/src/plugin.rs index 9f53e8f1..b775bcf1 100644 --- a/crates/plugin-host/src/plugin.rs +++ b/crates/plugin-host/src/plugin.rs @@ -18,7 +18,7 @@ use std::path::Path; use std::sync::{Arc, Mutex, OnceLock}; use std::time::{Duration, Instant}; -use diagnostics::{ColumnInfo, ColumnValidator, Severity}; +use diagnostics::{ColumnInfo, ColumnValidator, ColumnValues, Severity}; use gpui::SharedString; use mlua::{Function, Lua, LuaOptions, LuaSerdeExt as _, SerializeOptions, StdLib, Table, VmState}; use plugin_api::{ @@ -505,8 +505,8 @@ impl ColumnValidator for LuaPlugin { fn validate( &self, column: &ColumnInfo, - values: &[SharedString], - ) -> Vec<(usize, Severity, SharedString)> { + values: ColumnValues<'_>, + ) -> Vec { // A plugin that never loaded reports nothing here; `reload` logged that failure once // instead of once per column. let Ok(loaded) = self.state.as_ref() else { @@ -516,8 +516,8 @@ impl ColumnValidator for LuaPlugin { return Vec::new(); }; - match self.call_validate(loaded, validate, column, values) { - Ok(found) => found, + match self.call_validate(loaded, validate, column, values.raw()) { + Ok(found) => found.into_iter().map(Into::into).collect(), // Logged, not returned: a plugin that throws is broken code, and this list is what is // wrong with the archivist's data. Reporting it here also had to invent a row. Err(err) => { @@ -1118,7 +1118,7 @@ fn setting_spec(item: Table) -> mlua::Result { mod tests { use crate::plugin::HTTP_BUDGET; use crate::{Env, LuaPlugin, PERMISSION_NET, PackageDescriptor, Writes}; - use diagnostics::{ColumnInfo, ColumnValidator, Severity}; + use diagnostics::{ColumnInfo, ColumnValidator, ColumnValues, Severity}; use gpui::SharedString; use plugin_api::{ Bar, BarAction, CommandContext, ExportColumn, ExportSnapshot, MenuTarget, SettingKind, @@ -1249,7 +1249,17 @@ mod tests { .iter() .map(|value| (*value).into()) .collect::>(); - plugin.validate(&column, &values) + plugin + .validate(&column, ColumnValues::new(&values, "")) + .into_iter() + .map(|finding| { + ( + finding.row.expect("IIIF findings address cells"), + finding.severity, + finding.message, + ) + }) + .collect() } fn check(source: &str, values: &[&str]) -> Vec<(usize, Severity, SharedString)> { @@ -1271,7 +1281,17 @@ mod tests { settings: &settings, }; let values: Vec = values.iter().map(|v| SharedString::from(*v)).collect(); - plugin.validate(&column, &values) + plugin + .validate(&column, ColumnValues::new(&values, "")) + .into_iter() + .map(|f| { + ( + f.row.expect("script findings address cells"), + f.severity, + f.message, + ) + }) + .collect() } const FLAG_BAD: &str = r#" @@ -1388,9 +1408,10 @@ mod tests { data_type: "Text", settings: &settings, }; - let found = plugin(ECHO_SETTINGS).validate(&column, &["x".into()]); + let values = ["x".into()]; + let found = plugin(ECHO_SETTINGS).validate(&column, ColumnValues::new(&values, "")); assert_eq!( - found[0].2, "/nil/nil", + found[0].message, "/nil/nil", "another plugin's object is invisible" ); } @@ -1554,8 +1575,9 @@ mod tests { data_type: "Text", settings: &settings, }; - let found = plugin.validate(&column, &["x".into()]); - assert_eq!(found[0].2, "Film"); + let values = ["x".into()]; + let found = plugin.validate(&column, ColumnValues::new(&values, "")); + assert_eq!(found[0].message, "Film"); } #[test] @@ -1861,11 +1883,12 @@ mod tests { data_type: "Text", settings: &settings, }; - let found = reloaded.validate(&column, &[url.into()]); + let values = [url.into()]; + let found = reloaded.validate(&column, ColumnValues::new(&values, "")); assert!( found .iter() - .any(|(_, _, message)| message.contains("Settings ▸ Plugins")), + .any(|finding| finding.message.contains("Settings ▸ Plugins")), "{found:?}" ); } @@ -2050,7 +2073,12 @@ mod tests { data_type: "Text", settings: &settings, }; - let run = |plugin: &LuaPlugin| plugin.validate(&column, &["x".into()])[0].2.clone(); + let values = ["x".into()]; + let run = |plugin: &LuaPlugin| { + plugin.validate(&column, ColumnValues::new(&values, ""))[0] + .message + .clone() + }; let plugin = plugin(source); assert!( @@ -2117,15 +2145,20 @@ mod tests { settings: &settings, }; let values = ["Film; Pottery", "Film", ""]; - plugin.validate(&column, &values.map(SharedString::from)) + let values = values.map(SharedString::from); + plugin.validate(&column, ColumnValues::new(&values, "")) }; let one = checked(json!(["subject"])); assert_eq!(one.len(), 1, "{one:?}"); - assert_eq!(one[0].0, 0, "a sub-delimited cell is checked term by term"); - assert_eq!(one[0].1, Severity::Error); assert_eq!( - one[0].2, + one[0].row, + Some(0), + "a sub-delimited cell is checked term by term" + ); + assert_eq!(one[0].severity, Severity::Error); + assert_eq!( + one[0].message, r#""Pottery" is not a term in the Islandora subject vocabulary"# ); @@ -2169,10 +2202,15 @@ mod tests { settings: &settings, }; let values = ["Text", "Nonsense Value", "Still Image"]; - let found = plugin.validate(&column, &values.map(SharedString::from)); + let values = values.map(SharedString::from); + let found = plugin.validate(&column, ColumnValues::new(&values, "")); assert_eq!(found.len(), 1, "only the invented term is wrong: {found:?}"); - assert!(found[0].2.contains("Nonsense Value"), "{}", found[0].2); + assert!( + found[0].message.contains("Nonsense Value"), + "{}", + found[0].message + ); assert!( plugin.take_storage().is_some(), "the verdicts were cached, or every edit would re-ask the server" diff --git a/crates/project-wizard/src/data.rs b/crates/project-wizard/src/data.rs index 7e772697..8d8d6d6a 100644 --- a/crates/project-wizard/src/data.rs +++ b/crates/project-wizard/src/data.rs @@ -331,6 +331,8 @@ pub struct ColumnConfigEntry { pub authority: Option, /// Whether to spell-check it. `None` leaves the default (on) alone. pub spellcheck: Option, + /// Whether to review near-duplicate displayed values. `None` leaves the default (off) alone. + pub variant_review: Option, /// How loud the authority above reports, as a `Severity` key spelling. Per producer, so each /// plugin's own severity travels in [`Self::extra`] beside its mapping instead. pub authority_severity: Option, @@ -385,12 +387,13 @@ fn canonical_type(declared: &str) -> String { /// The headers this module understands. Everything else in the file is a plugin's, and lands in /// [`ColumnConfigEntry::extra`] under its own spelling. -const KNOWN: [&str; 6] = [ +const KNOWN: [&str; 7] = [ "Column Name", "Data Type", "Description", "Authority", "Spellcheck", + "Variant Review", "Authority Severity", ]; @@ -421,7 +424,11 @@ pub fn load_column_config( let at = |wanted: &str| headers.iter().position(|h| h.eq_ignore_ascii_case(wanted)); let (name_ix, type_ix) = (at("Column Name"), at("Data Type")); let (desc_ix, authority_ix) = (at("Description"), at("Authority")); - let (spellcheck_ix, severity_ix) = (at("Spellcheck"), at("Authority Severity")); + let (spellcheck_ix, variant_ix, severity_ix) = ( + at("Spellcheck"), + at("Variant Review"), + at("Authority Severity"), + ); // Whatever is left belongs to a plugin, carried through by the name it was written under. let extra_ixs: Vec<(usize, String)> = headers .iter() @@ -452,6 +459,7 @@ pub fn load_column_config( description: cell(desc_ix), authority: Some(cell(authority_ix)).filter(|s| !s.is_empty()), spellcheck: yes_no(&cell(spellcheck_ix)), + variant_review: yes_no(&cell(variant_ix)), authority_severity: Some(cell(severity_ix).to_ascii_lowercase()) .filter(|s| !s.is_empty()), extra: extra_ixs diff --git a/crates/project-wizard/src/steps/review.rs b/crates/project-wizard/src/steps/review.rs index 840ab17b..040748c5 100644 --- a/crates/project-wizard/src/steps/review.rs +++ b/crates/project-wizard/src/steps/review.rs @@ -47,6 +47,9 @@ fn column_settings( if let Some(on) = entry.spellcheck { settings.spellcheck = on; } + if let Some(on) = entry.variant_review { + settings.variant_review = on; + } // Severity is per producer: the authority names its own, and each plugin's rides in the // `::Severity` column beside its mapping. if let (Some(authority), Some(severity)) = diff --git a/crates/settings/src/columns.rs b/crates/settings/src/columns.rs index 18727d61..6adc4057 100644 --- a/crates/settings/src/columns.rs +++ b/crates/settings/src/columns.rs @@ -112,6 +112,18 @@ pub struct ColumnSettings { /// can't derive `Default`. #[serde(default = "yes")] pub spellcheck: bool, + /// Whether this column should report near-duplicate displayed values for human review. + #[serde(default)] + pub variant_review: bool, + /// Exact value pairs the cataloguer confirmed are distinct, in canonical lexical order. + #[serde(default)] + pub distinct_variants: std::collections::BTreeSet<(String, String)>, + /// Producer and stable finding keys the cataloguer chose to suppress in this column. + #[serde(default)] + pub ignored_diagnostics: std::collections::BTreeSet<(String, String)>, + /// Producer, finding key and stable row id of single occurrences suppressed in this column. + #[serde(default)] + pub ignored_occurrences: std::collections::BTreeSet<(String, String, crate::project::RowId)>, /// Which authority file this column's values must exist in, by name (`"LCSH"`), or `None` to /// check nothing. Separate from the column's [`ColumnType`] because the two answer different /// questions: the type is the shape a value has, this is the list it has to appear on. @@ -139,6 +151,10 @@ impl Default for ColumnSettings { Self { filter_enabled: false, spellcheck: true, + variant_review: false, + distinct_variants: Default::default(), + ignored_diagnostics: Default::default(), + ignored_occurrences: Default::default(), authority: None, plugins: BTreeMap::new(), severity: BTreeMap::new(), @@ -219,6 +235,16 @@ pub fn update(col_key: &str, f: impl FnOnce(&mut ColumnSettings), cx: &mut App) store(&map, cx); } +/// Drop every ignore, both column-wide and single-occurrence, in every column. +pub fn clear_ignored(cx: &mut App) { + let mut map = load(cx); + for settings in map.values_mut() { + settings.ignored_diagnostics.clear(); + settings.ignored_occurrences.clear(); + } + store(&map, cx); +} + /// Carry a column's settings over to its new name. The key *is* the name, so a rename that /// skipped this would silently reset the column's authority, spellcheck and plugin mappings. pub fn rename(before: &str, after: &str, cx: &mut App) { @@ -312,6 +338,7 @@ mod tests { ColumnSettings::default() ); assert!(!ColumnSettings::default().filter_enabled); + assert!(!ColumnSettings::default().variant_review); } /// The reason this is a map and not `ColumnLayout`'s positional vec: a column disappearing @@ -344,6 +371,9 @@ mod tests { fn a_blob_without_a_plugin_bucket_reads_as_empty() { let parsed = parse(Some(r#"{"c2":{"filter_enabled":true}}"#)); assert!(parsed["c2"].plugins.is_empty()); + assert!(!parsed["c2"].variant_review); + assert!(parsed["c2"].ignored_diagnostics.is_empty()); + assert!(parsed["c2"].ignored_occurrences.is_empty()); } /// One column can override two producers differently — the map is keyed by producer for diff --git a/crates/settings/src/lib.rs b/crates/settings/src/lib.rs index 1cf5ecf8..eebcd099 100644 --- a/crates/settings/src/lib.rs +++ b/crates/settings/src/lib.rs @@ -205,6 +205,12 @@ pub enum Setting { label: &'static str, description: &'static str, }, + TextWithAction { + key: &'static str, + label: &'static str, + description: &'static str, + on_change: fn(&mut App), + }, Switch { key: &'static str, label: &'static str, @@ -247,12 +253,34 @@ impl Setting { move |cx: &App| scoped_text(key, cx), move |val: SharedString, cx: &mut App| set_scoped_text(key, val, cx), ), + None, ), ) .description(described(description, key, cx)) // gpui-component pins horizontal-layout inputs to a fixed `w_64`, which clips inside a // narrow settings pane; the stacked layout gives them `w_full` instead. .layout(Axis::Vertical), + Setting::TextWithAction { + key, + label, + description, + on_change, + } => SettingItem::new( + label, + resettable( + key, + SettingField::input( + move |cx: &App| scoped_text(key, cx), + move |val: SharedString, cx: &mut App| { + set_scoped_text(key, val, cx); + on_change(cx); + }, + ), + Some(on_change), + ), + ) + .description(described(description, key, cx)) + .layout(Axis::Vertical), Setting::Switch { key, @@ -266,6 +294,7 @@ impl Setting { move |cx: &App| scoped_bool(key, cx), move |val: bool, cx: &mut App| set_scoped_bool(key, val, cx), ), + None, ), ) .description(described(description, key, cx)), @@ -289,6 +318,7 @@ impl Setting { move |cx: &App| scoped_text(key, cx), move |val: SharedString, cx: &mut App| set_scoped_text(key, val, cx), ), + None, ), ) .description(described(description, key, cx)) @@ -347,12 +377,21 @@ fn described(description: &'static str, key: &'static str, cx: &App) -> SharedSt /// Wires a dual-scope field to the page's Reset button, which clears the project's own value so the /// row inherits the user-wide one again. Nothing to reset in the User scope: that store *is* the /// default this returns to. -fn resettable(key: &'static str, field: SettingField) -> SettingField { +fn resettable( + key: &'static str, + field: SettingField, + on_change: Option, +) -> SettingField { field.on_reset( move |cx: &App| { SettingsScope::current(cx) == SettingsScope::Project && has_project_override(key, cx) }, - move |_window: &mut Window, cx: &mut App| clear_project_override(key, cx), + move |_window: &mut Window, cx: &mut App| { + clear_project_override(key, cx); + if let Some(on_change) = on_change { + on_change(cx); + } + }, ) } diff --git a/crates/spellcheck/Cargo.toml b/crates/spellcheck/Cargo.toml index 1c4888df..292f8ea7 100644 --- a/crates/spellcheck/Cargo.toml +++ b/crates/spellcheck/Cargo.toml @@ -12,6 +12,7 @@ log.workspace = true # the data dir, not a fork. Its only two dependencies are a hasher, so the dictionary is the # weight here, not the crate. spellbook = "0.4" +whatlang = "0.18" # Downloading a dictionary the user picked. Blocking like `data-exchange`'s fetch and for the same # reason: gpui's executor is not a tokio reactor, so this runs on the background executor. reqwest = { version = "0.12", features = ["blocking"] } diff --git a/crates/spellcheck/src/catalogue.rs b/crates/spellcheck/src/catalogue.rs index 583e982b..16e7deea 100644 --- a/crates/spellcheck/src/catalogue.rs +++ b/crates/spellcheck/src/catalogue.rs @@ -395,11 +395,18 @@ mod round_trip { download("fr").expect("French downloads"); assert!(is_installed("fr"), "both files landed"); - let spell = SpellCheck::load("fr", false).expect("the downloaded dictionary parses"); - let dictionary = spell.dictionary.read().unwrap(); + let spell = + SpellCheck::load(&["fr".to_string()], false).expect("the downloaded dictionary parses"); + let dictionaries = spell.dictionaries.read().unwrap(); + let dictionary = &dictionaries + .loaded + .iter() + .find(|loaded| loaded.code == "fr") + .expect("French was loaded") + .dictionary; assert!(dictionary.check("bonjour"), "French words pass"); assert!(!dictionary.check("bonjoure"), "and French typos do not"); - drop(dictionary); + drop(dictionaries); remove("fr").expect("removable"); assert!(!is_installed("fr")); diff --git a/crates/spellcheck/src/lib.rs b/crates/spellcheck/src/lib.rs index ddbb42fe..01b2ed4d 100644 --- a/crates/spellcheck/src/lib.rs +++ b/crates/spellcheck/src/lib.rs @@ -10,11 +10,14 @@ pub mod catalogue; use std::borrow::Cow; +use std::collections::{BTreeMap, HashMap}; use std::io::Write as _; use std::path::PathBuf; -use std::sync::{Arc, RwLock}; +use std::sync::{Arc, Mutex, RwLock}; -use diagnostics::{ColumnInfo, ColumnValidator, Misspelling, Severity}; +use diagnostics::{ + ColumnInfo, ColumnValidator, ColumnValues, GroupFix, GroupMember, Misspelling, Severity, +}; use gpui::{App, Global, SharedString}; use settings::columns::ColumnType; use spellbook::Dictionary; @@ -40,7 +43,7 @@ static EN_AFF: &str = include_str!("../dictionaries/en.aff"); static EN_DIC: &str = include_str!("../dictionaries/en.dic"); /// The name the Problems panel shows, and the key this validator's output is replaced by. -const VALIDATOR_NAME: &str = "spell"; +pub const SPELLING_VALIDATOR_NAME: &str = "spell"; /// Shortest token worth checking. Below this the dictionary answers "yes" for almost anything. const MIN_WORD_LEN: usize = 3; @@ -65,7 +68,7 @@ const MAX_SUGGESTED_WORDS: usize = 3; /// re-parsing half a megabyte to learn one word. #[derive(Clone)] pub struct SpellCheck { - dictionary: Arc>, + dictionaries: Arc>, /// Read once at load, so changing it takes a restart — the same deal the language gets, and /// for the same reason: [`ColumnValidator::validate`] is handed no context to re-read from. ignore_capitalized: bool, @@ -73,6 +76,135 @@ pub struct SpellCheck { impl Global for SpellCheck {} +struct LoadedDictionary { + code: String, + dictionary: Dictionary, +} + +struct DictionarySet { + loaded: Vec, + preferred: String, + /// Each distinct cell value’s verdict, kept across runs — checking the sheet is seconds, and + /// almost none of it changes between runs. Only [`Self::learn`] changes an answer, so only it + /// clears this. + checked: Mutex>, +} + +/// The dictionary chosen for one value and its flagged words, or `None` when no language fits. +type Checked = Option<(String, Vec<(String, WordOutcome)>)>; + +/// ponytail: past this many distinct values the cache is dropped wholesale; evict by age if edits +/// ever churn through it in a session. +const MAX_CHECKED: usize = 200_000; + +impl DictionarySet { + fn base(code: &str) -> &str { + code.split('-').next().unwrap_or(code) + } + + /// Teach every loaded dictionary `word`. Returns whether any accepted it. + fn learn(&mut self, word: &str) -> bool { + let mut changed = false; + for loaded in &mut self.loaded { + match loaded.dictionary.add(word) { + Ok(()) => changed = true, + Err(err) => log::warn!( + "could not add \"{word}\" to the {} dictionary: {err}", + loaded.code + ), + } + } + if changed && let Ok(checked) = self.checked.get_mut() { + checked.clear(); + } + changed + } + + fn check(&self, value: &str, ignore_capitalized: bool) -> Checked { + let loaded = self.for_text(value)?; + let english = Self::base(&loaded.code) == "en"; + let mut seen = std::collections::BTreeSet::new(); + let flagged = words(value, false) + .filter(|word| !(english && non_english_elision(word)) && seen.insert(*word)) + .filter_map( + |word| match classify_word(&loaded.dictionary, word, ignore_capitalized) { + outcome @ (WordOutcome::Misspelled | WordOutcome::Capitalization(_)) => { + Some((word.to_owned(), outcome)) + } + WordOutcome::Clean | WordOutcome::ProperNoun => None, + }, + ) + .collect(); + Some((loaded.code.clone(), flagged)) + } + + /// Pick one installed base language only when the dictionaries provide enough evidence. + fn for_text(&self, text: &str) -> Option<&LoadedDictionary> { + let english_only = self + .loaded + .iter() + .all(|loaded| Self::base(&loaded.code) == "en"); + let enough_text = text.chars().filter(|c| c.is_alphabetic()).count() >= 20; + if english_only + && enough_text + && whatlang::detect(text).is_some_and(|info| { + info.lang() != whatlang::Lang::Eng + && (info.is_reliable() || info.confidence() >= 0.45) + }) + { + return None; + } + let tokens: Vec<&str> = words(text, false) + .filter(|word| { + !starts_uppercase(word) + || self + .loaded + .iter() + .any(|loaded| known_in_any_case(&loaded.dictionary, word).is_some()) + }) + .collect(); + let mut bases: BTreeMap<&str, (usize, usize)> = BTreeMap::new(); + for (ix, loaded) in self.loaded.iter().enumerate() { + let accepted = tokens + .iter() + .filter(|word| known_in_any_case(&loaded.dictionary, word).is_some()) + .count(); + let base = Self::base(&loaded.code); + let entry = bases.entry(base).or_insert((accepted, ix)); + let configured_variant = + Self::base(&self.preferred) == base && loaded.code == self.preferred; + let current_is_configured = self.loaded[entry.1].code == self.preferred; + if configured_variant || !current_is_configured && accepted > entry.0 { + entry.1 = ix; + } + entry.0 = entry.0.max(accepted); + } + + if bases.len() == 1 { + return bases.into_values().next().map(|(_, ix)| &self.loaded[ix]); + } + if tokens.len() < 2 { + return None; + } + + let mut ranked: Vec<(usize, usize)> = bases.into_values().collect(); + ranked.sort_by_key(|score| std::cmp::Reverse(score.0)); + let winner = ranked[0]; + let runner_up = ranked.get(1).map_or(0, |score| score.0); + let enough_coverage = winner.0 * 5 >= tokens.len() * 3; + let clear_lead = (winner.0 - runner_up) * 5 >= tokens.len(); + (enough_coverage && clear_lead).then(|| &self.loaded[winner.1]) + } +} + +#[derive(Clone)] +enum WordOutcome { + Clean, + Capitalization(String), + ProperNoun, + Misspelled, +} + /// Whether spell checking is on. Compared against the text a stored bool renders as, so that /// *unset* reads as on the way `AUTOSAVE_KEY != "off"` does — `effective_bool` cannot express /// that, since it answers `false` for a key nobody has written yet. @@ -92,25 +224,36 @@ pub fn ignore_capitalized(cx: &App) -> bool { /// a click that did nothing. A download takes seconds and there is no progress to report from a /// blocking request, so "in flight or not" is the whole state there is. #[derive(Default)] -pub struct Downloading(pub std::collections::HashSet); +pub struct Downloading { + active: std::collections::HashSet, + wanted: std::collections::HashSet, +} impl Global for Downloading {} pub fn is_downloading(code: &str, cx: &App) -> bool { cx.try_global::() - .is_some_and(|d| d.0.contains(code)) + .is_some_and(|d| d.active.contains(code)) } -/// Fetch `code` in the background and switch to it once it lands, the way tapping an uninstalled -/// language on a phone both downloads and selects it. Failures are logged rather than surfaced: -/// the row simply goes back to offering the download. -pub fn start_download(code: SharedString, cx: &mut App) { +pub fn retain_wanted_downloads(selected: &[SharedString], cx: &mut App) { + let downloading = cx.default_global::(); + downloading + .wanted + .retain(|code| selected.iter().any(|selected| selected == code)); +} + +/// Fetch `code` in the background and add it to the checked languages once it lands, the way +/// tapping an uninstalled language on a phone both downloads and selects it. `then` runs after the +/// selection changes, so the caller can reload the checker. Failures are logged rather than +/// surfaced: the row simply goes back to offering the download. +pub fn start_download(code: SharedString, then: fn(&mut App), cx: &mut App) { if is_downloading(&code, cx) { return; } - cx.default_global::() - .0 - .insert(code.to_string()); + let downloading = cx.default_global::(); + downloading.active.insert(code.to_string()); + downloading.wanted.insert(code.to_string()); let fetching = code.clone(); let fetch = cx .background_executor() @@ -118,12 +261,21 @@ pub fn start_download(code: SharedString, cx: &mut App) { cx.spawn(async move |cx| { let result = fetch.await; cx.update(|cx| { - cx.default_global::().0.remove(code.as_ref()); + let downloading = cx.default_global::(); + downloading.active.remove(code.as_ref()); + let wanted = downloading.wanted.remove(code.as_ref()); match result { - Ok(()) => { + Ok(()) if wanted => { log::info!("downloaded the {} dictionary", catalogue::name_of(&code)); - settings::set_scoped_text(SPELLCHECK_LANGUAGE_KEY, code, cx); + let mut codes = languages(cx); + codes.push(code.to_string()); + set_languages(&codes, cx); + then(cx); } + Ok(()) => log::info!( + "downloaded the {} dictionary without selecting it", + catalogue::name_of(&code) + ), Err(err) => log::error!( "could not download the {} dictionary, so it was not installed: {err}", catalogue::name_of(&code) @@ -135,73 +287,92 @@ pub fn start_download(code: SharedString, cx: &mut App) { .detach(); } -/// The configured language, or [`DEFAULT_LANGUAGE`] when unset. -pub fn language(cx: &App) -> SharedString { - let configured = settings::effective_text(SPELLCHECK_LANGUAGE_KEY, cx); - if configured.is_empty() { - DEFAULT_LANGUAGE.into() - } else { - configured +/// The languages to check with, in the order they were picked, or [`DEFAULT_LANGUAGE`] when none +/// are. Stored comma-separated, so a setting written before multi-select still reads as one. +pub fn languages(cx: &App) -> Vec { + let mut codes: Vec = Vec::new(); + for code in settings::effective_text(SPELLCHECK_LANGUAGE_KEY, cx).split(',') { + let code = code.trim(); + if !code.is_empty() && !codes.iter().any(|c| c == code) { + codes.push(code.to_owned()); + } + } + if codes.is_empty() { + codes.push(DEFAULT_LANGUAGE.to_owned()); } + codes +} + +pub fn set_languages(codes: &[String], cx: &mut App) { + settings::set_scoped_text(SPELLCHECK_LANGUAGE_KEY, codes.join(",").into(), cx); } impl SpellCheck { - /// Parse `language`'s dictionary and replay the user's custom words over it. Slow enough - /// (half a megabyte of affix rules) that the caller should do this off the UI thread. - pub fn load(language: &str, ignore_capitalized: bool) -> Option { - let (aff, dic) = source(language); - let mut dictionary = match Dictionary::new(&aff, &dic) { - Ok(dictionary) => dictionary, - Err(err) => { - log::error!( - "the {language} dictionary is unreadable, so spell checking is off: {err}" - ); - return None; - } - }; + /// Parse the chosen dictionaries and replay the user's custom words over each. Slow enough + /// that the caller must do this off the UI thread. + pub fn load(languages: &[String], ignore_capitalized: bool) -> Option { let custom = custom_words(); + let mut loaded = Vec::new(); + for code in languages { + let Some((aff, dic)) = source_exact(code) else { + log::warn!("the {code} dictionary is not installed, so it was skipped"); + continue; + }; + let mut dictionary = match Dictionary::new(&aff, &dic) { + Ok(dictionary) => dictionary, + Err(err) => { + log::warn!("the {code} dictionary is unreadable and was skipped: {err}"); + continue; + } + }; + for word in &custom { + if let Err(err) = dictionary.add(word) { + log::warn!("skipped \"{word}\" from the {code} custom dictionary: {err}"); + } + } + loaded.push(LoadedDictionary { + code: code.clone(), + dictionary, + }); + } + if loaded.is_empty() { + log::error!("none of the chosen dictionaries was readable, so spell checking is off"); + return None; + } + let preferred = loaded[0].code.clone(); log::info!( - "spell checking with the {language} dictionary and {} of your own words", + "spell checking with {}, preferring {preferred}, and {} of your own words", + loaded + .iter() + .map(|d| d.code.as_str()) + .collect::>() + .join(", "), custom.len() ); - for word in custom { - if let Err(err) = dictionary.add(&word) { - log::warn!( - "skipped \"{word}\" from your custom dictionary, which is not a word the dictionary format accepts: {err}" - ); - } - } Some(Self { - dictionary: Arc::new(RwLock::new(dictionary)), + dictionaries: Arc::new(RwLock::new(DictionarySet { + loaded, + preferred: preferred.to_string(), + checked: Default::default(), + })), ignore_capitalized, }) } } -/// The `.aff`/`.dic` pair for `language`: compiled in for the two built-in English variants, -/// otherwise read from the folder [`catalogue::download`] writes to. -/// -/// A language that is configured but not on disk falls back to the default rather than turning -/// the feature off — a half-finished download must not cost the user their spell checking. -fn source(language: &str) -> (Cow<'static, str>, Cow<'static, str>) { - let default = || (Cow::Borrowed(EN_CA_AFF), Cow::Borrowed(EN_CA_DIC)); +/// The `.aff`/`.dic` pair for an installed language. Never substitute English for a missing file: +/// treating a failed French download as English would manufacture the false positives this module +/// is meant to prevent. +fn source_exact(language: &str) -> Option<(Cow<'static, str>, Cow<'static, str>)> { match language { - "" | DEFAULT_LANGUAGE => return default(), - "en" => return (Cow::Borrowed(EN_AFF), Cow::Borrowed(EN_DIC)), - _ => {} - } - let Some((aff, dic)) = catalogue::paths(language) else { - return default(); - }; - match (std::fs::read_to_string(&aff), std::fs::read_to_string(&dic)) { - (Ok(aff), Ok(dic)) => (aff.into(), dic.into()), + DEFAULT_LANGUAGE => Some((Cow::Borrowed(EN_CA_AFF), Cow::Borrowed(EN_CA_DIC))), + "en" => Some((Cow::Borrowed(EN_AFF), Cow::Borrowed(EN_DIC))), _ => { - log::warn!( - "the {} dictionary is not downloaded, so spell checking falls back to {}", - catalogue::name_of(language), - catalogue::name_of(DEFAULT_LANGUAGE) - ); - default() + let (aff, dic) = catalogue::paths(language)?; + Some(( + std::fs::read_to_string(aff).ok()?.into(), + std::fs::read_to_string(dic).ok()?.into(), + )) } } } @@ -241,15 +412,14 @@ pub fn add_word(word: &str, cx: &mut App) -> bool { let Some(this) = cx.try_global::().cloned() else { return false; }; - let Ok(mut dictionary) = this.dictionary.write() else { + let Ok(mut dictionaries) = this.dictionaries.write() else { log::error!("the dictionary is poisoned, so \"{word}\" was not added"); return false; }; - if let Err(err) = dictionary.add(word) { - log::warn!("could not add \"{word}\" to the dictionary: {err}"); + if !dictionaries.learn(word) { return false; } - drop(dictionary); + drop(dictionaries); if let Some(path) = custom_dictionary_path() { let appended = std::fs::create_dir_all(path.parent().unwrap_or(&path)).and_then(|()| { @@ -276,42 +446,114 @@ pub fn misspellings(text: &str, cx: &App) -> Vec { let Some(this) = cx.try_global::() else { return Vec::new(); }; - let Ok(dictionary) = this.dictionary.read() else { + let Ok(dictionaries) = this.dictionaries.read() else { return Vec::new(); }; + let Some(loaded) = dictionaries.for_text(text) else { + return Vec::new(); + }; + let dictionary = &loaded.dictionary; let mut found: Vec<(SharedString, Vec)> = Vec::new(); - for word in words(text, this.ignore_capitalized) { + for word in words(text, false) { + if DictionarySet::base(&loaded.code) == "en" && non_english_elision(word) { + continue; + } if found.len() == MAX_SUGGESTED_WORDS { break; } - if dictionary.check(word) || found.iter().any(|(seen, _)| seen == word) { + if found.iter().any(|(seen, _)| seen == word) { continue; } - // Measured on the shipped dictionary: ngram suggestion costs 25-40ms a word against well under - // one, and only earns its keep on a word too mangled for the edit-distance pass to reach - // ("documentaire" → "documentary"). So it runs as a fallback, not as the first answer. - let mut suggestions = Vec::new(); - dictionary - .suggester() - .with_ngram_suggestions(false) - .suggest(word, &mut suggestions); - if suggestions.is_empty() { - dictionary.suggest(word, &mut suggestions); + if let Some(corrections) = corrections(dictionary, word, this.ignore_capitalized) { + found.push((word.into(), corrections)); } - suggestions.truncate(MAX_SUGGESTIONS); - found.push(( - word.into(), - suggestions.into_iter().map(SharedString::from).collect(), - )); } found } -/// Split `text` into the tokens worth checking. Word characters are alphanumerics plus the -/// apostrophe, so `don't` survives as one word. +/// What a flagged word could become: the dictionary's casing for a capitalization slip, else +/// ranked suggestions. `None` for a word nothing flags. +fn corrections( + dictionary: &Dictionary, + word: &str, + ignore_capitalized: bool, +) -> Option> { + match classify_word(dictionary, word, ignore_capitalized) { + WordOutcome::Misspelled => Some(suggestions(dictionary, word)), + WordOutcome::Capitalization(canonical) => Some(vec![canonical.into()]), + WordOutcome::Clean | WordOutcome::ProperNoun => None, + } +} + +fn suggestions(dictionary: &Dictionary, word: &str) -> Vec { + // N-gram suggestions cost 25-40ms per word, so use them only when edit distance finds nothing. + let mut suggestions = Vec::new(); + dictionary + .suggester() + .with_ngram_suggestions(false) + .suggest(word, &mut suggestions); + if suggestions.is_empty() { + dictionary.suggest(word, &mut suggestions); + } + suggestions.truncate(MAX_SUGGESTIONS); + suggestions.into_iter().map(SharedString::from).collect() +} + +pub fn spelling_group_fixes(members: &[GroupMember], cx: &App) -> Vec { + let Some(first) = members.first() else { + return Vec::new(); + }; + let Some(word) = first.subject.as_deref() else { + return Vec::new(); + }; + let Some(this) = cx.try_global::() else { + return Vec::new(); + }; + let Ok(dictionaries) = this.dictionaries.read() else { + return Vec::new(); + }; + let Some(loaded) = dictionaries.for_text(&first.text) else { + return Vec::new(); + }; + let Some(corrections) = corrections(&loaded.dictionary, word, this.ignore_capitalized) else { + return Vec::new(); + }; + let learned = word.to_owned(); + let add = GroupFix::action(format!("Add “{word}” to dictionary"), move |cx| { + if add_word(&learned, cx) + && let Some(hooks) = cx.try_global::().copied() + { + (hooks.revalidate)(cx); + } + }); + corrections + .into_iter() + .filter_map(|suggestion| { + let replacements = members + .iter() + .map(|member| { + (member.subject.as_deref() == Some(word)).then(|| { + ( + member.location.clone(), + member.text.replace(word, suggestion.as_ref()).into(), + ) + }) + }) + .collect::>>()?; + Some(GroupFix::replacements( + format!("Change all “{word}” to “{suggestion}”"), + replacements, + )) + }) + .chain([add]) + .collect() +} + +/// Split `text` into the tokens worth checking. Straight and typographic apostrophes stay inside +/// words, so contractions survive as one token regardless of how they were entered. fn words(text: &str, ignore_capitalized: bool) -> impl Iterator { - text.split(|c: char| !c.is_alphanumeric() && c != '\'') - .map(|token| token.trim_matches('\'')) + text.split(|c: char| !c.is_alphanumeric() && c != '\'' && c != '’') + .map(|token| token.trim_matches(['\'', '’'])) .filter(move |token| checkable(token, ignore_capitalized)) } @@ -329,44 +571,117 @@ fn checkable(token: &str, ignore_capitalized: bool) -> bool { if len <= ACRONYM_LEN && token.chars().all(char::is_uppercase) { return false; } - !ignore_capitalized || !token.starts_with(char::is_uppercase) + !ignore_capitalized || !starts_uppercase(token) +} + +fn non_english_elision(word: &str) -> bool { + let Some(split) = word.find(['\'', '’']) else { + return false; + }; + matches!( + word[..split].to_lowercase().as_str(), + "c" | "d" | "j" | "l" | "m" | "n" | "qu" | "s" | "t" + ) +} + +fn starts_uppercase(value: &str) -> bool { + value.chars().next().is_some_and(char::is_uppercase) +} + +fn titlecase(word: &str) -> String { + let mut chars = word.chars(); + let Some(first) = chars.next() else { + return String::new(); + }; + first + .to_uppercase() + .chain(chars.flat_map(char::to_lowercase)) + .collect() +} + +/// Return the dictionary's canonical casing, including the exact input when it is already valid. +fn known_in_any_case(dictionary: &Dictionary, word: &str) -> Option { + let dictionary_word = word.replace('’', "'"); + if dictionary.check(&dictionary_word) { + return Some(word.to_string()); + } + let lower = dictionary_word.to_lowercase(); + let title = titlecase(&dictionary_word); + let upper = dictionary_word.to_uppercase(); + [lower, title, upper] + .into_iter() + .find(|candidate| candidate != word && dictionary.check(candidate)) +} + +fn classify_word(dictionary: &Dictionary, word: &str, ignore_capitalized: bool) -> WordOutcome { + match known_in_any_case(dictionary, word) { + Some(canonical) if canonical == word => WordOutcome::Clean, + Some(canonical) => WordOutcome::Capitalization(canonical), + None if ignore_capitalized && starts_uppercase(word) => WordOutcome::ProperNoun, + None => WordOutcome::Misspelled, + } } impl ColumnValidator for SpellCheck { fn name(&self) -> SharedString { - VALIDATOR_NAME.into() + SPELLING_VALIDATOR_NAME.into() } fn validate( &self, column: &ColumnInfo, - values: &[SharedString], - ) -> Vec<(usize, Severity, SharedString)> { - // Only prose is worth checking; a date or an accession number is not misspelled, and the - // token rules in `checkable` are what keep the rest quiet. + values: ColumnValues<'_>, + ) -> Vec { if !column.settings.spellcheck || !ColumnType::from_declared(column.data_type).is_prose() { return Vec::new(); } - let Ok(dictionary) = self.dictionary.read() else { + let Ok(dictionaries) = self.dictionaries.read() else { return Vec::new(); }; + let Ok(mut checked) = dictionaries.checked.lock() else { + return Vec::new(); + }; + if checked.len() > MAX_CHECKED { + checked.clear(); + } values .iter() - .enumerate() - .filter_map(|(row, value)| { - let mut bad: Vec<&str> = Vec::new(); - for word in words(value, self.ignore_capitalized) { - if !dictionary.check(word) && !bad.contains(&word) { - bad.push(word); + .flat_map(|cell| { + let mut found = Vec::new(); + let mut seen = std::collections::BTreeSet::new(); + for value in cell.parts() { + if !checked.contains_key(value) { + let verdict = dictionaries.check(value, self.ignore_capitalized); + checked.insert(value.to_owned(), verdict); + } + let Some((code, flagged)) = &checked[value] else { + continue; + }; + for (word, outcome) in flagged { + if !seen.insert(word.clone()) { + continue; + } + let (message, target) = match outcome { + WordOutcome::Misspelled => (format!("misspelled: {word}"), ""), + WordOutcome::Capitalization(canonical) => ( + format!("capitalization: “{word}” should be “{canonical}”"), + canonical.as_str(), + ), + WordOutcome::Clean | WordOutcome::ProperNoun => continue, + }; + found.push(diagnostics::ColumnFinding { + row: Some(cell.row), + severity: Severity::Warning, + group: Some(diagnostics::DiagnosticGroup { + key: format!("{:?}", (code, word, target)).into(), + summary: message.clone().into(), + subject: Some(word.clone().into()), + }), + message: message.into(), + }); } } - (!bad.is_empty()).then(|| { - ( - row, - Severity::Warning, - format!("misspelled: {}", bad.join(", ")).into(), - ) - }) + found }) .collect() } @@ -376,13 +691,46 @@ impl ColumnValidator for SpellCheck { mod tests { // Never `use super::*` here — the chained glob would let gpui's `test` macro shadow the // `#[test]` its own expansion emits. See the note in `table`'s `note.rs` test module. - use crate::{SpellCheck, checkable, words}; - use diagnostics::{ColumnInfo, ColumnValidator, Severity}; - use gpui::SharedString; + use crate::{ + DictionarySet, Downloading, EN_CA_AFF, EN_CA_DIC, LoadedDictionary, SpellCheck, checkable, + retain_wanted_downloads, spelling_group_fixes, words, + }; + use diagnostics::{ColumnInfo, ColumnValidator, DATASET_MAIN, GroupMember, Location, Severity}; + use gpui::{SharedString, TestAppContext}; use settings::columns::ColumnSettings; + use spellbook::Dictionary; + + fn dictionary_with_names_ignored(ignore_capitalized: bool) -> SpellCheck { + SpellCheck { + dictionaries: std::sync::Arc::new(std::sync::RwLock::new(DictionarySet { + loaded: vec![LoadedDictionary { + code: "en-CA".to_string(), + dictionary: Dictionary::new(EN_CA_AFF, EN_CA_DIC) + .expect("the embedded en-CA dictionary parses"), + }], + preferred: "en-CA".to_string(), + checked: Default::default(), + })), + ignore_capitalized, + } + } fn dictionary() -> SpellCheck { - SpellCheck::load("en-CA", false).expect("the embedded en-CA dictionary parses") + dictionary_with_names_ignored(false) + } + + #[gpui::test] + fn deselecting_an_in_flight_download_keeps_it_unselected(cx: &mut TestAppContext) { + cx.update(|cx| { + let mut downloading = Downloading::default(); + downloading.active.insert("fr".into()); + downloading.wanted.insert("fr".into()); + cx.set_global(downloading); + retain_wanted_downloads(&[], cx); + let downloading = cx.global::(); + assert!(downloading.active.contains("fr")); + assert!(!downloading.wanted.contains("fr")); + }); } fn findings( @@ -390,7 +738,7 @@ mod tests { data_type: &str, settings: &ColumnSettings, values: &[&str], - ) -> Vec<(usize, Severity, SharedString)> { + ) -> Vec { let values: Vec = values.iter().map(|v| SharedString::from(*v)).collect(); spell.validate( &ColumnInfo { @@ -398,10 +746,18 @@ mod tests { data_type, settings, }, - &values, + diagnostics::ColumnValues::new(&values, ""), ) } + fn small_dictionary(code: &str, entries: &[&str]) -> LoadedDictionary { + let dic = format!("{}\n{}\n", entries.len(), entries.join("\n")); + LoadedDictionary { + code: code.to_string(), + dictionary: Dictionary::new("SET UTF-8\n", &dic).expect("test dictionary parses"), + } + } + #[test] fn the_embedded_dictionary_knows_english() { let spell = dictionary(); @@ -412,9 +768,70 @@ mod tests { &["I did receive the reel", "I did recieve the reel"], ); assert_eq!(found.len(), 1, "only the misspelled row is reported"); - assert_eq!(found[0].0, 1); - assert_eq!(found[0].1, Severity::Warning); - assert_eq!(found[0].2, "misspelled: recieve"); + assert_eq!(found[0].row, Some(1)); + assert_eq!(found[0].severity, Severity::Warning); + assert_eq!(found[0].message, "misspelled: recieve"); + } + + #[test] + fn known_words_in_the_wrong_case_are_capitalization_not_misspellings() { + let found = findings( + &dictionary(), + "", + &ColumnSettings::default(), + &["alice visited Canada"], + ); + assert_eq!(found.len(), 1); + assert_eq!(found[0].severity, Severity::Warning); + assert_eq!( + found[0].message, + "capitalization: “alice” should be “Alice”" + ); + } + + #[test] + fn dictionary_selection_requires_coverage_and_a_clear_lead() { + let dictionaries = DictionarySet { + loaded: vec![ + small_dictionary("en-CA", &["film", "history", "the"]), + small_dictionary("fr", &["film", "histoire", "le"]), + ], + preferred: "en-CA".to_string(), + checked: Default::default(), + }; + assert_eq!( + dictionaries.for_text("the film history").map(|d| &*d.code), + Some("en-CA") + ); + assert_eq!( + dictionaries.for_text("le film histoire").map(|d| &*d.code), + Some("fr") + ); + assert_eq!( + dictionaries + .for_text("Agnès Varda the film history") + .map(|d| &*d.code), + Some("en-CA"), + "unknown title-cased names do not dilute the language evidence" + ); + assert!(dictionaries.for_text("film").is_none()); + assert!(dictionaries.for_text("unknown tokens").is_none()); + } + + #[test] + fn unsupported_non_english_text_is_not_checked_as_english() { + let spell = dictionary(); + let defaults = ColumnSettings::default(); + let french = "bonjour, je suis heureux de vous rencontrer"; + assert!( + findings(&spell, "", &defaults, &["l’enfant"]).is_empty(), + "a common non-English elision is not an English misspelling" + ); + assert!( + findings(&spell, "", &defaults, &[french]).is_empty(), + "reliably identified French text is not checked with an English dictionary: {:?}", + whatlang::detect(french) + ); } /// Which variant ships is a real choice, not a default: SCOWL size 60 carries one spelling per @@ -463,6 +880,10 @@ mod tests { words("don't stop", false).collect::>(), vec!["don't", "stop"] ); + assert_eq!( + words("don’t stop", false).collect::>(), + vec!["don’t", "stop"] + ); assert_eq!( words("'quoted' word", false).collect::>(), vec!["quoted", "word"] @@ -506,15 +927,16 @@ mod tests { let defaults = ColumnSettings::default(); let cell = ["shot on betacam"]; assert_eq!( - findings(&spell, "", &defaults, &cell)[0].2, + findings(&spell, "", &defaults, &cell)[0].message, "misspelled: betacam" ); - spell - .dictionary - .write() - .expect("uncontended") - .add("betacam") - .expect("a bare word is a valid .dic line"); + assert!( + spell + .dictionaries + .write() + .expect("uncontended") + .learn("betacam") + ); assert!(findings(&spell, "", &defaults, &cell).is_empty()); } @@ -522,24 +944,55 @@ mod tests { /// answer is to stop treating a capital letter as a claim about spelling. #[test] fn capitalized_words_are_names_when_the_rule_is_on() { - let names = SpellCheck::load("en-CA", true).expect("parses"); + let names = dictionary_with_names_ignored(true); let defaults = ColumnSettings::default(); // Varda and Anansi are absent from SCOWL, so both flag with the rule off. assert_eq!( findings(&dictionary(), "", &defaults, &["Agnès Varda"]).len(), - 1 + 2 ); assert!(findings(&names, "", &defaults, &["Agnès Varda"]).is_empty()); assert!(findings(&names, "", &defaults, &["Anansi the Spider"]).is_empty()); // A lowercase typo still gets caught, which is the whole point of scoping it to capitals. assert_eq!( - findings(&names, "", &defaults, &["Varda recieved it"])[0].2, + findings(&names, "", &defaults, &["Varda recieved it"])[0].message, "misspelled: recieved" ); assert!(checkable("recieve", true)); assert!(!checkable("Recieve", true)); } + #[gpui::test] + fn grouped_words_offer_one_resolution_for_every_cell(cx: &mut TestAppContext) { + cx.update(|cx| { + cx.set_global(dictionary()); + let members = |text: &str, message: &str| { + [0, 1].map(|row| GroupMember { + location: Location::cell(DATASET_MAIN, row, None, "Title"), + text: text.into(), + message: message.into(), + subject: Some(message.split_whitespace().last().unwrap_or_default().into()), + }) + }; + assert!( + spelling_group_fixes(&members("recieve it", "misspelled: recieve"), cx) + .iter() + .any(|fix| fix.label == "Change all “recieve” to “receive”") + ); + let capitalization = spelling_group_fixes( + &members("alice visited Canada", "capitalization: alice"), + cx, + ); + assert_eq!( + capitalization + .iter() + .map(|fix| fix.label.as_ref()) + .collect::>(), + ["Change all “alice” to “Alice”", "Add “alice” to dictionary"] + ); + }); + } + #[test] fn every_misspelling_in_a_row_is_named_once() { let spell = dictionary(); @@ -549,6 +1002,50 @@ mod tests { &ColumnSettings::default(), &["teh recieve teh reel"], ); - assert_eq!(found[0].2, "misspelled: teh, recieve"); + assert_eq!(found.len(), 2); + assert_eq!(found[0].message, "misspelled: teh"); + assert_eq!(found[1].message, "misspelled: recieve"); + } + + #[test] + fn repeated_words_share_groups_but_not_locations() { + let found = findings( + &dictionary(), + "", + &ColumnSettings::default(), + &["recieve recieve", "recieve"], + ); + assert_eq!(found.len(), 2); + assert_eq!(found[0].group, found[1].group); + assert_eq!(found[0].row, Some(0)); + assert_eq!(found[1].row, Some(1)); + } + + #[test] + fn a_hundred_cells_emit_one_word_group() { + let found = findings( + &dictionary(), + "", + &ColumnSettings::default(), + &["recieve"; 100], + ); + assert_eq!(found.len(), 100); + assert!(found.iter().all(|finding| finding.group == found[0].group)); + } + + #[test] + fn subdelimited_names_are_checked_as_separate_values() { + let spell = dictionary(); + let values = ["receive the reel|recieve the film".into()]; + let found = spell.validate( + &ColumnInfo { + name: "Title", + data_type: "Text", + settings: &ColumnSettings::default(), + }, + diagnostics::ColumnValues::new(&values, "|"), + ); + assert_eq!(found.len(), 1); + assert_eq!(found[0].message, "misspelled: recieve"); } } diff --git a/crates/table/Cargo.toml b/crates/table/Cargo.toml index 83fadc88..5896d37e 100644 --- a/crates/table/Cargo.toml +++ b/crates/table/Cargo.toml @@ -22,3 +22,4 @@ sha2 = "0.10" [dev-dependencies] gpui = { workspace = true, features = ["test-support"] } +clustering = { path = "../clustering" } diff --git a/crates/table/src/agent.rs b/crates/table/src/agent.rs index b45a1140..507a110b 100644 --- a/crates/table/src/agent.rs +++ b/crates/table/src/agent.rs @@ -834,6 +834,7 @@ fn stage_findings( }, source: Source::Validator(AGENT_SOURCE.into()), message: finding.message.into(), + group: None, // A computed finding carries no filing stamp — it is recomputed, not observed once. filed: None, }); @@ -877,7 +878,12 @@ fn stage_findings( /// What the Fixes menu offers for a staged finding — but only while the cell still says what the /// agent judged, because a cell edited since staging is a cell nobody reviewed. -fn offer_staged(location: &Location, text: &str, cx: &App) -> Vec { +fn offer_staged( + location: &Location, + text: &str, + _: Option<&str>, + cx: &App, +) -> Vec { let (Some(row), Some(column)) = (location.row, location.column.clone()) else { return Vec::new(); }; diff --git a/crates/table/src/delegate.rs b/crates/table/src/delegate.rs index c8a528f3..6e7efc4d 100644 --- a/crates/table/src/delegate.rs +++ b/crates/table/src/delegate.rs @@ -433,7 +433,7 @@ impl QrateTableDelegate { .iter() .map(|c| (c.key.clone(), c.name.clone())) .collect(); - diagnostics::Validators::run(&columns, &self.rows, cx); + diagnostics::Validators::run(&columns, &self.rows, &self.row_ids, cx); } /// Every row's text for a data column, in source-row order — the same view a validator gets, @@ -519,7 +519,7 @@ impl QrateTableDelegate { /// through here — inline commit, diagnostic fix, paste, bulk fill — so none of them carries /// undo logic of its own. Cells whose text is unchanged are dropped, so a commit that typed /// nothing doesn't consume an undo. - pub(crate) fn apply_edit(&mut self, cells: Cells) { + pub(crate) fn apply_edit(&mut self, cells: Cells) -> bool { let mut edit = Vec::with_capacity(cells.len()); for (row, col, after) in cells { let Some(before) = self.cell(row, col).cloned() else { @@ -531,7 +531,9 @@ impl QrateTableDelegate { self.set_cell(row, col, after.clone()); edit.push((row, col, before, after)); } + let changed = !edit.is_empty(); self.history.push(Step::Cells(edit)); + changed } /// Reverse the last recorded step. The answer says whether it moved rows; `None` means there diff --git a/crates/table/src/editing.rs b/crates/table/src/editing.rs index 860ac298..4169e373 100644 --- a/crates/table/src/editing.rs +++ b/crates/table/src/editing.rs @@ -125,6 +125,12 @@ pub(crate) fn cancel(delegate: &mut QrateTableDelegate) -> bool { !matches!(std::mem::take(&mut delegate.editing), EditState::Idle) } +pub(crate) enum Committed { + Unchanged, + Cell, + Rename(usize, SharedString), +} + /// Write the editor's current text back into whatever is being edited and leave edit mode. No-op /// if nothing is (e.g. a stray blur, or an edit [`cancel`] has already dropped). /// @@ -133,16 +139,25 @@ pub(crate) fn cancel(delegate: &mut QrateTableDelegate) -> bool { pub(crate) fn commit( delegate: &mut QrateTableDelegate, cx: &mut Context>, -) -> Option<(usize, SharedString)> { +) -> Committed { let value = delegate.editor.read(cx).value(); - let editing = std::mem::take(&mut delegate.editing); - match editing { - EditState::Idle => return None, - EditState::Renaming { col } => return Some((col, value)), - EditState::Editing { row, col } => delegate.apply_edit(vec![(row, col, value)]), + match std::mem::take(&mut delegate.editing) { + EditState::Idle => Committed::Unchanged, + EditState::Renaming { col } => { + if delegate.column_name(col) == value { + Committed::Unchanged + } else { + Committed::Rename(col, value) + } + } + EditState::Editing { row, col } => { + if !delegate.apply_edit(vec![(row, col, value)]) { + return Committed::Unchanged; + } + settings::dirty::mark(settings::dirty::PROJECT_DATA, cx); + Committed::Cell + } } - settings::dirty::mark(settings::dirty::PROJECT_DATA, cx); - None } #[cfg(test)] @@ -203,7 +218,10 @@ mod tests { assert!(editing::cancel(delegate), "there was an edit to abandon"); assert_eq!(delegate.editing, EditState::Idle); - assert!(editing::commit(delegate, cx).is_none()); + assert!(matches!( + editing::commit(delegate, cx), + editing::Committed::Unchanged + )); assert_eq!( delegate .cell(0, 0) @@ -292,7 +310,10 @@ mod tests { let delegate = state.delegate_mut(); delegate.editing = EditState::Editing { row: 0, col: 0 }; - assert!(editing::commit(delegate, cx).is_none()); + assert!(matches!( + editing::commit(delegate, cx), + editing::Committed::Cell + )); assert_eq!( delegate .cell(0, 0) @@ -305,6 +326,26 @@ mod tests { }); } + #[gpui::test] + fn committing_the_existing_column_name_is_unchanged(cx: &mut TestAppContext) { + let state = panel(cx); + let window = cx.windows()[0]; + cx.update_window(window, |_, window, cx| { + state.update(cx, |state, cx| { + let delegate = state.delegate_mut(); + delegate + .editor + .update(cx, |input, cx| input.set_value("Title", window, cx)); + delegate.editing = EditState::Renaming { col: 0 }; + assert!(matches!( + editing::commit(delegate, cx), + editing::Committed::Unchanged + )); + }); + }) + .unwrap(); + } + /// Nothing to abandon has to be reported as such: the Escape handler hands the key on when /// this is false, and swallowing it there is what used to make Escape a dead key mid-edit. #[gpui::test] diff --git a/crates/table/src/file_links.rs b/crates/table/src/file_links.rs index c399db23..8530c4d7 100644 --- a/crates/table/src/file_links.rs +++ b/crates/table/src/file_links.rs @@ -70,7 +70,16 @@ pub fn check(columns: &[ColumnSnapshot], cx: &mut App) { let items: Vec<_> = named .iter() - .flat_map(|column| address_findings(SOURCE.into(), column, missing(index, &column.values))) + .flat_map(|column| { + address_findings( + SOURCE.into(), + column, + missing(index, &column.values) + .into_iter() + .map(Into::into) + .collect(), + ) + }) .collect(); Diagnostics::set(&source(), DATASET_MAIN, items, cx); } diff --git a/crates/table/src/filter.rs b/crates/table/src/filter.rs index f0f73e42..9bb0005e 100644 --- a/crates/table/src/filter.rs +++ b/crates/table/src/filter.rs @@ -7,20 +7,20 @@ //! pushed from the delegate on render and pulled back in the `Change` subscription, so the excluded //! set stays the one answer and nothing has to reconcile two. -use diagnostics::Diagnostics; +use diagnostics::{CheckList, Diagnostics}; use gpui::prelude::FluentBuilder as _; use gpui::{ AnyElement, App, ClickEvent, Context, Entity, InteractiveElement as _, IntoElement, ParentElement as _, SharedString, StatefulInteractiveElement as _, Styled as _, Subscription, - Task, Window, canvas, div, px, + Window, canvas, div, px, }; use gpui_component::menu::ContextMenuExt as _; use gpui_component::{ - ActiveTheme, Icon, IconName, IndexPath, Sizable as _, StyledExt as _, + ActiveTheme, IconName, IndexPath, Sizable as _, StyledExt as _, button::{Button, ButtonVariants as _}, combobox::{Combobox, ComboboxEvent, ComboboxState}, h_flex, - searchable_list::{SearchableListDelegate, SearchableListItem}, + searchable_list::SearchableListItem, table::TableState, }; @@ -31,85 +31,6 @@ use crate::{EditSpawn, TableStateHandle, delegate::QrateTableDelegate, editing:: #[derive(Clone, PartialEq)] pub(crate) struct FilterValue(SharedString); -/// Searchable values with a filter-specific row layout. -struct FilterValues { - items: Vec, - matched_items: Vec, -} - -impl FilterValues { - fn new(items: Vec) -> Self { - Self { - matched_items: items.clone(), - items, - } - } -} - -impl SearchableListDelegate for FilterValues { - type Item = FilterValue; - - fn items_count(&self, _: usize) -> usize { - self.matched_items.len() - } - - fn item(&self, ix: IndexPath) -> Option<&Self::Item> { - self.matched_items.get(ix.row) - } - - fn position(&self, value: &V) -> Option - where - Self::Item: SearchableListItem, - V: PartialEq, - { - self.matched_items - .iter() - .position(|item| item.value() == value) - .map(IndexPath::new) - } - - fn perform_search(&mut self, query: &str, _: &mut Window, _: &mut App) -> Task<()> { - self.matched_items = self - .items - .iter() - .filter(|item| item.matches(query)) - .cloned() - .collect(); - Task::ready(()) - } - - fn render_item( - &self, - _: IndexPath, - item: &Self::Item, - checked: bool, - window: &mut Window, - cx: &mut App, - ) -> Option { - Some( - h_flex() - .w_full() - .items_start() - .gap_1() - .child( - Icon::new(IconName::Check) - .xsmall() - .mt(px(2.)) - .when(!checked, |icon| icon.invisible()), - ) - .child( - div() - .flex_1() - .min_w_0() - .h_6() - .overflow_hidden() - .child(item.render(window, cx)), - ) - .into_any_element(), - ) - } -} - impl SearchableListItem for FilterValue { type Value = SharedString; @@ -232,9 +153,9 @@ fn filter_dropdown( window: &mut Window, cx: &mut App, ) -> AnyElement { - let state: Entity> = + let state: Entity>> = window.use_keyed_state(("col-filter", data_col), cx, |window, cx| { - ComboboxState::new(FilterValues::new(Vec::new()), vec![], window, cx) + ComboboxState::new(CheckList::new(Vec::new()), vec![], window, cx) .multiple(true) .searchable(true) }); @@ -271,7 +192,7 @@ fn filter_dropdown( .collect(); if generation.read(cx) != &delegate.values_generation() { state.update(cx, |state, cx| { - state.set_items(FilterValues::new(values.clone()), window, cx); + state.set_items(CheckList::new(values.clone()), window, cx); }); generation.update(cx, |g, _| *g = delegate.values_generation()); } diff --git a/crates/table/src/lib.rs b/crates/table/src/lib.rs index 76705d1c..da46727f 100644 --- a/crates/table/src/lib.rs +++ b/crates/table/src/lib.rs @@ -95,7 +95,9 @@ pub(crate) fn column_type(delegate: &QrateTableDelegate, col: usize, cx: &App) - /// Re-run every registered validator against the live table. The other half of reloading plugins: /// dropping one clears its findings, but only a run publishes the replacements. +#[track_caller] pub fn revalidate_now(cx: &mut App) { + log::debug!("revalidate requested at {}", std::panic::Location::caller()); let Some(state) = cx .try_global::() .and_then(|h| h.0.upgrade()) @@ -415,6 +417,33 @@ pub fn set_cell_text(location: &diagnostics::Location, text: SharedString, cx: & } } +/// Resolve several diagnostic locations through one table edit. +pub fn set_cell_texts(replacements: Vec<(diagnostics::Location, SharedString)>, cx: &mut App) { + let Some(state) = cx + .try_global::() + .and_then(|h| h.0.upgrade()) + else { + return; + }; + let cells = { + let state = state.read(cx); + let delegate = state.delegate(); + replacements + .into_iter() + .filter_map(|(location, text)| { + delegate + .data_col(location.column.as_ref()?) + .zip(location.row) + .map(|(col, row)| ((row, col), text)) + }) + .collect::>() + .into_iter() + .map(|((row, col), text)| (row, col, text)) + .collect() + }; + write_cells(cells, cx); +} + /// What a command invoked from outside the table acts on: the selected column, or nothing at all. /// /// A bar item has no column under it the way a right-click menu does, so the selection is the only @@ -472,9 +501,13 @@ pub fn save_now(cx: &mut App) { else { return; }; + let started = std::time::Instant::now(); let (headers, row_ids, rows) = state.read(cx).delegate().dataset_snapshot(); match settings::project::save_dataset(&file, &headers, &row_ids, &rows) { - Ok(()) => settings::dirty::clear(settings::dirty::PROJECT_DATA, cx), + Ok(()) => { + log::debug!("saved {} rows in {:?}", rows.len(), started.elapsed()); + settings::dirty::clear(settings::dirty::PROJECT_DATA, cx) + } Err(err) => log::error!("failed to save project data: {err}"), } } diff --git a/crates/table/src/note.rs b/crates/table/src/note.rs index 722f1ed8..3fba52b2 100644 --- a/crates/table/src/note.rs +++ b/crates/table/src/note.rs @@ -16,9 +16,9 @@ use gpui::{ use gpui_component::input::Textarea; use gpui_component::menu::{PopupMenu, PopupMenuItem}; use gpui_component::table::TableState; -use gpui_component::{ActiveTheme as _, h_flex}; +use gpui_component::{ActiveTheme as _, Icon, IconName, h_flex}; -use diagnostics::{Diagnostics, Location, Severity, severity_color}; +use diagnostics::{Diagnostics, Location, Severity, Source, severity_color}; use plugin_api::{ ColumnMapContributions, CommandContext, MenuContributions, MenuTarget, PluginHooks, }; @@ -447,6 +447,7 @@ pub fn menu( ) }; let stored = settings::columns::get(&key, cx).plugins; + let variant_review = settings::columns::get(&key, cx).variant_review; let (freeze_table, rename_table) = (table.clone(), table.clone()); let menu = structural_items(menu, None, Some(col)) .item( @@ -504,6 +505,19 @@ pub fn menu( ) }) }); + let review_key = key.clone(); + let menu = menu.separator().item( + PopupMenuItem::new("Review value variants") + .checked(variant_review) + .on_click(move |_, _, cx| { + settings::columns::update( + &review_key, + |settings| settings.variant_review = !settings.variant_review, + cx, + ); + crate::revalidate_now(cx); + }), + ); let menu = mapping_submenus(&key, menu, window, cx); MenuContributions::for_target(MenuTarget::Column, cx) .into_iter() @@ -542,18 +556,57 @@ pub fn menu( let menu = match target { Target::Cell { row, col } => { - let menu = - diagnostics::spelling::menu(&spell_text, menu, window, cx, move |fixed, cx| { - crate::write_cell(row, col, fixed, cx); - }); - diagnostics::fixes::menu( - &location, - &spell_text, - menu, - window, + // ponytail: submenus cannot live in a scrollable menu, so a long list is capped instead. + const SHOWN: usize = 8; + let mut findings: Vec<_> = Diagnostics::at( + &location.dataset, + location.row, + location.column.as_deref(), cx, - move |fixed, cx| crate::write_cell(row, col, fixed, cx), ) + .filter(|d| d.source != Source::Note) + .cloned() + .collect(); + findings.sort_by(|a, b| (a.severity, &a.message).cmp(&(b.severity, &b.message))); + menu.when(!findings.is_empty(), |menu| { + let label = format!("Problems ({})", findings.len()); + menu.submenu(label, window, cx, move |sub, window, cx| { + let sub = + findings + .iter() + .take(SHOWN) + .fold(sub.max_w(px(360.)), |sub, finding| { + let icon = Icon::new(match finding.severity { + Severity::Error => IconName::CircleX, + Severity::Warning => IconName::TriangleAlert, + Severity::Note => IconName::Info, + }) + .text_color(severity_color(finding.severity, cx)); + let (finding, text) = (finding.clone(), spell_text.clone()); + sub.submenu_with_icon( + Some(icon), + diagnostics::fixes::menu_label(&finding.message), + window, + cx, + move |sub, _, cx| { + diagnostics::fixes::finding_menu( + &finding, + Some(&text), + sub.max_w(px(360.)), + cx, + move |fixed, cx| crate::write_cell(row, col, fixed, cx), + ) + }, + ) + }); + sub.when(findings.len() > SHOWN, |sub| { + sub.separator().label(format!( + "+{} more in the Problems panel", + findings.len() - SHOWN + )) + }) + }) + }) } _ => menu, }; @@ -789,6 +842,7 @@ mod tests { severity: Severity::Note, source: Source::Note, message: "look at this".into(), + group: None, filed: None, }; // A cell note, a whole-row note, and a whole-column note — one per marker site. @@ -813,6 +867,7 @@ mod tests { severity: Severity::Error, source: v.clone(), message: "bad".into(), + group: None, filed: None, }], cx, diff --git a/crates/table/src/panel.rs b/crates/table/src/panel.rs index b8fccd32..fc13db34 100644 --- a/crates/table/src/panel.rs +++ b/crates/table/src/panel.rs @@ -154,16 +154,24 @@ impl TablePanel { return; } let by_enter = matches!(event, InputEvent::PressEnter { .. }); - let rename = table_state.update(cx, |state, cx| { - let rename = editing::commit(state.delegate_mut(), cx); - cx.emit(TableChanged); - cx.notify(); - rename + let committed = table_state.update(cx, |state, cx| { + let committed = editing::commit(state.delegate_mut(), cx); + if matches!(committed, editing::Committed::Cell) { + cx.emit(TableChanged); + cx.notify(); + } + committed }); - if let Some((col, name)) = rename { - crate::structural(crate::Structural::RenameColumn { col, name }, cx); + match committed { + editing::Committed::Unchanged => {} + editing::Committed::Cell => { + this.schedule_revalidate(cx); + this.schedule_autosave(cx); + } + editing::Committed::Rename(col, name) => { + crate::structural(crate::Structural::RenameColumn { col, name }, cx); + } } - this.schedule_revalidate(cx); this.forget_suggestions(cx); // Focus was on the editor, which has just gone away. Hand it back to the grid or // the arrow keys and Enter go nowhere — the `DataTable` key context lives on *its* @@ -171,7 +179,6 @@ impl TablePanel { if by_enter { this.focus_table(window, cx); } - this.schedule_autosave(cx); }, ); @@ -305,15 +312,18 @@ impl TablePanel { let project = cx.global::(); let file = project.file.clone(); if this.loaded_project.as_ref() == Some(&file) { + let started = std::time::Instant::now(); this.state.update(cx, |state, cx| { apply_settings(state.delegate_mut(), cx); state.refresh(cx); cx.emit(TableChanged); cx.notify(); }); + log::debug!("table re-read project settings in {:?}", started.elapsed()); cx.notify(); return; } + log::info!("loading project {}", file.display()); let (headers, row_ids, rows) = ( project.data.headers.clone(), @@ -1252,6 +1262,106 @@ mod tests { }); } + #[gpui::test] + fn a_variant_fix_revalidates_and_undo_restores_the_pair(cx: &mut TestAppContext) { + project_with_notes(cx); + cx.update(|cx| { + use gpui::BorrowAppContext as _; + cx.update_global::(|project, _| { + project.data.rows = vec![vec!["Agnès Varda".into()], vec!["Varda, Agnès".into()]]; + project.data.values.insert( + settings::AUTOSAVE_KEY.into(), + settings::Val::Text("off".into()), + ); + project.data.values.insert( + settings::columns::COLUMN_SETTINGS_KEY.into(), + settings::Val::Text(r#"{"Title":{"variant_review":true}}"#.into()), + ); + }); + let variants = clustering::ValueVariants::default(); + diagnostics::Validators::register(Box::new(variants.clone()), cx); + diagnostics::FixProviders::register( + clustering::VALUE_VARIANTS_NAME, + clustering::variant_fixes, + cx, + ); + cx.set_global(variants); + }); + let (panel, cx) = cx.add_window_view(super::TablePanel::new); + cx.run_until_parked(); + let group = panel.update(cx, |_, cx| { + assert_eq!(Diagnostics::all(cx).len(), 2); + let location = Location::cell(DATASET_MAIN, 0, None, "Title"); + let fix = clustering::variant_fixes(&location, "Agnès Varda", None, cx).remove(0); + crate::write_cell(0, 0, fix.replacement, cx); + Diagnostics::all(cx)[0].group.clone() + }); + cx.run_until_parked(); + panel.update(cx, |panel, cx| { + assert!(Diagnostics::all(cx).is_empty()); + assert_eq!( + panel.state.read(cx).delegate().cell(1, 0).unwrap(), + "Varda, Agnès" + ); + panel.state.update(cx, |state, _| { + assert_eq!(state.delegate_mut().undo(), Some(false)); + assert_eq!( + state.delegate_mut().undo(), + None, + "one fix is one undo step" + ); + }); + crate::revalidate_now(cx); + }); + cx.run_until_parked(); + panel.update(cx, |panel, cx| { + assert_eq!(Diagnostics::all(cx).len(), 2); + assert_eq!(Diagnostics::all(cx)[0].group, group); + assert_eq!( + panel.state.read(cx).delegate().cell(0, 0).unwrap(), + "Agnès Varda" + ); + }); + } + + #[gpui::test] + fn grouped_fixes_write_every_cell_in_one_undo_step(cx: &mut TestAppContext) { + project_with_notes(cx); + cx.update(|cx| { + use gpui::BorrowAppContext as _; + cx.update_global::(|project, _| { + project.data.values.insert( + settings::AUTOSAVE_KEY.into(), + settings::Val::Text("off".into()), + ); + }); + }); + let (panel, cx) = cx.add_window_view(super::TablePanel::new); + panel.update(cx, |panel, cx| { + crate::set_cell_texts( + vec![ + ( + Location::cell(DATASET_MAIN, 0, None, "Title"), + "fixed".into(), + ), + ( + Location::cell(DATASET_MAIN, 1, None, "Title"), + "fixed".into(), + ), + ], + cx, + ); + assert_eq!(panel.state.read(cx).delegate().cell(0, 0).unwrap(), "fixed"); + assert_eq!(panel.state.read(cx).delegate().cell(1, 0).unwrap(), "fixed"); + panel.state.update(cx, |state, _| { + assert_eq!(state.delegate_mut().undo(), Some(false)); + assert_eq!(state.delegate_mut().undo(), None); + assert_eq!(state.delegate().cell(0, 0).unwrap(), "one"); + assert_eq!(state.delegate().cell(1, 0).unwrap(), "two"); + }); + }); + } + /// Escape must clear the note's target before returning focus to the grid. That focus move /// inevitably blurs the input; if the target survived, the blur subscription would overwrite /// the existing note or create the new draft. @@ -1268,6 +1378,7 @@ mod tests { severity: Severity::Note, source: Source::Note, message: "original".into(), + group: None, filed: None, }], cx, diff --git a/docs/columns.md b/docs/columns.md index ba22b2b2..894bee06 100644 --- a/docs/columns.md +++ b/docs/columns.md @@ -22,10 +22,20 @@ the column header, for anyone else who opens the project. ## Spell check -Turn spell check on or off per column, and choose its dictionary language from the more -than 60 available. +Turn spell check on or off per column. The languages ticked in Settings ▸ Spelling take part in +automatic language selection; the first one ticked chooses the preferred regional spelling when +variants exist. + +## Value variants + +Turn on value-variant review for columns where inconsistent displayed forms matter, such as +people, organizations, places, subjects, collection titles, and controlled labels. qrate +suggests similar values already present in that column but never merges them automatically. ## Authority lists For a column checked against an authority, such as subject headings, qrate flags a value that the authority does not recognize and can suggest the closest match as a fix. + +GeoNames needs an activated GeoNames web-services account. If GeoNames rejects the account, +qrate stops the remaining requests and shows one warning. Change the account in Settings to retry. diff --git a/docs/diagnostics.md b/docs/diagnostics.md index 872ef9a4..93b09d4e 100644 --- a/docs/diagnostics.md +++ b/docs/diagnostics.md @@ -5,7 +5,18 @@ the right dock. A finding on a cell also shows as a small marker on that cell in ## What qrate checks -- **Spelling**, in over 60 languages. +- **Spelling**, in over 60 languages. Tick one or more in Settings ▸ Spelling; each value is + checked in whichever ticked language fits it, and changes apply without a restart; short or ambiguous values are left alone instead of being + checked as the wrong language. If only English dictionaries are installed, qrate also skips + reliably identified non-English text and common non-English apostrophe elisions. +- **Capitalization**, when a dictionary knows the word but requires a different case. + Capitalization findings are warnings. The Notes tab is reserved for user notes. +- **Value variants**, for columns where you opt into reviewing inconsistent displayed forms. + This catches punctuation, diacritic, word-order, and close-spelling differences in names, + organizations, places, subjects, titles, and other labels. Similarity is a review hint, not + a claim that two values identify the same entity. + Right-click a column header and select **Review value variants** to enable this check. + Select the checked command again to disable it. - **Date formats**, so a malformed or ambiguous date is caught before export. - **File links**, so a row whose linked file cannot be found is reported instead of silently showing a blank preview. See [Files and photos](files-and-photos.md). @@ -20,14 +31,104 @@ findings each time it runs, and that set replaces what it reported last time. ## The Problems panel -The panel lists every current finding, grouped and sorted for review. Click a finding to -jump to the cell it is about. +Repeated spelling, capitalization, value-variant, date, and authority findings appear in collapsed +groups. +Each group shows an occurrence count. Expand the group to see its locations. +Expanded value-variant groups show the value at each location instead of repeating the summary. +The expansion button supports keyboard activation. Click an occurrence to select its cell, row, or column. +A group header does not select an arbitrary location. Its context menu can resolve all occurrences. + +Findings can describe four scopes: + +| Scope | Location label | Navigation | +|---|---|---| +| Cell | `Row N · Column` | Select the cell | +| Row | `Row N` | Select the row | +| Column | Column name | Select the column | +| Dataset | Dataset name | No cell selection | + +The severity tabs count diagnostic occurrences, not collapsed groups or unique cells. +Two distinct misspelled words in one cell count as two occurrences. +Repeated copies of the same word in one cell count once. +A cell can contain more than one member of a value-variant cluster. + +The source filter changes both the visible findings and the tab counts. +It is a multi-select filter. Uncheck one or more diagnostic sources to hide them. +User notes do not appear as a source because the Notes tab already selects them. +The All, Errors, and Warnings tabs show computed findings only. The Notes tab shows user notes only. +The severity filter changes the visible findings, but each tab keeps its own total. +Notes and validators without group metadata remain separate entries. + +### Producer contracts + +The diagnostics store keeps each finding at its exact location. +The panel groups findings only when a producer supplies `DiagnosticGroup` metadata. +Group identity includes the dataset, source, severity, and producer key. The summary is display text, not identity. + +Column validators return `ColumnFinding`. A row index describes a cell. An absent row index describes the whole column. +Row-wide and dataset-wide producers publish addressed diagnostics directly. +Existing plugin scripts keep their current row-based output format. + +Spelling keys include the selected dictionary and observed token. +Capitalization keys also include the proposed spelling. +Value-variant keys include the column and the sorted set of exact displayed cluster members. +Similarity pairs form links. Connected values appear together for review, but qrate does not +change them until the user chooses a canonical form. +Date keys are the exact rejected value. Authority keys are the rejected value, ignoring case. ## Applying a fix -Some findings offer a suggested correction. Right-click the cell, or open its **Fixes** -menu, and pick a suggestion to apply it. Applying a fix replaces the cell's whole value, so +Some findings offer a suggested correction. Right-click the cell, open **Problems**, choose the +finding, and pick a suggestion. The same items appear when you right-click the finding in the +Problems panel. Long labels are shortened, and a cell with more than eight findings lists the +first eight and points to the Problems panel for the rest. Applying a fix replaces the cell's whole value, so you always know exactly what you are accepting. +Value-variant fixes offer forms that already occur in the same column. qrate never chooses a +canonical form, merges records, or changes every matching row automatically. + +Right-click a spelling or capitalization group to apply one correction to all its occurrences. +The group resolver applies the changes as one undo step. + +Right-click a value-variant group to replace all occurrences with any displayed member. +You can also mark the cluster members as distinct for that column. qrate saves this choice with the project. + +qrate uses the configured subdelimiter to read multiple logical values in one cell. +Validators borrow these values as text slices, so splitting does not allocate a list for each cell. +Value-variant fixes preserve the other logical values and replace the complete cell through the normal edit path. + A fix offered against one version of a cell's text does not apply once that text has changed. This stops a stale suggestion from silently overwriting a newer edit. + +Expanded cell occurrences offer the same spelling and fix menus as the grid. +Row, column, dataset, and group entries do not offer cell fixes. +Each accepted fix uses the existing undoable cell edit path. Validation then refreshes the groups. + +## Ignoring a finding + +Each finding offers two ways to ignore it: + +- **Ignore this occurrence** hides it in this one cell. qrate remembers the row, so the ignore + follows the row when you insert or delete rows above it. +- **Ignore in column** hides every finding with the same key in that column. For spelling, the + key includes the word, so a different misspelling still shows. + +A group row in the Problems panel offers **Ignore ▸ In column** for each column it covers. +After you ignore something, a notice with **Undo** appears for a few seconds. +qrate saves ignores with the project. They never hide findings from another check. + +Turn on **Show ignored** in the Problems panel to see ignored findings dimmed, and right-click one +to **Unignore** it. **Settings ▸ Columns ▸ Ignored problems** lists every ignore that still hides +something. Remove one there, or use **Clear all**. + +## Design credit + +qrate's value-clustering workflow is inspired by OpenRefine's +[Cluster and edit](https://openrefine.org/docs/manual/cellediting#cluster-and-edit) feature and +[clustering methods](https://openrefine.org/docs/technical-reference/clustering-in-depth). +qrate uses an independent implementation for its diagnostics and cataloging workflow. +This acknowledgment does not imply OpenRefine's endorsement. + +OpenRefine publishes its source under BSD-3-Clause and its documentation under CC BY 4.0. +This implementation does not copy OpenRefine source, documentation text, or UI assets. +Any future adaptation must retain the applicable copyright, license, and attribution notices. diff --git a/sample/column_config.csv b/sample/column_config.csv index 04c17005..baf751f7 100644 --- a/sample/column_config.csv +++ b/sample/column_config.csv @@ -1,7 +1,7 @@ -Column Name,Data Type,Description,Authority,Spellcheck,Authority Severity -Digital ID,Filename,"Names the slide's scan in the photos folder — 1 resolves to photos/1.jpg. Typed Filename so a row whose scan is missing is reported instead of quietly showing no image.",,no, -Photographer,Text,"Surname, Given. Checked against Wikidata, as a warning — an unlisted photographer is worth noticing, not worth blocking on.",Wikidata,no,warning -Photographer’s Note,Text,The photographer's own caption. Transcribe it as written rather than correcting it.,,yes, -Original Item,Text,"Physical description of what was scanned, e.g. 35 mm color slide.",,yes, -Original Collection,Text,"The named collection this item belongs to. Spell checking is off: it is a proper name, not prose.",,no, -Repository,Text,"The holding institution. Spell checking is off for the same reason as the collection above.",,no, +Column Name,Data Type,Description,Authority,Spellcheck,Variant Review,Authority Severity +Digital ID,Filename,"Names the slide's scan in the photos folder — 1 resolves to photos/1.jpg. Typed Filename so a row whose scan is missing is reported instead of quietly showing no image.",,no,no, +Photographer,Text,"Surname, Given. Checked against Wikidata, as a warning — an unlisted photographer is worth noticing, not worth blocking on.",Wikidata,no,yes,warning +Photographer’s Note,Text,The photographer's own caption. Transcribe it as written rather than correcting it.,,yes,no, +Original Item,Text,"Physical description of what was scanned, e.g. 35 mm color slide.",,yes,no, +Original Collection,Text,"The named collection this item belongs to. Spell checking is off: it is a proper name, not prose.",,no,yes, +Repository,Text,"The holding institution. Spell checking is off for the same reason as the collection above.",,no,yes,