diff --git a/config_example.toml b/config_example.toml index ba9cb47..d196ecd 100644 --- a/config_example.toml +++ b/config_example.toml @@ -173,7 +173,7 @@ tutorial_seen = false [search] # Start with the fuzzy passes enabled. -fuzzy_default = false +fuzzy_default = true # Ceiling on the fuzzy stages' typo budget. The allowance grows with the # search term, one edit per three characters, up to this value, so 2 # means "1 edit for 3-5 character terms, 2 for anything longer". 0 turns diff --git a/crates/quicksearch-core/examples/contentprobe.rs b/crates/quicksearch-core/examples/contentprobe.rs new file mode 100644 index 0000000..8b93e8a --- /dev/null +++ b/crates/quicksearch-core/examples/contentprobe.rs @@ -0,0 +1,1154 @@ +//! Where the time goes when a narrowed content filter scrubs the index. +//! +//! Narrowing `content_extensions` sets `IndexWork { reconcile_content: true }` +//! (`config::diff_actions`), and `scope::advance` then reads **every** stored +//! row under every root, re-decides each one, and for each row whose verdict +//! changed clears its content and flips `files.content_state`. On a large index +//! that has been reported taking excessively long. This attributes the time to +//! a phase and prices the shapes that could replace it. +//! +//! ```text +//! cargo build -p quicksearch-core --example contentprobe --release +//! ./target/release/examples/contentprobe +//! ``` +//! +//! `QSB_FILES` / `QSB_DIRS` / `QSB_PENDING_EVERY` size the corpus; +//! `QSB_ARMS=plain|keyed|both` picks the key states. A keyed arm is not +//! optional dressing — every page a clear touches is decrypted and +//! re-encrypted, and the two arms have disagreed before (see +//! `db::schema::PAGE_SIZE`). +//! +//! # The sibling probe +//! +//! `pruneprobe` measures the *other* half of the same function: an added ignore +//! pattern, which deletes rows outright. Read them together. The paths differ in +//! how much of what they touch holds a posting — a narrowed content filter is +//! aimed squarely at rows that do (95% of the rewritten rows on this corpus, +//! against 14% of the doomed rows on that one), which is why this is the +//! pathological one and why it moved so much further. +//! +//! The stages are cumulative, so consecutive rows subtract to a phase cost: +//! +//! ```text +//! page read the rows, decide nothing +//! +decide ...and re-run content_extractable per row +//! +state ...and flip content_state, per row +//! +clear(all) ...and clear FTS/text/failure per row +//! +clear(done) ...clearing only rows that can hold one +//! +chunked ...issuing the clears as chunked IN (...) lists +//! +chunk state ...and the state flip chunked too +//! ``` +//! +//! # What it found +//! +//! **The FTS5 tombstone is the whole cost** — 786 ms of a 975 ms pass, against +//! 13 ms for `documents_text` and 1 ms for `failed_files`. Which is why the +//! table splits the three: as one `clear` column they average into a number +//! that suggests fixing the wrong thing. +//! +//! **Statement shape barely matters, and reading it wrong is easy.** Chunking +//! the clears takes `fts` from 786 ms to 19 ms — and puts 848 ms into `commit`, +//! for a 6% total. The work did not go away; SQLite spilled it to whichever +//! statement was executing when the page cache filled. Only `total` is safe to +//! compare across shapes. `+chunk state` is kept as the demonstration: it moves +//! the same ~850 ms onto an `UPDATE` that the query plans above show is a plain +//! rowid seek. +//! +//! **`deletemerge` is the lever**, and it is worth 6-7x. The knob sweep is the +//! table to read; `file_handling::fts_begin_tombstone_burst` carries the +//! result and the reasoning, and now ships it — so `live` lands far under every +//! stage rather than on `+clear(all)`. +//! +//! Every stage runs against a byte-identical copy of one seeded index rather +//! than a fresh seed: FTS5 segment layout is most of what decides clear cost, +//! and reseeding would let it drift between the rows of the table. + +mod common; + +use std::path::Path; +use std::time::{Duration, Instant}; + +use rusqlite::Connection; + +use quicksearch_core::config::Config; +use quicksearch_core::db::{self, repo}; +use quicksearch_core::extract::Registry; +use quicksearch_core::file_handling::{content_extractable, ExtractCursor}; +use quicksearch_core::scope::{self, WorkCursor}; +use quicksearch_core::testutil::{self, Arm, SeedSpec}; + +use common::Io; + +/// The root every `seed_index` row hangs under; it need not exist on disk, +/// because nothing in the content re-decision stats a stored path — +/// `content_extractable` reads the extension off the string and the MIME off +/// the row. +const ROOT: &str = "/seed"; + +/// The corpus's extensions and the MIME each carries. +/// +/// **Eleven entries, and the count is deliberate.** `seed_index` assigns +/// extensions by `i % len` and directories by `i % dirs`, so a length sharing a +/// factor with `dirs` (2000 by default) would confine each extension to a +/// fraction of the directories — the rewritten rows would arrive in a few dense +/// runs instead of spread through the keyspace, which is the easy case for page +/// locality and would flatter every figure. 11 is coprime with 2000, so every +/// directory holds every extension. `main` asserts it. +const EXT_MIX: &[(&str, &str)] = &[ + ("txt", "text/plain"), + ("jpg", "image/jpeg"), + ("png", "image/png"), + ("md", "text/markdown"), + ("zip", "application/zip"), + ("mp4", "video/mp4"), + ("pdf", "application/pdf"), + ("gif", "image/gif"), + ("bin", "application/octet-stream"), + ("mov", "video/quicktime"), + ("ttf", "font/ttf"), +]; + +/// Which of [`EXT_MIX`] an extractor claims, and therefore which rows are +/// seeded with a posting: 3 of 11, so 3/11 of the corpus is searchable by +/// content. +/// +/// `main` checks this against `Registry::default_set()` rather than trusting +/// it. Everything below hard-codes these three — [`NARROWED`] and the fraction +/// sweep are written in terms of them — so an extractor gaining or losing a +/// MIME must fail loudly here rather than quietly reshape every figure. +const EXTRACTABLE: [&str; 3] = ["txt", "md", "pdf"]; + +/// The filter the stage table narrows *to*, from an unfiltered index. It keeps +/// `txt` and drops `md` and `pdf`, so two of the three extractable extensions +/// lose their content: 2/11 of all rows are rewritten and 2/3 of the postings +/// are tombstoned. Substantial without being total — a filter that takes every +/// posting is a different problem, and the fraction sweep at the end covers it. +const NARROWED: &[&str] = &["txt"]; + +/// The `files.name` test matching the rows [`NARROWED`] withdraws content from, +/// for the census that reports what the stage table is about to do. +const NARROWED_AWAY: &str = "(name LIKE '%.md' OR name LIKE '%.pdf')"; + +/// Greatest common divisor, for the stride assertion in `main`. +fn gcd(a: usize, b: usize) -> usize { + if b == 0 { + a + } else { + gcd(b, a % b) + } +} + +/// Rows per page, matching `processing.batch_size`'s default — the figure +/// `scope::advance` runs at. +const PAGE: i64 = 500; + +/// Ids per `IN (...)` list, matching `repo::DELETE_IDS_CHUNK`. +const CHUNK: usize = 512; + +/// Output leaf pages per `'merge'` call, matching `FINALIZE_MERGE_PAGES`. +const MERGE_PAGES: i64 = 1000; + +// --------------------------------------------------------------------------- +// Stages +// --------------------------------------------------------------------------- + +/// Cumulative slices of the re-decision. Each does everything the one before +/// it does. +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +enum Stage { + Page, + Decide, + State, + ClearAll, + ClearDone, + Chunked, + ChunkedState, +} + +impl Stage { + const ALL: [Stage; 7] = [ + Stage::Page, + Stage::Decide, + Stage::State, + Stage::ClearAll, + Stage::ClearDone, + Stage::Chunked, + Stage::ChunkedState, + ]; + + fn label(self) -> &'static str { + match self { + Stage::Page => "page", + Stage::Decide => "+decide", + Stage::State => "+state", + Stage::ClearAll => "+clear(all)", + Stage::ClearDone => "+clear(done)", + Stage::Chunked => "+chunked", + Stage::ChunkedState => "+chunk state", + } + } + + fn tag(self) -> &'static str { + match self { + Stage::Page => "page", + Stage::Decide => "decide", + Stage::State => "state", + Stage::ClearAll => "clear-all", + Stage::ClearDone => "clear-done", + Stage::Chunked => "chunked", + Stage::ChunkedState => "chunked-state", + } + } + + /// Whether the re-decision runs at all. + fn decides(self) -> bool { + self >= Stage::Decide + } + + /// Whether `files.content_state` is written. + fn writes_state(self) -> bool { + self >= Stage::State + } + + /// Whether the row's content is cleared alongside the state flip. + fn clears(self) -> bool { + self >= Stage::ClearAll + } + + /// Whether the clear is narrowed to rows whose stored state says they can + /// hold something to clear. + fn clear_filtered(self) -> bool { + self >= Stage::ClearDone + } + + /// Whether the clears go out as chunked `IN (...)` lists instead of one + /// bound statement per row. + fn chunked_clears(self) -> bool { + self >= Stage::Chunked + } + + /// Whether the `content_state` flip is chunked too. Kept as its own step + /// because it does not behave like the clears — see the plan `main` prints. + fn chunked_state(self) -> bool { + self >= Stage::ChunkedState + } +} + +/// What one stage cost, split by phase. +#[derive(Default)] +struct Timing { + page: Duration, + decide: Duration, + state: Duration, + /// The three clears, apart. They are one phase conceptually and three very + /// different pieces of work: an FTS5 contentless delete writes a tombstone + /// into every segment covering the row's origin, a `documents_text` delete + /// frees a compressed body's overflow pages, and a `failed_files` delete is + /// a seek into a table that is nearly always empty. + fts: Duration, + text: Duration, + failed: Duration, + commit: Duration, + total: Duration, + examined: usize, + /// Rows whose content was withdrawn. + to_na: usize, + /// Rows put back in the pending queue. + to_pending: usize, + /// Bytes the block layer served this pass; the arm was just copied, so a + /// figure near zero means the whole working set was already in memory and + /// every duration below is CPU and page-cache work. + read_bytes: u64, + misses: i64, +} + +// --------------------------------------------------------------------------- +// The measured loop +// --------------------------------------------------------------------------- + +fn placeholders(n: usize) -> String { + let mut s = String::with_capacity(n * 2); + for i in 0..n { + if i > 0 { + s.push(','); + } + s.push('?'); + } + s +} + +/// When the pass commits. +#[derive(Clone, Copy)] +enum Commit { + /// One transaction per page — what ships. + PerPage, + /// One per `n` pages; the sweep's variable. + Pages(usize), + /// One per elapsed slice, which is what `scope::advance` already uses as + /// its unit of interruptible work. + Slice(Duration), +} + +impl Commit { + fn due(self, pages_open: usize, since: Instant) -> bool { + match self { + Commit::PerPage => true, + Commit::Pages(n) => pages_open >= n, + Commit::Slice(d) => since.elapsed() >= d, + } + } +} + +/// The ids one page decided to write, split by what each one needs. +#[derive(Default)] +struct Pending { + /// Rows moving to `STATE_NA`. + to_na: Vec, + /// Rows moving to `STATE_PENDING`. + to_pending: Vec, + /// Of the two above, those whose *stored* state was `STATE_DONE` and so + /// can hold a posting and a stored body. + had_posting: Vec, + /// ...and those whose stored state was `STATE_FAILED`, the only ones that + /// can hold a `failed_files` row. + had_failure: Vec, + /// Every id above, in the order the shipped per-row loop visits them. + all: Vec, +} + +impl Pending { + fn clear(&mut self) { + self.to_na.clear(); + self.to_pending.clear(); + self.had_posting.clear(); + self.had_failure.clear(); + self.all.clear(); + } + + fn is_empty(&self) -> bool { + self.all.is_empty() + } + + /// Ascending, which for these ids is rowid order — see [`Knobs::sort_ids`]. + fn sort(&mut self) { + for list in [ + &mut self.to_na, + &mut self.to_pending, + &mut self.had_posting, + &mut self.had_failure, + &mut self.all, + ] { + list.sort_unstable(); + } + } +} + +/// Levers over FTS5's tombstone machinery, applied for the length of the pass. +/// +/// Neither changes what the index ends up holding — a tombstone is a tombstone +/// and the final `'merge'` reclaims them either way — so both are free to try. +#[derive(Clone, Copy, Default)] +struct Knobs { + /// Set FTS5's `deletemerge` to 0 for the pass, restoring it after. + /// + /// It defaults to 10, meaning "once a level is 10% tombstones, merge it". + /// During a mass withdrawal that threshold is crossed early and then + /// repeatedly, and each crossing rewrites a whole level of the full-text + /// index *inline with the scan* (`fts5IndexFindDeleteMerge`, reached from + /// `fts5IndexAutomerge` — every contentless delete counts into the same + /// write-counter that drives it). Off, the tombstones simply accumulate + /// and the merge at the end does the consolidation once. + no_deletemerge: bool, + /// Sort each page's ids ascending before deleting. + /// + /// The scan serves rows in `(parent, name)` order, which is uncorrelated + /// with rowid, so `fts5StorageContentlessDelete`'s `%_docsize` lookup lands + /// somewhere new every time. Sorting restores locality *within* a page. + /// It cannot restore it across pages — only scanning by rowid could, which + /// is a change to the scan and not to this. + sort_ids: bool, +} + +impl Knobs { + fn label(self) -> String { + match (self.no_deletemerge, self.sort_ids) { + (false, false) => "as shipped".into(), + (true, false) => "deletemerge off".into(), + (false, true) => "rowid-sorted".into(), + (true, true) => "both".into(), + } + } + + /// FTS5 persists `deletemerge` in `%_config`, so this has to be put back — + /// the arm is discarded either way, but a leaked setting would make any + /// later reading of the same file mean something else. + fn apply(self, conn: &Connection) { + if self.no_deletemerge { + set_deletemerge(conn, 0); + } + } + + fn restore(self, conn: &Connection) { + if self.no_deletemerge { + set_deletemerge(conn, FTS5_DEFAULT_DELETEMERGE); + } + } +} + +/// FTS5's own default, from `FTS5_DEFAULT_DELETE_AUTOMERGE` in the amalgamation. +const FTS5_DEFAULT_DELETEMERGE: i64 = 10; + +fn set_deletemerge(conn: &Connection, percent: i64) { + conn.execute( + "INSERT INTO searchabletext(searchabletext, rank) VALUES('deletemerge', ?1)", + [percent], + ) + .expect("set deletemerge"); +} + +/// Page the index the way `scope::advance` does, doing `stage`'s share of the +/// work and timing each phase separately. +/// +/// The writes are spelled out here rather than routed through `repo` so that +/// the per-row and chunked variants can be compared in one run — and so this +/// keeps measuring the same thing after `repo` is reshaped around whichever +/// wins. +fn run_stage( + conn: &Connection, + config: &Config, + registry: &Registry, + stage: Stage, + commit: Commit, +) -> Timing { + run_stage_with(conn, config, registry, stage, commit, Knobs::default()) +} + +fn run_stage_with( + conn: &Connection, + config: &Config, + registry: &Registry, + stage: Stage, + commit: Commit, + knobs: Knobs, +) -> Timing { + let range = ExtractCursor::for_root(ROOT); + let max_size = config.processing.maximum_text_file_size; + + let mut t = Timing::default(); + knobs.apply(conn); + let io_before = Io::read(); + let (_, misses_before) = testutil::cache_stats(conn); + let started = Instant::now(); + + let mut after = (range.lo.clone(), String::new()); + let mut pending = Pending::default(); + // `unchecked_transaction` borrows shared, which is what lets one end and + // the next begin inside the loop — the same trick `testutil::seed_index` + // uses for `commit_every`. + let mut tx = conn.unchecked_transaction().expect("begin"); + let mut pages_open = 0usize; + let mut tx_since = Instant::now(); + loop { + let at = Instant::now(); + let rows = repo::rows_in_range_page(&tx, &after.0, &after.1, &range.hi, PAGE) + .expect("read a page"); + t.page += at.elapsed(); + let Some(last) = rows.last() else { break }; + after = (last.parent.clone(), last.name.clone()); + pages_open += 1; + t.examined += rows.len(); + + if !stage.decides() { + continue; + } + + // `scope::apply_page`'s decision, verbatim. `restore_text` is false + // here — this probe narrows the filter and nothing else — so the + // second arm reduces to "wants content but has none". + let at = Instant::now(); + pending.clear(); + for row in &rows { + let path = Path::new(&row.path); + let wants = row.size <= max_size + && content_extractable(path, row.mime.as_deref(), config, registry); + if !wants && row.content_state != repo::STATE_NA { + pending.to_na.push(row.id); + } else if wants && row.content_state == repo::STATE_NA { + pending.to_pending.push(row.id); + } else { + continue; + } + pending.all.push(row.id); + if row.content_state == repo::STATE_DONE { + pending.had_posting.push(row.id); + } else if row.content_state == repo::STATE_FAILED { + pending.had_failure.push(row.id); + } + } + if knobs.sort_ids { + pending.sort(); + } + t.decide += at.elapsed(); + t.to_na += pending.to_na.len(); + t.to_pending += pending.to_pending.len(); + + if stage.writes_state() && !pending.is_empty() { + write_page(&tx, stage, &pending, &mut t); + } + + if commit.due(pages_open, tx_since) { + let at = Instant::now(); + tx.commit().expect("commit"); + t.commit += at.elapsed(); + tx = conn.unchecked_transaction().expect("begin"); + pages_open = 0; + tx_since = Instant::now(); + } + } + let at = Instant::now(); + tx.commit().expect("commit"); + t.commit += at.elapsed(); + + t.total = started.elapsed(); + t.read_bytes = Io::read().since(&io_before).read_bytes; + let (_, misses_after) = testutil::cache_stats(conn); + t.misses = misses_after - misses_before; + knobs.restore(conn); + t +} + +/// Apply one page's decisions, in `stage`'s shape, timing the clear and the +/// state flip apart. +fn write_page(tx: &Connection, stage: Stage, pending: &Pending, t: &mut Timing) { + // Which ids the clear is offered. Unfiltered is what ships: every flipped + // row, whatever its stored state said about whether it could hold anything. + let (fts_ids, failed_ids): (&[i64], &[i64]) = if stage.clear_filtered() { + (&pending.had_posting, &pending.had_failure) + } else { + (&pending.all, &pending.all) + }; + + // The clears. The shipped shape is `remove_content_for_id` followed by + // `set_state_clearing_failure`'s `failed_files` sweep, one bound statement + // at a time — prepared through the connection's cache, as `repo::exec` + // does, since preparing afresh per row would price a mistake the product + // does not make. + if stage.clears() { + // Each target timed on its own, and each loop run to completion before + // the next starts: interleaving them the way `remove_content_for_id` + // does would charge whichever statement happened to be executing when + // the page cache spilled. + let at = Instant::now(); + if stage.chunked_clears() { + chunked(tx, "DELETE FROM searchabletext WHERE rowid IN", fts_ids); + } else { + for id in fts_ids { + one(tx, "DELETE FROM searchabletext WHERE rowid = ?1", *id); + } + } + t.fts += at.elapsed(); + + let at = Instant::now(); + if stage.chunked_clears() { + chunked(tx, "DELETE FROM documents_text WHERE file_id IN", fts_ids); + } else { + for id in fts_ids { + one(tx, "DELETE FROM documents_text WHERE file_id = ?1", *id); + } + } + t.text += at.elapsed(); + + let at = Instant::now(); + if stage.chunked_clears() { + chunked(tx, "DELETE FROM failed_files WHERE file_id IN", failed_ids); + } else { + for id in failed_ids { + one(tx, "DELETE FROM failed_files WHERE file_id = ?1", *id); + } + } + t.failed += at.elapsed(); + } + + let at = Instant::now(); + for (state, ids) in [ + (repo::STATE_NA, &pending.to_na), + (repo::STATE_PENDING, &pending.to_pending), + ] { + if stage.chunked_state() { + chunked_state(tx, state, ids); + } else { + for id in ids { + tx.prepare_cached("UPDATE files SET content_state = ?1 WHERE id = ?2") + .and_then(|mut s| s.execute(rusqlite::params![state, id])) + .expect("flip content_state"); + } + } + } + t.state += at.elapsed(); +} + +fn one(tx: &Connection, sql: &str, id: i64) { + tx.prepare_cached(sql) + .and_then(|mut s| s.execute([id])) + .expect("per-row write"); +} + +/// `sql` is the statement up to and including `IN`; the list follows. +fn chunked(tx: &Connection, sql: &str, ids: &[i64]) { + for chunk in ids.chunks(CHUNK) { + let full = format!("{} ({})", sql, placeholders(chunk.len())); + tx.prepare_cached(&full) + .and_then(|mut s| s.execute(rusqlite::params_from_iter(chunk.iter()))) + .expect("chunked write"); + } +} + +fn chunked_state(tx: &Connection, state: i64, ids: &[i64]) { + for chunk in ids.chunks(CHUNK) { + let sql = format!( + "UPDATE files SET content_state = ?1 WHERE id IN ({})", + placeholders(chunk.len()) + ); + let params = std::iter::once(&state).chain(chunk.iter()); + tx.prepare_cached(&sql) + .and_then(|mut s| s.execute(rusqlite::params_from_iter(params))) + .expect("chunked state flip"); + } +} + +/// How much data FTS5 is holding — the only quiescence signal that works. +/// +/// `sqlite3_changes()` after a `'merge'` does **not** report whether the merge +/// did any work: measured in `pruneprobe`, it reports non-zero forever, so a +/// `while changes() != 0` loop never terminates. Watching `%_data` shrink and +/// stop is what actually detects a consolidated index. +fn fts_data_rows(conn: &Connection) -> i64 { + conn.query_row("SELECT COUNT(*) FROM searchabletext_data", [], |r| r.get(0)) + .unwrap_or(-1) +} + +/// Merge until `%_data` stops moving, and report what that took. +/// +/// The **positive** argument is deliberate: negative picks +/// `fts5IndexOptimizeStruct` instead, which is a different algorithm and a +/// documented trap (`file_handling::records::fts_finalize_after_text_indexing`). +fn merge_to_quiescence(conn: &Connection) -> (Duration, u32, i64, i64) { + let before = fts_data_rows(conn); + let started = Instant::now(); + let mut rounds = 0; + let mut last = before; + loop { + conn.execute( + "INSERT INTO searchabletext(searchabletext, rank) VALUES('merge', ?1)", + [MERGE_PAGES], + ) + .expect("merge"); + rounds += 1; + let now = fts_data_rows(conn); + if now == last || rounds > 500 { + break; + } + last = now; + } + (started.elapsed(), rounds, before, last) +} + +// --------------------------------------------------------------------------- +// Corpus and arms +// --------------------------------------------------------------------------- + +fn env_usize(key: &str, default: usize) -> usize { + std::env::var(key) + .ok() + .and_then(|v| v.trim().parse().ok()) + .unwrap_or(default) +} + +fn spec() -> SeedSpec { + SeedSpec { + files: env_usize("QSB_FILES", 200_000), + dirs: env_usize("QSB_DIRS", 2_000), + // **One, and it has to be.** With a mix, extractability is decided by + // the row's MIME, and `seed_index` intersects that with this stride. + // Any other value would leave rows that an extractor claims sitting at + // `STATE_NA` — a corpus born disagreeing with its own configuration, + // whose first reconcile repairs the seed instead of applying the edit. + content_every: 1, + // One in twenty extractable rows never got its content: the residue an + // interrupted run leaves. These are the rows the shipped code clears + // FTS, stored text and a failure record for, all three of which are + // provably absent — `+clear(done)` is what prices that. + pending_every: env_usize("QSB_PENDING_EVERY", 20), + ext_mix: EXT_MIX, + // Depth is most of what decides stored row width, and therefore how + // many pages the scan reads — see `SeedSpec::dir_depth`. + dir_depth: env_usize("QSB_DIR_DEPTH", 6).max(2), + // A real run commits in slices, and each commit flushes FTS5's hash to + // its own segment. Seeding in one transaction would leave a single + // segment and understate every clear cost below. + commit_every: 500, + ..SeedSpec::default() + } +} + +fn config_with(extensions: &[&str]) -> Config { + let mut config = Config::default(); + config.paths.indexing_paths = vec![ROOT.to_string()]; + config.indexing.content_extensions = extensions.iter().map(|e| e.to_string()).collect(); + config.processing.batch_size = PAGE as usize; + config +} + +/// A byte-identical copy of `master`, under its own scratch directory. +/// +/// `seed_index` ends with a TRUNCATE checkpoint, so the one file holds the +/// whole index and there is no `-wal` to carry across. +fn clone_arm(master: &Arm, tag: &str) -> Arm { + let path = testutil::scratch_db(tag); + std::fs::copy(&master.path, &path).expect("copy the seeded index"); + Arm { + what: master.what.clone(), + keyed: master.keyed, + pgsz: master.pgsz, + page_size: master.page_size, + hmac: master.hmac, + path, + seeded_in: Duration::ZERO, + } +} + +/// The coordinator's own writer profile, which is what the reconcile actually +/// runs on — `PRAGMAS_INCREMENTAL`, a 4 MiB page cache. Opening this +/// `open_existing(.., true)` instead would measure `PRAGMAS_FAST`'s 8 MiB and +/// quietly flatter every figure below. +fn open(arm: &Arm) -> Connection { + arm.with_key(|| { + db::open::open_incremental_writer(&arm.path.to_string_lossy()).expect("open the copy") + }) +} + +/// What `PRAGMAS_INCREMENTAL` gives the reconcile today, in MiB. +const SHIPPED_MIB: i64 = 4; + +fn set_cache(conn: &Connection, mib: i64) { + conn.execute_batch(&format!("PRAGMA cache_size = -{};", mib * 1024)) + .expect("cache_size"); +} + +fn count(conn: &Connection, sql: &str) -> i64 { + conn.query_row(sql, [], |r| r.get(0)).unwrap_or(-1) +} + +// --------------------------------------------------------------------------- + +fn main() { + let spec = spec(); + let arms: Vec = match std::env::var("QSB_ARMS").as_deref() { + Ok("plain") => vec![false], + Ok("keyed") => vec![true], + _ => vec![false, true], + }; + let registry = Registry::default_set(); + + // The corpus's premise, checked rather than trusted. + let claimed: Vec<&str> = EXT_MIX + .iter() + .filter(|(_, mime)| registry.supports(mime)) + .map(|(ext, _)| *ext) + .collect(); + assert_eq!( + claimed, EXTRACTABLE, + "the registry no longer claims exactly the extensions this probe is \ + written around; NARROWED and the fraction sweep name them by hand" + ); + assert_eq!( + gcd(EXT_MIX.len(), spec.dirs), + 1, + "the extension stride ({}) shares a factor with the directory stride \ + ({}), so each extension reaches only some of the directories — the \ + rewritten rows would cluster instead of spreading. See EXT_MIX", + EXT_MIX.len(), + spec.dirs + ); + + println!( + "corpus: {} files across {} dirs, {} of {} extensions extractable ({}), \ + 1 in {} of those left pending; narrowing to {:?}", + spec.files, + spec.dirs, + EXTRACTABLE.len(), + EXT_MIX.len(), + EXTRACTABLE.join("/"), + spec.pending_every, + NARROWED + ); + + for keyed in arms { + let label = if keyed { "keyed" } else { "plain" }; + let master = Arm::seed(label, &format!("content-{}", label), keyed, &spec); + println!( + "\n=== {} === seeded in {:.1}s, {}", + label, + master.seeded_in.as_secs_f64(), + common::mib(master.size_bytes()) + ); + + // What the narrowing is worth, read off the index rather than assumed. + { + let conn = open(&master); + let total = count(&conn, "SELECT COUNT(*) FROM files"); + let done = count(&conn, "SELECT COUNT(*) FROM files WHERE content_state = 1"); + let pending = count(&conn, "SELECT COUNT(*) FROM files WHERE content_state = 0"); + let na = count(&conn, "SELECT COUNT(*) FROM files WHERE content_state = 3"); + let fts = count(&conn, "SELECT COUNT(*) FROM searchabletext"); + assert_eq!(fts, done, "a posting exists exactly for content_state = 1"); + assert!( + pending > 0, + "no pending residue — QSB_PENDING_EVERY is 0, and +clear(done) \ + has nothing to skip" + ); + // The rows the narrowing rewrites: everything not already NA whose + // extension leaves the filter. + let rewritten = count( + &conn, + &format!( + "SELECT COUNT(*) FROM files WHERE content_state != 3 AND {}", + NARROWED_AWAY + ), + ); + let with_posting = count( + &conn, + &format!( + "SELECT COUNT(*) FROM files WHERE content_state = 1 AND {}", + NARROWED_AWAY + ), + ); + assert!( + rewritten > 0, + "the narrowing rewrites nothing — seed_index's naming moved" + ); + assert!( + with_posting < rewritten, + "every rewritten row holds a posting, so +clear(done) has \ + nothing to skip and the stage cannot answer anything" + ); + println!( + " {} rows: {} done, {} pending, {} n/a. {} rewritten by the \ + narrowing ({:.0}%), of which {} hold a posting ({:.0}%) — \ + the other {} are cleared today for nothing", + total, + done, + pending, + na, + rewritten, + 100.0 * rewritten as f64 / total as f64, + with_posting, + 100.0 * with_posting as f64 / rewritten as f64, + rewritten - with_posting, + ); + // Why the state flip is listed apart from the clears. A chunked + // `IN (...)` is an unambiguous win for a `DELETE`, and the same + // shape applied to the `UPDATE` is not — the plans say which of + // them seeks and which scans, and no timing can be read without + // knowing that. + for (what, sql) in [ + ( + "delete, per row ", + "DELETE FROM searchabletext WHERE rowid = 1".to_string(), + ), + ( + "delete, chunked ", + "DELETE FROM searchabletext WHERE rowid IN (1,2,3)".to_string(), + ), + ( + "state, per row ", + "UPDATE files SET content_state = 3 WHERE id = 1".to_string(), + ), + ( + "state, chunked ", + "UPDATE files SET content_state = 3 WHERE id IN (1,2,3)".to_string(), + ), + ] { + let plan: Vec = conn + .prepare(&format!("EXPLAIN QUERY PLAN {}", sql)) + .and_then(|mut s| { + s.query_map([], |r| r.get::<_, String>(3))? + .collect::>>() + }) + .unwrap_or_else(|e| vec![format!("unavailable: {}", e)]); + println!(" plan {}: {}", what, plan.join(" | ")); + } + println!( + " fts: {} %_data rows ({}), {} %_idx, {} %_docsize", + count(&conn, "SELECT COUNT(*) FROM searchabletext_data"), + common::mib( + count( + &conn, + "SELECT COALESCE(SUM(pgsize), 0) FROM dbstat \ + WHERE name LIKE 'searchabletext%'" + ) + .max(0) as u64 + ), + count(&conn, "SELECT COUNT(*) FROM searchabletext_idx"), + count(&conn, "SELECT COUNT(*) FROM searchabletext_docsize"), + ); + } + + let config = config_with(NARROWED); + let header = || { + println!( + "\n {:<16} {:>6} {:>6} {:>6} {:>7} {:>6} {:>6} {:>7} {:>7} {:>8} {:>7}", + "stage", + "page", + "decide", + "state", + "fts", + "text", + "failed", + "commit", + "total", + "misses", + "written" + ); + }; + let row = |name: &str, t: &Timing| { + let ms = |d: Duration| d.as_secs_f64() * 1000.0; + println!( + " {:<16} {:>6.0} {:>6.0} {:>6.0} {:>7.0} {:>6.0} {:>6.0} {:>7.0} {:>7.0} {:>8} {:>7}", + name, + ms(t.page), + ms(t.decide), + ms(t.state), + ms(t.fts), + ms(t.text), + ms(t.failed), + ms(t.commit), + ms(t.total), + t.misses, + t.to_na + t.to_pending, + ); + }; + + header(); + for stage in Stage::ALL { + let arm = clone_arm(&master, &format!("content-{}-{}", label, stage.tag())); + let conn = open(&arm); + let t = run_stage(&conn, &config, ®istry, stage, Commit::PerPage); + row(stage.label(), &t); + // Consolidation, priced apart: today one 1000-page `'merge'` runs + // and the scrub stops, whatever it left behind. + if stage.clears() { + let (took, rounds, before, after) = merge_to_quiescence(&conn); + println!( + " {:<16} merge: {:.0} ms over {} rounds, %_data {} -> {} rows", + "", + took.as_secs_f64() * 1000.0, + rounds, + before, + after + ); + } + drop(conn); + arm.discard(); + } + + // Is the cost priced per commit or per row? Today's + // one-transaction-per-page is the first line of each block; if it + // collapses as pages are folded together, commit cadence is the lever + // and the write shape above is not. Both shapes are swept, because the + // two fixes are independent and either could subsume the other. + for (what, stage) in [ + ("as shipped", Stage::ClearAll), + ("+chunked", Stage::Chunked), + ] { + println!("\n transaction size, at {}:", what); + header(); + for pages in [1usize, 8, 64, 512] { + let arm = clone_arm( + &master, + &format!("content-{}-{}-tx{}", label, stage.tag(), pages), + ); + let conn = open(&arm); + let t = run_stage(&conn, &config, ®istry, stage, Commit::Pages(pages)); + row(&format!("{} page/tx", pages), &t); + drop(conn); + arm.discard(); + } + let arm = clone_arm(&master, &format!("content-{}-{}-slice", label, stage.tag())); + let conn = open(&arm); + let t = run_stage( + &conn, + &config, + ®istry, + stage, + Commit::Slice(scope::SLICE), + ); + row(&format!("{:?} slice", scope::SLICE), &t); + drop(conn); + arm.discard(); + } + + // FTS5's own machinery, which is where the time actually is once the + // statement shape stops mattering. Both knobs leave the index in the + // same state — see `Knobs` — so either is free to adopt if it pays. + // Run at the chunked shape, so what is left to see is FTS5's and not + // the per-row overhead's. + println!("\n FTS5 knobs, at +chunked (+ merge to quiescence):"); + header(); + for knobs in [ + Knobs::default(), + Knobs { + no_deletemerge: true, + ..Knobs::default() + }, + Knobs { + sort_ids: true, + ..Knobs::default() + }, + Knobs { + no_deletemerge: true, + sort_ids: true, + }, + ] { + let arm = clone_arm( + &master, + &format!("content-{}-k{}", label, knobs.label().len()), + ); + let conn = open(&arm); + let t = run_stage_with( + &conn, + &config, + ®istry, + Stage::Chunked, + Commit::Slice(scope::SLICE), + knobs, + ); + row(&knobs.label(), &t); + let (took, rounds, before, after) = merge_to_quiescence(&conn); + println!( + " {:<16} merge: {:.0} ms over {} rounds, %_data {} -> {} rows", + "", + took.as_secs_f64() * 1000.0, + rounds, + before, + after + ); + drop(conn); + arm.discard(); + } + + // The page cache. `PRAGMAS_INCREMENTAL` gives the reconcile a flat + // 4 MiB while it scans and rewrites across the *whole* index. The scan + // reads `idx_files_parent` in order but fetches a table row per entry, + // and the writes then scatter into `%_docsize`, `documents_text` and + // FTS5's tombstone pages in orders uncorrelated with `(parent, name)`. + // `misses` is the column to read. + println!("\n page cache, as shipped [{} = shipped]:", SHIPPED_MIB); + header(); + for mib in [SHIPPED_MIB, 16, 32, 64, 128, 256] { + for (name, commit) in [ + ("per page", Commit::PerPage), + ("per slice", Commit::Slice(scope::SLICE)), + ] { + let arm = clone_arm( + &master, + &format!("content-{}-c{}-{}", label, mib, name.len()), + ); + let conn = open(&arm); + set_cache(&conn, mib); + let t = run_stage(&conn, &config, ®istry, Stage::ClearAll, commit); + row(&format!("{} MiB, {}", mib, name), &t); + drop(conn); + arm.discard(); + } + } + + // How much of the index the narrowing takes, swept. A filter that + // withdraws most of the postings is a different problem from one that + // withdraws a few: past some fraction FTS5 stops tombstoning and starts + // rewriting segments, and rewriting the full-text index is what + // building it was. `today` is what ships; `fixed` is the projection. + println!("\n withdrawn fraction (today -> fixed, both + merge to quiescence):"); + println!( + " {:<13} {:>8} {:>9} {:>9} {:>9} {:>11} {:>11}", + "keeping", "written", "today", "fixed", "merge", "%_data", "postings" + ); + for keep in [&["txt", "md"][..], &["txt"][..], &["rtf"][..]] { + let config = config_with(keep); + let tag = keep.join("+"); + let mut timings = Vec::new(); + for (what, stage, commit) in [ + ("today", Stage::ClearAll, Commit::PerPage), + ("fixed", Stage::Chunked, Commit::Slice(scope::SLICE)), + ] { + let arm = clone_arm(&master, &format!("content-{}-{}-{}", label, tag, what)); + let conn = open(&arm); + let before = fts_data_rows(&conn); + let t = run_stage(&conn, &config, ®istry, stage, commit); + let (merge, _, _, after) = merge_to_quiescence(&conn); + let left = count(&conn, "SELECT COUNT(*) FROM searchabletext"); + timings.push((t, merge, before, after, left)); + drop(conn); + arm.discard(); + } + let ms = |d: Duration| d.as_secs_f64() * 1000.0; + println!( + " {:<13} {:>8} {:>8.0} {:>8.0} {:>8.0} {:>11} {:>11}", + tag, + timings[0].0.to_na + timings[0].0.to_pending, + ms(timings[0].0.total), + ms(timings[1].0.total), + ms(timings[1].1), + format!("{}->{}", timings[1].2, timings[1].3), + timings[1].4, + ); + } + + // The reference number every stage above is decomposing: the real + // `scope::advance`, driven to completion the way the coordinator drives + // it. It must land on `+clear(all)`. + { + let arm = clone_arm(&master, &format!("content-{}-live", label)); + let mut conn = open(&arm); + let unfiltered = config_with(&[]); + let actions = quicksearch_core::config::diff_actions(&unfiltered, &config); + assert!( + actions.work.reconcile_content && !actions.work.reindex, + "the narrowing should reconcile content and ask for no walk, got {:?}", + actions.work + ); + let mut cursor = WorkCursor::new(actions.work, &config).expect("plan"); + let run = std::sync::atomic::AtomicBool::new(false); + let started = Instant::now(); + while !cursor.done() { + scope::advance( + &mut conn, + &config, + ®istry, + &mut cursor, + Instant::now() + scope::SLICE, + &run, + ) + .expect("advance"); + } + println!( + " {:<16} {:>6} {:>6} {:>6} {:>7} {:>6} {:>6} {:>7} {:>7.0} {:>8} {:>7} <- scope::advance", + "live", + "", + "", + "", + "", + "", + "", + "", + started.elapsed().as_secs_f64() * 1000.0, + "", + cursor.recontented, + ); + drop(conn); + arm.discard(); + } + + master.discard(); + } +} diff --git a/crates/quicksearch-core/examples/pruneprobe.rs b/crates/quicksearch-core/examples/pruneprobe.rs index 14a556b..2aa23e2 100644 --- a/crates/quicksearch-core/examples/pruneprobe.rs +++ b/crates/quicksearch-core/examples/pruneprobe.rs @@ -23,11 +23,24 @@ //! page read the rows, decide nothing //! +cover ...and run the scope test per row //! +files ...and delete the doomed `files` rows -//! +fts(all) ...and tombstone every doomed id <- ships today +//! +fts(all) ...and tombstone every doomed id //! +fts(done) ...tombstoning only ids that can have an FTS row //! +subtree ...range-deleting a doomed directory instead of paging it //! ``` //! +//! **No stage is what ships any more.** `scope::advance` tombstones only the +//! ids that can hold a posting (`+fts(done)`), commits per slice rather than +//! per page, and turns FTS5's delete-merging off for the pass +//! (`file_handling::fts_begin_tombstone_burst`), so the `live` row below lands +//! under every stage rather than on one of them. The stages remain the +//! decomposition — they say where the time is — and `live` says what the sum +//! of the shipped decisions costs. +//! +//! Two of them were measured and **not** adopted, which is why they are still +//! here: `+subtree` bought nothing over `+fts(done)` (247 ms against 243 on the +//! 40k corpus, with all 8,080 doomed rows genuinely skipping the page loop), and +//! raising the page cache moved misses twelvefold while barely moving the clock. +//! //! **Read the `commit` column, not `fts`.** FTS5 buffers a contentless delete //! in memory and writes the tombstone pages when the transaction is flushed, so //! the `DELETE` statement itself times as nearly free and the cost lands in the @@ -824,7 +837,8 @@ fn main() { // The reference number every stage above is decomposing: the real // `scope::advance`, driven to completion the way the coordinator drives - // it. It must land on `+fts(all)`. + // it. It lands *under* every stage — see the header for which decisions + // put it there. { let arm = clone_arm(&master, &format!("prune-{}-live", label)); let mut conn = open(&arm); diff --git a/crates/quicksearch-core/src/config/mod.rs b/crates/quicksearch-core/src/config/mod.rs index 22ed758..4d01a65 100644 --- a/crates/quicksearch-core/src/config/mod.rs +++ b/crates/quicksearch-core/src/config/mod.rs @@ -251,7 +251,7 @@ impl Default for ProcessingConfig { impl Default for SearchConfig { fn default() -> Self { SearchConfig { - fuzzy_default: false, + fuzzy_default: true, fuzzy_max_edits: 2, display_limit: 1000, results_per_page: 100, diff --git a/crates/quicksearch-core/src/config/tests.rs b/crates/quicksearch-core/src/config/tests.rs index cad1a51..00e0a56 100644 --- a/crates/quicksearch-core/src/config/tests.rs +++ b/crates/quicksearch-core/src/config/tests.rs @@ -837,7 +837,7 @@ fn newer_fields_round_trip_and_default_when_absent() { fs::write( &path, "[paths]\nindexing_paths=[\"/x\"]\ndatabase_path=\"db.sqlite\"\n\ - [ui]\nscale=1.25\n[search]\nfuzzy_default=true\ndisplay_limit=250\n", + [ui]\nscale=1.25\n[search]\nfuzzy_default=false\ndisplay_limit=250\n", ) .unwrap(); let cfg = Config::load_from(&path).unwrap(); @@ -850,7 +850,8 @@ fn newer_fields_round_trip_and_default_when_absent() { assert_eq!(cfg.ui.color_scheme, "dark"); assert_eq!(cfg.search.fuzzy_max_edits, 2); assert_eq!(cfg.ui.scale, 1.25, "existing ui keys still parse"); - assert!(cfg.search.fuzzy_default, "existing search keys still parse"); + // `false` is the non-default value, so this only passes if the key parsed. + assert!(!cfg.search.fuzzy_default, "existing search keys still parse"); assert_eq!(cfg.search.display_limit, 250); // A value nobody recognises is not a broken config file. diff --git a/crates/quicksearch-core/src/db/repo.rs b/crates/quicksearch-core/src/db/repo.rs index d05dd88..b21fa64 100644 --- a/crates/quicksearch-core/src/db/repo.rs +++ b/crates/quicksearch-core/src/db/repo.rs @@ -318,7 +318,23 @@ pub fn raw_text_len(blob: &[u8]) -> Option { } /// Mark a file's content extraction as failed. Keeps the basic row in place. +/// +/// Clears any posting and stored body first, which matters twice over. It is +/// what a re-extraction that fails *owes* the reader: the text that is there +/// came out of an earlier version of a file that has since changed, so leaving +/// it serves hits for content the file no longer has. And it is what makes +/// "a `searchabletext` row exists exactly when `content_state` is +/// `STATE_DONE`" true of every transition rather than of most of them — +/// an equivalence `count_root` reports from and `delete_files_matching` now +/// narrows on, so a transition that quietly broke it would leave postings +/// behind for files that are no longer indexed. +/// +/// Every path that reaches here today is already re-extracting a row it has +/// just reset (`update_file_basic`) or that was born pending, so the two +/// deletes are normally no-ops; extraction failures are rare enough that +/// paying for the guarantee is not worth measuring. pub fn set_content_failed(tx: &Transaction<'_>, file_id: i64, reason: &str) -> Result<(), String> { + remove_content_for_id(tx, file_id)?; let now = crate::log::now_unix() as i64; exec( tx, @@ -337,7 +353,15 @@ pub fn set_content_failed(tx: &Transaction<'_>, file_id: i64, reason: &str) -> R /// Mark content extraction as not applicable; the row still serves filename /// search. +/// +/// Clears any posting and stored body first, for the reasons spelled out on +/// [`set_content_failed`] — a row arrives here because its content should no +/// longer be searchable, so leaving the old text behind contradicts the very +/// transition. Every caller already cleared first or had nothing to clear, so +/// this changes no behaviour; what it changes is that the invariant no longer +/// depends on all of them remembering. pub fn set_content_na(tx: &Transaction<'_>, file_id: i64) -> Result<(), String> { + remove_content_for_id(tx, file_id)?; set_state_clearing_failure(tx, file_id, STATE_NA, "update NA") } @@ -380,6 +404,16 @@ pub fn delete_subtree(tx: &Transaction<'_>, lo: &str, hi: &str) -> Result, files_where: &str, @@ -437,18 +471,42 @@ fn placeholders(n: usize) -> String { /// Delete the given file ids and everything keyed to them. Returns how many /// `files` rows went. Dependent tables: see `delete_files_matching`. -pub fn delete_ids(tx: &Transaction<'_>, ids: &[i64]) -> Result { - let mut removed = 0; - for chunk in ids.chunks(DELETE_IDS_CHUNK) { - let placeholders = placeholders(chunk.len()); +/// +/// `with_postings` is the subset of `ids` whose `content_state` was +/// `STATE_DONE`, and so the only ones FTS5 can have anything to tombstone for — +/// `repo_tests::leaving_done_always_takes_the_posting_with_it` is what makes +/// that true of every transition. Handing FTS5 the rest is not free: each is a +/// `%_docsize` seek to discover an absence, and on an index where most rows +/// carry no text that is most of the list. +/// +/// Taking it as a second argument rather than deriving it here is the point: +/// the caller decided these rows from a page it had already read, so it holds +/// `content_state` for nothing, where a `SELECT` back out of `files` would cost +/// a row fetch each (see `delete_files_matching`, which for that reason does +/// not narrow). +/// +/// It must be a subset: an id left out keeps its posting after its `files` row +/// is gone, which surfaces as a hit for a file that is no longer indexed. +pub fn delete_ids( + tx: &Transaction<'_>, + ids: &[i64], + with_postings: &[i64], +) -> Result { + for chunk in with_postings.chunks(DELETE_IDS_CHUNK) { let sql = format!( "DELETE FROM searchabletext WHERE rowid IN ({})", - placeholders + placeholders(chunk.len()) ); exec(tx, &sql, params_from_iter(chunk.iter()), || { format!("delete searchabletext for {} ids", chunk.len()) })?; - let sql = format!("DELETE FROM files WHERE id IN ({})", placeholders); + } + let mut removed = 0; + for chunk in ids.chunks(DELETE_IDS_CHUNK) { + let sql = format!( + "DELETE FROM files WHERE id IN ({})", + placeholders(chunk.len()) + ); removed += exec(tx, &sql, params_from_iter(chunk.iter()), || { format!("delete {} file rows", chunk.len()) })?; @@ -456,6 +514,77 @@ pub fn delete_ids(tx: &Transaction<'_>, ids: &[i64]) -> Result { Ok(removed) } +/// Set `content_state` on many rows at once, leaving everything else alone. +/// +/// The batch form of the `UPDATE` inside [`set_state_clearing_failure`], and +/// deliberately *without* its `failed_files` sweep: only a `STATE_FAILED` row +/// can hold such a record, so a caller that knows the stored states can clear +/// the few that need it with [`clear_failed_for_ids`] instead of paying a +/// delete per row. A caller that does not know them must call the per-row +/// helpers, which cannot get this wrong. +pub fn set_content_state(tx: &Transaction<'_>, ids: &[i64], state: i64) -> Result { + let mut changed = 0; + for chunk in ids.chunks(DELETE_IDS_CHUNK) { + let sql = format!( + "UPDATE files SET content_state = ?1 WHERE id IN ({})", + placeholders(chunk.len()) + ); + let params = params_from_iter( + std::iter::once(&state as &dyn rusqlite::ToSql) + .chain(chunk.iter().map(|id| id as &dyn rusqlite::ToSql)), + ); + changed += exec(tx, &sql, params, || { + format!("set content_state {} on {} rows", state, chunk.len()) + })?; + } + Ok(changed) +} + +/// Drop the FTS posting and the stored body of many rows at once, leaving +/// their `files` rows in place: the batch form of [`remove_content_for_id`]. +/// +/// Pass only ids whose stored `content_state` was `STATE_DONE`; the others +/// have neither, and asking is what costs (see `delete_files_matching`). +pub fn clear_content_for_ids(tx: &Transaction<'_>, ids: &[i64]) -> Result<(), String> { + for chunk in ids.chunks(DELETE_IDS_CHUNK) { + let list = placeholders(chunk.len()); + for (what, sql) in [ + ( + "searchabletext", + format!("DELETE FROM searchabletext WHERE rowid IN ({})", list), + ), + ( + "documents_text", + format!("DELETE FROM documents_text WHERE file_id IN ({})", list), + ), + ] { + exec(tx, &sql, params_from_iter(chunk.iter()), || { + format!("clear {} for {} ids", what, chunk.len()) + })?; + } + } + Ok(()) +} + +/// Forget the failure records of many rows at once. `list-failed` reads +/// `failed_files` directly, so a stale entry keeps reporting a file broken — +/// this is the batch half of what [`set_state_clearing_failure`] does per row. +/// +/// Pass only ids whose stored `content_state` was `STATE_FAILED`: nothing else +/// can hold a record here. +pub fn clear_failed_for_ids(tx: &Transaction<'_>, ids: &[i64]) -> Result<(), String> { + for chunk in ids.chunks(DELETE_IDS_CHUNK) { + let sql = format!( + "DELETE FROM failed_files WHERE file_id IN ({})", + placeholders(chunk.len()) + ); + exec(tx, &sql, params_from_iter(chunk.iter()), || { + format!("clear failed_files for {} ids", chunk.len()) + })?; + } + Ok(()) +} + /// Every indexed file directly inside `parent`, as `name -> mtime`. `parent` /// must be in stored spelling — trailing separator and all; build it with /// [`crate::file_handling::dir_to_db_parent`]. diff --git a/crates/quicksearch-core/src/db/repo_tests.rs b/crates/quicksearch-core/src/db/repo_tests.rs index 09c6a60..a0e0fde 100644 --- a/crates/quicksearch-core/src/db/repo_tests.rs +++ b/crates/quicksearch-core/src/db/repo_tests.rs @@ -527,9 +527,12 @@ fn delete_ids_clears_every_dependent_table() { } let doomed = vec![ids["/t/b.log"], ids["/t/deep/c.log"]]; + // `c.log` is FAILED and so holds no posting — the distinction the second + // argument draws, and the whole reason it is a separate list. + let with_postings = vec![ids["/t/b.log"]]; let removed = { let tx = conn.transaction().unwrap(); - let n = delete_ids(&tx, &doomed).unwrap(); + let n = delete_ids(&tx, &doomed, &with_postings).unwrap(); tx.commit().unwrap(); n }; @@ -553,7 +556,7 @@ fn delete_ids_clears_every_dependent_table() { // Empty input is a no-op, not a statement with an empty `IN ()`. let tx = conn.transaction().unwrap(); - assert_eq!(delete_ids(&tx, &[]).unwrap(), 0); + assert_eq!(delete_ids(&tx, &[], &[]).unwrap(), 0); tx.commit().unwrap(); } @@ -572,7 +575,8 @@ fn delete_ids_spans_chunk_boundaries() { let keep = all.pop().unwrap(); let removed = { let tx = conn.transaction().unwrap(); - let n = delete_ids(&tx, &all).unwrap(); + // `seeded` leaves every row DONE, so both lists span the boundary. + let n = delete_ids(&tx, &all, &all).unwrap(); tx.commit().unwrap(); n }; @@ -1199,6 +1203,74 @@ fn fts_rows(conn: &Connection) -> i64 { .unwrap() } +/// Every transition *out* of `STATE_DONE` takes the posting and the stored +/// body with it, so "a `searchabletext` row exists exactly when `content_state` +/// is `STATE_DONE`" holds however a row got where it is. +/// +/// `count_root_counts_the_fts_rows_it_says_it_does` below checks the same +/// equivalence over rows that were *never* DONE, which is the easy half — it +/// passes even for a transition that leaves a stale posting behind. This is the +/// half that does not, and `delete_files_matching` narrows its tombstone +/// statement on the strength of it: a row that kept a posting past `DONE` would +/// keep it past deletion too, and answer searches for a file that is gone. +#[test] +fn leaving_done_always_takes_the_posting_with_it() { + let (_dir, p) = tmp_path(); + let mut conn = open_or_recreate(p.to_str().unwrap(), "trigram").unwrap(); + + // Each row starts DONE — with a posting and a stored body — and then takes + // one of the four ways out. + let leave: [(&str, fn(&rusqlite::Transaction<'_>, i64)); 4] = [ + ("/t/failed.txt", |tx, id| { + set_content_failed(tx, id, "bad parse").unwrap() + }), + ("/t/na.txt", |tx, id| set_content_na(tx, id).unwrap()), + ("/t/pending.txt", |tx, id| { + reset_content_pending(tx, id).unwrap() + }), + ("/t/rewritten.txt", |tx, id| { + // The shape a changed file takes: metadata updated in place. + update_file_basic( + tx, + &NewFile { + name: "rewritten.txt", + parent: "/t/", + size: 99, + mtime: 99, + mime: Some("text/plain"), + ftype: FileType::TEXT, + hash: None, + needs_content: true, + }, + ) + .unwrap() + .expect("the row is there"); + let _ = id; + }), + ]; + + for (path, transition) in leave { + let tx = conn.transaction().unwrap(); + let id = insert_at(&tx, path, true); + set_content_done(&tx, id, "body text", zstd_of("body text").as_deref()).unwrap(); + transition(&tx, id); + tx.commit().unwrap(); + } + + let count = |sql: &str| -> i64 { conn.query_row(sql, [], |r| r.get(0)).unwrap() }; + assert_eq!( + count("SELECT COUNT(*) FROM files WHERE content_state = 1"), + 0, + "every row left DONE" + ); + assert_eq!(fts_rows(&conn), 0, "and none of them kept its posting"); + assert_eq!( + count("SELECT COUNT(*) FROM documents_text"), + 0, + "nor its stored body, which would outlive the file it was read from" + ); +} + /// The premise of `count_root`: `content_state = DONE` exactly when a /// `searchabletext` row exists. Pinned against the FTS table itself, because /// the equivalence is what breaks if a transition writes one without the @@ -1348,12 +1420,16 @@ fn the_parent_scan_reaches_the_roots_own_directory() { fn deleting_a_file_row_cascades_the_fk_tables() { let (_dir, path) = tmp_path(); let mut conn = open_or_recreate(path.to_str().unwrap(), "trigram").unwrap(); - let ids = seeded(&mut conn, &["/casc/a.txt", "/casc/b.txt"]); + seeded(&mut conn, &["/casc/a.txt", "/casc/b.txt"]); let count = |conn: &Connection, sql: &str| -> i64 { conn.query_row(sql, [], |r| r.get(0)).unwrap() }; { + // The failure goes on a row that never extracted, which is the only + // way a run reaches `set_content_failed` — and keeps the two searchable + // rows searchable, so the FTS figures below stay about the cascade. let tx = conn.transaction().unwrap(); - set_content_failed(&tx, ids["/casc/b.txt"], "boom").unwrap(); + let never = insert_at(&tx, "/casc/c.bin", true); + set_content_failed(&tx, never, "boom").unwrap(); tx.commit().unwrap(); } assert_eq!(count(&conn, "SELECT COUNT(*) FROM documents_text"), 2); diff --git a/crates/quicksearch-core/src/file_handling/mod.rs b/crates/quicksearch-core/src/file_handling/mod.rs index a36a360..c74b4ad 100644 --- a/crates/quicksearch-core/src/file_handling/mod.rs +++ b/crates/quicksearch-core/src/file_handling/mod.rs @@ -27,7 +27,8 @@ pub(crate) use paths::{ }; pub use records::{ classify_by_mtime, classify_for_indexing, content_extractable, decide_content, - extract_and_store, fts_begin_bulk_write, fts_finalize_after_text_indexing, fts_set_automerge, + extract_and_store, fts_begin_bulk_write, fts_begin_tombstone_burst, fts_end_tombstone_burst, + fts_finalize_after_text_indexing, fts_set_automerge, FTS_DELETEMERGE, get_file_hash, hash_failure_counts, outcome_body, prepare_file_record, prepare_file_record_from_path, reset_run_warnings, store_content_outcome, ContentOutcome, DirRows, FileIndexAction, OwnedNewFile, diff --git a/crates/quicksearch-core/src/file_handling/records.rs b/crates/quicksearch-core/src/file_handling/records.rs index 7805d95..a6f699e 100644 --- a/crates/quicksearch-core/src/file_handling/records.rs +++ b/crates/quicksearch-core/src/file_handling/records.rs @@ -116,6 +116,147 @@ pub fn fts_set_automerge(conn: &Connection, segments: u8) { /// smaller, more build-stable index for no cost; 64 measured identically. const WRITE_CRISISMERGE: u8 = 32; +/// Percentage of a level's entries that must be tombstones before FTS5 rewrites +/// the level to reclaim them, and **the single most expensive setting a bulk +/// withdrawal of content runs into.** FTS5's own default, restored by +/// [`fts_begin_bulk_write`] and by [`fts_end_tombstone_burst`]. +/// +/// `0` disables delete-merging entirely — including from an explicit `'merge'`, +/// because `fts5IndexFindDeleteMerge` returns early on it — so it must never be +/// the resting value of an index. [`fts_begin_tombstone_burst`] is the only +/// thing that sets it, and always in a pair. +pub const FTS_DELETEMERGE: u8 = 10; + +/// Set FTS5's delete-merge threshold. Best-effort; failure is logged. +fn fts_set_deletemerge(conn: &Connection, percent: u8) { + if let Err(e) = conn.execute( + "INSERT INTO searchabletext(searchabletext, rank) VALUES('deletemerge', ?1)", + [percent as i64], + ) { + crate::log_warn!("FTS deletemerge failed (non-fatal): {}", e); + } +} + +/// Stop FTS5 reclaiming tombstones *while* a pass is creating them, for a +/// caller that is about to delete a great many postings in one go and will call +/// [`fts_end_tombstone_burst`] when it is done. +/// +/// # What it is worth +/// +/// `examples/contentprobe.rs`, 40k rows of which 7,273 lose their content +/// (6,909 of them holding a posting), chunked deletes, committing per +/// `scope::SLICE`. Both arms, each pair from one run: +/// +/// | arm | | fts | commit | pass | + merge | misses | +/// |---|---|---|---|---|---|---| +/// | plain | `deletemerge` 10 | 734 ms | 96 ms | **891 ms** | 892 ms | 97,130 | +/// | plain | `deletemerge` 0 | 86 ms | 11 ms | **141 ms** | 218 ms | 9,654 | +/// | keyed | `deletemerge` 10 | 1174 ms | 263 ms | **1576 ms** | 1579 ms | 96,181 | +/// | keyed | `deletemerge` 0 | 128 ms | 17 ms | **211 ms** | 373 ms | 9,476 | +/// +/// 6.3x on the pass plain and 7.5x keyed; 4.1x and 4.2x once the trailing merge +/// is counted. Page-cache misses fall tenfold, which is why the keyed arm gains +/// more — every one of those pages was being decrypted and re-encrypted. +/// +/// It also lands on a **smaller** index than the shipped path does — `%_data` +/// 1,753 rows against 1,886 — because one merge at the end consolidates better +/// than many mid-pass ones. That is the whole bargain: the mid-pass merges are +/// not just expensive, they are worse at the job. +/// +/// End to end — `scope::advance` driven to completion the way the coordinator +/// drives it, on an idle machine — this and the two changes beside it are worth +/// **4.3x to 5.6x**, and the ratio *grows* with the index, because the levels +/// being needlessly rewritten grow with it: +/// +/// | corpus | arm | before | after | | +/// |---|---|---|---|---| +/// | 40k | plain | 1,066 ms | **248 ms** | 4.3x | +/// | 40k | keyed | 1,814 ms | **417 ms** | 4.4x | +/// | 200k | plain | 4,600 ms | **925 ms** | 5.0x | +/// | 200k | keyed | 8,598 ms | **1,530 ms** | 5.6x | +/// +/// The 40k pair is a true A/B: the same probe and corpus run against this tree +/// and against `HEAD` without these changes. Its *control* is what makes it a +/// measurement rather than two numbers from two binaries — +/// `contentprobe`'s `+clear(all)` stage reproduces the old shape in the probe's +/// own code, so it must not move between the builds, and it did not (1000 → 996 +/// plain, 1801 → 1770 keyed). The 200k rows use that validated stage as the +/// "before", which is why they can come from a single run. +/// +/// Measured and rejected alongside it: sorting each page's ids into rowid order +/// before deleting, which moved nothing (898 ms against 891 plain, 1565 against +/// 1576 keyed). FTS5 picks a tombstone page by *hashing* the rowid, so there is +/// no locality to restore. +/// +/// # Why it is so large +/// +/// Every contentless delete counts into the same write-counter that drives +/// `fts5IndexAutomerge`, and once a level passes this threshold +/// `fts5IndexFindDeleteMerge` picks it and rewrites the whole level — inline +/// with the scan, and then again as the pass keeps deleting. Rewriting the +/// full-text index is what building it was. +/// +/// # The pairing is load-bearing +/// +/// `deletemerge` is persisted in FTS5's `%_config` shadow table, so a value +/// left at `0` outlives the process and no later `'merge'` would ever reclaim a +/// tombstone again. [`fts_end_tombstone_burst`] restores it, and +/// [`fts_begin_bulk_write`] sets it unconditionally so that a crash between the +/// two is repaired by the next indexing run rather than being permanent. +pub fn fts_begin_tombstone_burst(conn: &Connection) { + fts_set_deletemerge(conn, 0); +} + +/// Rounds of [`fts_finalize_after_text_indexing`] a burst may spend +/// consolidating before it gives up and leaves the rest to the next run. +/// +/// Three is what the corpus in [`fts_begin_tombstone_burst`] needed; the cap is +/// above that so the common case finishes, and exists only so a pathological +/// index cannot hold the coordinator thread indefinitely. +const BURST_MERGE_ROUNDS: u32 = 8; + +/// Restore delete-merging and consolidate what the burst left behind. The +/// mirror of [`fts_begin_tombstone_burst`]; see there for the measurements. +/// +/// The order matters twice: the merge would reclaim nothing with the threshold +/// still at `0`, and the threshold must go back even when there is nothing to +/// merge, because a value left there would outlive the process. +/// +/// # Why this merges to quiescence and `fts_finalize_after_text_indexing` +/// does not +/// +/// A single 1000-page `'merge'` is the right bargain at the end of an indexing +/// run — whatever it leaves, the next run's merge finishes, and it was never +/// far behind. A burst is a different bargain: it deliberately built up a +/// backlog several times larger than a run ever does (`%_data` 4,931 rows +/// against the 1,886 the shipped path leaves), and searching against that until +/// some future run is a cost this pass created and should pay. It takes 77 ms +/// plain and 162 ms keyed, against the ~750 ms and ~1,450 ms the burst saved. +/// +/// The quiescence signal is the `%_data` row count, and it has to be: measured +/// in `examples/pruneprobe.rs`, `sqlite3_changes()` after a `'merge'` reports +/// non-zero forever, so the obvious `while changes() != 0` never terminates. +pub fn fts_end_tombstone_burst(conn: &Connection) { + fts_set_deletemerge(conn, FTS_DELETEMERGE); + let mut last = fts_data_rows(conn); + for _ in 0..BURST_MERGE_ROUNDS { + fts_finalize_after_text_indexing(conn); + let now = fts_data_rows(conn); + if now == last { + return; + } + last = now; + } +} + +/// Rows in FTS5's `%_data` shadow table — how much the full-text index is +/// physically holding, tombstones and all. `None` if it cannot be read, which +/// stops [`fts_end_tombstone_burst`]'s loop rather than spinning it. +fn fts_data_rows(conn: &Connection) -> Option { + conn.query_row("SELECT COUNT(*) FROM searchabletext_data", [], |r| r.get(0)) + .ok() +} + /// Apply the write-side FTS5 settings, before a run starts writing. /// /// `pgsz` is deliberately absent: it is not a per-run setting. Sweeping it @@ -123,8 +264,13 @@ const WRITE_CRISISMERGE: u8 = 32; /// default 4050 stands there. Keyed is the opposite — SQLCipher's page reserve /// makes 4050 a cliff — and that case is handled once at schema creation; see /// [`crate::db::schema::FTS_PGSZ_ENCRYPTED`]. +/// +/// `deletemerge` is set even though this never lowers it: it is how an index +/// whose reconcile was killed mid-burst gets its tombstone reclamation back. +/// See [`fts_begin_tombstone_burst`]. pub fn fts_begin_bulk_write(conn: &Connection) { fts_set_automerge(conn, WRITE_AUTOMERGE); + fts_set_deletemerge(conn, FTS_DELETEMERGE); if let Err(e) = conn.execute( "INSERT INTO searchabletext(searchabletext, rank) VALUES('crisismerge', ?1)", [WRITE_CRISISMERGE as i64], diff --git a/crates/quicksearch-core/src/file_handling/tests.rs b/crates/quicksearch-core/src/file_handling/tests.rs index 8d2b500..700e287 100644 --- a/crates/quicksearch-core/src/file_handling/tests.rs +++ b/crates/quicksearch-core/src/file_handling/tests.rs @@ -600,3 +600,38 @@ fn a_symlinked_root_yields_no_directories_when_following_is_off() { std::fs::remove_dir_all(&base).ok(); } + +/// A reconcile killed mid-burst leaves `deletemerge` at 0, where no later +/// `'merge'` would reclaim a tombstone again. The next indexing run repairs it. +/// +/// This is the whole reason [`fts_begin_bulk_write`] writes a value it never +/// lowers: the pairing in `scope::advance` covers the orderly cases, and this +/// covers the process simply not coming back. +#[test] +fn a_bulk_write_repairs_a_delete_merge_threshold_left_off() { + let dir = crate::testutil::scratch_dir("deletemerge-repair"); + let db = dir.join("index.sqlite"); + let conn = crate::db::open_or_recreate(db.to_str().unwrap(), "trigram").unwrap(); + + let threshold = || -> Option { + conn.query_row( + "SELECT v FROM searchabletext_config WHERE k = 'deletemerge'", + [], + |r| r.get(0), + ) + .ok() + }; + + // What a killed burst leaves behind. + fts_begin_tombstone_burst(&conn); + assert_eq!(threshold(), Some(0), "the burst is in effect"); + + fts_begin_bulk_write(&conn); + assert_eq!( + threshold(), + Some(i64::from(FTS_DELETEMERGE)), + "the next run puts tombstone reclamation back" + ); + + std::fs::remove_dir_all(&dir).ok(); +} diff --git a/crates/quicksearch-core/src/scope.rs b/crates/quicksearch-core/src/scope.rs index 3eab671..57e05cf 100644 --- a/crates/quicksearch-core/src/scope.rs +++ b/crates/quicksearch-core/src/scope.rs @@ -1,6 +1,34 @@ //! Brings a stored index back in line with a changed configuration without //! rebuilding it. Nothing here stamps the stored configuration — the caller //! does, and only once the cursor reports finished; see [`outstanding_work`]. +//! +//! # Where the time goes +//! +//! Almost entirely in FTS5: withdrawing content from a row tombstones its +//! posting, and that is 100x what deciding the row costs. `examples/contentprobe.rs` +//! and `examples/pruneprobe.rs` attribute a pass to a phase; the three +//! decisions that came out of them are [`SLICE`]-long transactions rather than +//! one per page, [`PagePlan`]'s chunked writes narrowed to the rows that can +//! actually hold what is being cleared, and +//! [`crate::file_handling::fts_begin_tombstone_burst`] — much the largest of +//! the three, and the place to read for why. +//! +//! Measured and **not** adopted, so they are not re-derived. Both looked +//! obvious; neither paid. +//! +//! - **A bigger page cache for the pass.** `PRAGMAS_INCREMENTAL`'s 4 MiB looks +//! far too small for a scan that rewrites across the whole index, and raising +//! it does exactly what it should to the miss count — 79,266 down to 6,499 at +//! 64 MiB — while the clock stays put (927 ms against 976). The misses were +//! never the expensive part; the tombstone writes behind them were. +//! - **Range-deleting a wholly-excluded directory** rather than paging through +//! it (`pruneprobe`'s `+subtree`). It does skip the page loop for every +//! doomed row, and costs 247 ms against `+fts(done)`'s 243. +//! +//! A third was never built, for the same reason: scanning by rowid instead of +//! by `(parent, name)` when `prune_scope` is off, which the root loop would +//! allow. Reading the pages is 24–30 ms of a ~950 ms pass — there is nothing +//! there to win. use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, Ordering}; @@ -11,7 +39,9 @@ use rusqlite::Connection; use crate::config::{Config, IgnoreSet, IndexWork}; use crate::db::repo; use crate::extract::Registry; -use crate::file_handling::{content_extractable, fts_finalize_after_text_indexing, ExtractCursor}; +use crate::file_handling::{ + content_extractable, fts_begin_tombstone_burst, fts_end_tombstone_burst, ExtractCursor, +}; use crate::indexing::ReconcileProgress; /// How long [`advance`] may work before handing control back. This bounds @@ -180,6 +210,12 @@ impl WorkCursor { self.finalized } + /// Whether the row scan has reached the end of the last root. Distinct from + /// [`WorkCursor::done`], which also waits on the tombstone merge. + fn scan_done(&self) -> bool { + self.root_idx >= self.scope.roots.len() + } + pub fn progress(&self) -> ReconcileProgress { ReconcileProgress { examined: self.examined, @@ -227,6 +263,9 @@ pub fn advance( let tx = conn .transaction() .map_err(|e| format!("begin drop-root transaction: {}", e))?; + // A de-configured root can take every posting under it with it, which + // is the same burst the row scan makes; `advance` restores and merges. + fts_begin_tombstone_burst(&tx); let removed = repo::delete_subtree(&tx, &range.lo, &range.hi)?; tx.commit() .map_err(|e| format!("commit drop-root transaction: {}", e))?; @@ -253,6 +292,7 @@ pub fn advance( let tx = conn .transaction() .map_err(|e| format!("begin drop-alias transaction: {}", e))?; + fts_begin_tombstone_burst(&tx); let removed = repo::delete_outside_ranges(&tx, &ranges)?; tx.commit() .map_err(|e| format!("commit drop-alias transaction: {}", e))?; @@ -270,29 +310,91 @@ pub fn advance( } cursor.total = Some(repo::row_count(conn)?); } - let page = config.processing.batch_size.max(1) as i64; - let mut covered = CoverCache::default(); - while cursor.root_idx < cursor.scope.roots.len() { - if cancelled(cancel) { - return Ok(()); - } + scan_rows(conn, config, registry, cursor, deadline, cancel)?; + if !cursor.scan_done() { + return Ok(()); + } + } + + // Deletions leave FTS tombstones; restoring the threshold and merging + // collapses them. Skipping the merge costs only tidiness — the next run's + // does the same — but the threshold must go back whether anything was + // deleted or not, since `scan_rows` lowers it before it knows. + if cancelled(cancel) { + return Ok(()); + } + fts_end_tombstone_burst(conn); + cursor.finalized = true; + Ok(()) +} + +/// Rows one transaction may cover before it commits, whatever the clock says. +/// +/// A backstop, not the primary limit: [`SLICE`] normally ends a transaction +/// first. It exists so that a fast disk and a wide slice cannot build an +/// unbounded set of dirty pages before anything is durable. +const COMMIT_ROWS: usize = 20_000; + +/// Page every configured root, deciding and writing rows until the deadline. +/// +/// One transaction spans as much of the slice as it can rather than one per +/// page. The read runs inside it, so each page sees the previous one's writes; +/// the cursor advances with the reads, so a rollback leaves it ahead of the +/// database — which is safe only because every caller of a failed [`advance`] +/// discards the cursor and nothing is stamped until one finishes. Cancellation +/// commits what it has: the work is idempotent, so a resumed cursor redoing it +/// would be correct too, but there is no reason to throw it away. +/// +/// **The deadline is tested only after a page has been applied**, so a call +/// always makes progress. Testing it on entry instead is a spin: the caller +/// loops until the cursor finishes, and one that hands over an already-expired +/// deadline — which `advance`'s own `row_count` can produce on a large index, +/// and which `scope_tests` does deliberately — would never advance it. +fn scan_rows( + conn: &mut Connection, + config: &Config, + registry: &Registry, + cursor: &mut WorkCursor, + deadline: Instant, + cancel: &AtomicBool, +) -> Result<(), String> { + let page = config.processing.batch_size.max(1) as i64; + let mut covered = CoverCache::default(); + let mut plan = PagePlan::default(); + while !cursor.scan_done() { + if cancelled(cancel) { + return Ok(()); + } + let tx = conn + .transaction() + .map_err(|e| format!("begin reconcile transaction: {}", e))?; + // Inside the transaction, so a rollback puts the threshold back with + // everything else; `advance` restores it for good when the pass ends. + // First thing in it, so the flush this implies has nothing to flush. + fts_begin_tombstone_burst(&tx); + let mut buffered = 0usize; + let mut spent = false; + while !cursor.scan_done() { let root = &cursor.scope.roots[cursor.root_idx]; if cursor.after.0.is_empty() { // `(lo, "")` sorts below every row in the range — no stored name is empty. cursor.after = (root.lo.clone(), String::new()); } let rows = - repo::rows_in_range_page(conn, &cursor.after.0, &cursor.after.1, &root.hi, page)?; + repo::rows_in_range_page(&tx, &cursor.after.0, &cursor.after.1, &root.hi, page)?; let Some(last) = rows.last() else { + // An exhausted root is not a page of work — moving to the next + // one must not be able to spend the slice on its own. cursor.root_idx += 1; cursor.after = (String::new(), String::new()); continue; }; cursor.after = (last.parent.clone(), last.name.clone()); cursor.examined += rows.len(); + buffered += rows.len(); let root = cursor.scope.roots[cursor.root_idx].path.clone(); let (deleted, recontented) = apply_page( - conn, + &tx, config, registry, &cursor.scope, @@ -300,24 +402,21 @@ pub fn advance( &root, &rows, &mut covered, + &mut plan, )?; cursor.deleted += deleted; cursor.recontented += recontented; - if Instant::now() >= deadline { - return Ok(()); + if buffered >= COMMIT_ROWS || cancelled(cancel) || Instant::now() >= deadline { + spent = true; + break; } } + tx.commit() + .map_err(|e| format!("commit reconcile transaction: {}", e))?; + if spent { + return Ok(()); + } } - - // Deletions leave FTS tombstones; a merge collapses them. Skipping it - // costs only tidiness — the next run's merge does the same. - if cancelled(cancel) { - return Ok(()); - } - if cursor.deleted > 0 || cursor.recontented > 0 { - fts_finalize_after_text_indexing(conn); - } - cursor.finalized = true; Ok(()) } @@ -325,10 +424,85 @@ fn cancelled(cancel: &AtomicBool) -> bool { cancel.load(Ordering::Relaxed) } -/// Decide and write one page of rows. Returns `(deleted, recontented)`. +/// One page's decisions, bucketed so every write goes out as a chunked +/// `IN (...)` list rather than a bound statement per row. +/// +/// The buckets are by *stored* state as well as by destination, because that is +/// what says which of the three dependent writes a row actually needs: a +/// posting and a stored body exist only for `STATE_DONE`, a failure record only +/// for `STATE_FAILED` (`repo::leaving_done_always_takes_the_posting_with_it` +/// pins both). A `STATE_PENDING` row leaving for `STATE_NA` needs one `UPDATE` +/// and nothing else, where the per-row form spent four statements discovering +/// that twice over. +/// +/// Reused across pages — `clear` keeps the capacity — so a whole pass allocates +/// these once. +#[derive(Default)] +struct PagePlan { + /// Rows leaving the index entirely, and the subset of them that can hold a + /// posting (see [`repo::delete_ids`]). + doomed: Vec, + doomed_content: Vec, + /// Rows keeping their posting but losing the snippet source. + stale_text: Vec, + /// Surviving rows changing `content_state`, and — across both — those whose + /// stored state says they have content or a failure record to clear first. + to_pending: Vec, + to_na: Vec, + restated_content: Vec, + restated_failure: Vec, +} + +impl PagePlan { + fn clear(&mut self) { + for list in [ + &mut self.doomed, + &mut self.doomed_content, + &mut self.stale_text, + &mut self.to_pending, + &mut self.to_na, + &mut self.restated_content, + &mut self.restated_failure, + ] { + list.clear(); + } + } + + /// Note that a surviving row is changing state, and what it must shed first. + fn restate(&mut self, row: &repo::ScopeRow, to_pending: bool) { + if to_pending { + self.to_pending.push(row.id); + } else { + self.to_na.push(row.id); + } + match row.content_state { + repo::STATE_DONE => self.restated_content.push(row.id), + repo::STATE_FAILED => self.restated_failure.push(row.id), + _ => {} + } + } + + /// Returns `(deleted, recontented)`. + fn write(&self, tx: &rusqlite::Transaction<'_>) -> Result<(usize, usize), String> { + let deleted = if self.doomed.is_empty() { + 0 + } else { + repo::delete_ids(tx, &self.doomed, &self.doomed_content)? + }; + repo::drop_stored_text(tx, &self.stale_text)?; + repo::clear_content_for_ids(tx, &self.restated_content)?; + repo::clear_failed_for_ids(tx, &self.restated_failure)?; + repo::set_content_state(tx, &self.to_pending, repo::STATE_PENDING)?; + repo::set_content_state(tx, &self.to_na, repo::STATE_NA)?; + Ok((deleted, self.to_pending.len() + self.to_na.len())) + } +} + +/// Decide one page of rows into `plan`, then write it. Returns +/// `(deleted, recontented)`. #[allow(clippy::too_many_arguments)] fn apply_page( - conn: &mut Connection, + tx: &rusqlite::Transaction<'_>, config: &Config, registry: &Registry, scope: &Scope, @@ -336,20 +510,21 @@ fn apply_page( root: &Path, rows: &[repo::ScopeRow], covered: &mut CoverCache, + plan: &mut PagePlan, ) -> Result<(usize, usize), String> { - let mut doomed: Vec = Vec::new(); - let mut stale_text: Vec = Vec::new(); - let mut to_pending: Vec = Vec::new(); - let mut to_na: Vec = Vec::new(); - + plan.clear(); for row in rows { let path = Path::new(&row.path); if work.prune_scope && !scope.covers_cached(root, path, covered) { - doomed.push(row.id); + plan.doomed.push(row.id); + if row.content_state == repo::STATE_DONE { + plan.doomed_content.push(row.id); + } continue; } - if work.drop_text { - stale_text.push(row.id); + // Only a DONE row can have a `documents_text` body to drop. + if work.drop_text && row.content_state == repo::STATE_DONE { + plan.stale_text.push(row.id); } if work.reconcile_content || work.restore_text { // The walker's decision, recomputed. Both directions run whenever @@ -357,37 +532,16 @@ fn apply_page( let wants = row.size <= config.processing.maximum_text_file_size && content_extractable(path, row.mime.as_deref(), config, registry); if !wants && row.content_state != repo::STATE_NA { - to_na.push(row.id); + plan.restate(row, false); } else if wants && (row.content_state == repo::STATE_NA || (work.restore_text && row.content_state == repo::STATE_DONE)) { - to_pending.push(row.id); + plan.restate(row, true); } } } - - let tx = conn - .transaction() - .map_err(|e| format!("begin reconcile transaction: {}", e))?; - let deleted = if doomed.is_empty() { - 0 - } else { - repo::delete_ids(&tx, &doomed)? - }; - if !stale_text.is_empty() { - repo::drop_stored_text(&tx, &stale_text)?; - } - for id in &to_pending { - repo::reset_content_pending(&tx, *id)?; - } - for id in &to_na { - repo::remove_content_for_id(&tx, *id)?; - repo::set_content_na(&tx, *id)?; - } - tx.commit() - .map_err(|e| format!("commit reconcile transaction: {}", e))?; - Ok((deleted, to_pending.len() + to_na.len())) + plan.write(tx) } /// The configuration the index was last built with, as far as diff --git a/crates/quicksearch-core/src/scope_tests.rs b/crates/quicksearch-core/src/scope_tests.rs index 7499306..d3c62a3 100644 --- a/crates/quicksearch-core/src/scope_tests.rs +++ b/crates/quicksearch-core/src/scope_tests.rs @@ -1,5 +1,6 @@ use super::*; use crate::walk::{walk_indexable_files, WalkEvent}; +use rusqlite::OptionalExtension; use std::collections::HashSet; use std::sync::atomic::AtomicBool; use std::sync::Arc; @@ -211,6 +212,97 @@ fn the_scan_reports_its_way_through_every_row() { std::fs::remove_dir_all(&db_dir).ok(); } +/// FTS5's delete-merge threshold is turned off for the length of a pass and +/// must come back on at the end of it. +/// +/// Both halves are asserted, and both matter. Without the first the pass is +/// several times slower than it needs to be +/// (`file_handling::fts_begin_tombstone_burst` carries the table). Without the +/// second the index is left in a state where **no** later `'merge'` ever +/// reclaims a tombstone again — `fts5IndexFindDeleteMerge` returns early on a +/// zero threshold — and because FTS5 persists the setting in its `%_config` +/// shadow table, that outlives the process. It is a silent, permanent +/// degradation, which is exactly the kind of thing that needs a test rather +/// than a comment. +#[test] +fn a_pass_turns_delete_merging_off_and_puts_it_back() { + let root = tmp_tree("burst"); + for i in 0..6 { + touch(&root.join(format!("f{}.log", i))); + } + touch(&root.join("keep.txt")); + + let db_dir = tmp_tree("burst-db"); + let db = empty_db(&db_dir); + let mut conn = crate::db::open_existing(db.to_str().unwrap(), true).unwrap(); + let mut config = Config::default(); + config.paths.indexing_paths = vec![root.to_string_lossy().into_owned()]; + config.processing.batch_size = 1; + seed(&mut conn, &on_disk(&root)); + + // Read back out of FTS5's own `%_config`, not out of a value we remember: + // what outlives the process is what is written there. + let threshold = |conn: &Connection| -> Option { + conn.query_row( + "SELECT v FROM searchabletext_config WHERE k = 'deletemerge'", + [], + |r| r.get(0), + ) + .optional() + .unwrap() + }; + assert_eq!( + threshold(&conn), + None, + "a fresh index leaves FTS5 on its own default and records nothing" + ); + + let mut narrowed = config.clone(); + narrowed.indexing.ignore_patterns = vec!["*.log".into()]; + let work = crate::config::diff_actions(&config, &narrowed).work; + let mut cursor = WorkCursor::new(work, &narrowed).unwrap(); + let registry = Registry::default_set(); + let run = AtomicBool::new(false); + + // One slice, with a deadline already past: the pass is under way and has + // committed at least one page, so the threshold is off and durably so. + advance( + &mut conn, + &narrowed, + ®istry, + &mut cursor, + Instant::now(), + &run, + ) + .unwrap(); + assert!(!cursor.done(), "one page cannot have finished seven rows"); + assert_eq!( + threshold(&conn), + Some(0), + "delete-merging is off while the pass is creating tombstones" + ); + + while !cursor.done() { + advance( + &mut conn, + &narrowed, + ®istry, + &mut cursor, + Instant::now(), + &run, + ) + .unwrap(); + } + assert_eq!( + threshold(&conn), + Some(i64::from(crate::file_handling::FTS_DELETEMERGE)), + "a finished pass leaves tombstone reclamation working again" + ); + + std::fs::remove_dir_all(&root).ok(); + std::fs::remove_dir_all(&db_dir).ok(); +} + /// The cursor is left un-finished, so nothing downstream records the config /// as reconciled. Rows already reached stay gone: the pass is idempotent. #[test] diff --git a/crates/quicksearch-core/src/testutil.rs b/crates/quicksearch-core/src/testutil.rs index 53ec207..8e4c129 100644 --- a/crates/quicksearch-core/src/testutil.rs +++ b/crates/quicksearch-core/src/testutil.rs @@ -286,8 +286,35 @@ pub struct SeedSpec { /// file cannot be reopened without it. Ignored on a plain arm, which has /// no reserve. pub hmac: Option, + /// `(extension, mime)` pairs cycled across the rows, deciding what a + /// `content_extensions` filter can select. [`EXT_PLAIN`] — one pair, so + /// every row is `.txt`/`text/plain` — is what every harness measuring + /// search or indexing wants, and it is the default so their corpora are + /// byte-identical to what they have always been. + /// + /// It exists for `contentprobe`, where a filter that either takes the + /// whole index or none of it answers nothing. **A mix whose length shares + /// a factor with `content_every` puts every document behind the same few + /// extensions** — the degenerate-corpus trap `pruneprobe` documents + /// against its own strides — so a harness using this should assert the + /// fractions it ends up with rather than trusting the arithmetic. + pub ext_mix: &'static [(&'static str, &'static str)], + /// One row in every `pending_every` that *would* hold content is left in + /// the pending queue instead — born `STATE_PENDING` and never extracted, + /// which is the residue an interrupted content pass leaves behind. `0` + /// (the default) seeds none. + /// + /// It exists because such a row is the case a re-decision can skip the + /// most work on: it is neither `STATE_NA` (so a narrowed filter must still + /// flip it) nor `STATE_DONE` (so it has no posting and no stored text to + /// clear). A corpus without any cannot tell whether clearing content for + /// rows that cannot hold it costs anything. + pub pending_every: usize, } +/// The single-extension corpus every harness but `contentprobe` seeds. +pub const EXT_PLAIN: &[(&str, &str)] = &[("txt", "text/plain")]; + impl Default for SeedSpec { fn default() -> SeedSpec { SeedSpec { @@ -306,6 +333,8 @@ impl Default for SeedSpec { page_size: None, pgsz: None, hmac: None, + ext_mix: EXT_PLAIN, + pending_every: 0, } } } @@ -314,7 +343,7 @@ impl Default for SeedSpec { /// measurement harnesses so they all describe the same corpus. pub fn seed_index(path: &std::path::Path, spec: &SeedSpec) { use crate::db::repo::{insert_file, set_content_done, NewFile}; - use crate::mime::FileType; + use crate::mime::mime_to_type; // Before the open, not after: the profile decides how the file is // *created*, and on a keyed file it decides whether it can be read at all. @@ -335,6 +364,19 @@ pub fn seed_index(path: &std::path::Path, spec: &SeedSpec) { .unwrap(); } let mut rng = Lcg::new(0x5eed); + let ext_mix = if spec.ext_mix.is_empty() { + EXT_PLAIN + } else { + spec.ext_mix + }; + // A row gets content only if an extractor would have claimed its MIME, so + // the seeded `content_state` is what a real run under an *unfiltered* + // config would have left. Without this a corpus of mixed extensions is + // born disagreeing with its own configuration, and the first reconcile + // against it spends its time repairing the seed rather than applying the + // edit. `EXT_PLAIN` is claimed by the plaintext extractor, so the + // single-extension corpus every other harness seeds is unchanged. + let registry = crate::extract::Registry::default_set(); // Spacing, not a random draw: a cluster at the front would let a pass // stop early and report a fraction of the work a real rare query costs. let name_stride = spec.files / spec.needle_names.max(1); @@ -346,10 +388,14 @@ pub fn seed_index(path: &std::path::Path, spec: &SeedSpec) { for i in 0..spec.files { let w1 = rng.pick(WORDS); let w2 = rng.pick(WORDS); + // Extension and MIME move together: a row whose name says `.pdf` and + // whose MIME says `text/plain` would let `content_extractable`'s two + // halves disagree, which is exactly what a content filter is testing. + let (ext, mime) = ext_mix[i % ext_mix.len()]; let name = if spec.needle_names > 0 && i % name_stride.max(1) == 0 { - format!("{}-{}-{:07}.txt", w1, NEEDLE, i) + format!("{}-{}-{:07}.{}", w1, NEEDLE, i, ext) } else { - format!("{}-{}-{:07}.txt", w1, w2, i) + format!("{}-{}-{:07}.{}", w1, w2, i, ext) }; // Stored parents always end in a separator; see `dir_to_db_parent`. // Deeper segments are derived from the directory index, not the file @@ -376,6 +422,11 @@ pub fn seed_index(path: &std::path::Path, spec: &SeedSpec) { } bytes }); + // `needs_content` is what the row is *born* as — `insert_file` gives it + // `STATE_PENDING`. Skipping the `set_content_done` below is therefore + // all it takes to leave one behind in the queue. + let needs_content = i % spec.content_every.max(1) == 0 && registry.supports(mime); + let extracted = needs_content && (spec.pending_every == 0 || i % spec.pending_every != 0); let id = insert_file( &tx, &NewFile { @@ -383,15 +434,15 @@ pub fn seed_index(path: &std::path::Path, spec: &SeedSpec) { parent: &dir, size: 4096, mtime: 1_700_000_000 + i as u64, - mime: Some("text/plain"), - ftype: FileType::TEXT, + mime: Some(mime), + ftype: mime_to_type(mime), hash: hash.as_ref().map(|h| h.as_slice()), - needs_content: i % spec.content_every.max(1) == 0, + needs_content, }, ) .unwrap() .expect("unique path"); - if i % spec.content_every.max(1) == 0 { + if extracted { let mut body: Vec<&str> = (0..spec.body_words).map(|_| *rng.pick(WORDS)).collect(); if spec.needle_docs > 0 && i % doc_stride.max(1) == 0 { // Mid-body, so a snippet window has to be cut around it. diff --git a/crates/quicksearch-gui/Cargo.toml b/crates/quicksearch-gui/Cargo.toml index ef6ced0..4a19c66 100644 --- a/crates/quicksearch-gui/Cargo.toml +++ b/crates/quicksearch-gui/Cargo.toml @@ -109,4 +109,8 @@ windows-sys = { version = "0.59", features = [ "Win32_Storage_FileSystem", "Win32_System_IO", "Win32_Security", + # The foreground handshake (`activate`) and native raise (`activate::raise`): + # GetCurrentProcessId, AllowSetForegroundWindow / SetForegroundWindow. + "Win32_System_Threading", + "Win32_UI_WindowsAndMessaging", ] } diff --git a/crates/quicksearch-gui/src/activate.rs b/crates/quicksearch-gui/src/activate.rs index 8d9caac..ba44898 100644 --- a/crates/quicksearch-gui/src/activate.rs +++ b/crates/quicksearch-gui/src/activate.rs @@ -7,9 +7,12 @@ //! box whether or not the app was started", because a shortcut an application //! registers for itself cannot fire while the application is not there. //! -//! The message carries nothing: "come forward" is the whole protocol, and -//! the reply exists only so the sender can tell a live instance from a -//! leftover socket. An xdg-activation token would be the natural thing to +//! The message carries nothing: "come forward" is the whole protocol. On +//! unix the reply exists only so the sender can tell a live instance from a +//! leftover socket; on Windows it instead carries the server's PID, which +//! the sender feeds to `AllowSetForegroundWindow` so the running window may +//! actually take the foreground — see the `#[cfg(windows)]` module below. +//! An xdg-activation token would be the natural thing to //! carry — it is what a compositor wants before letting a background client //! take focus — but nothing downstream can consume one: winit 0.30 applies a //! token only in `WindowAttributes`, and egui's `ViewportBuilder` has no @@ -28,6 +31,21 @@ use std::sync::atomic::{AtomicBool, Ordering}; /// the same thing as one. static PENDING: AtomicBool = AtomicBool::new(false); +/// The most a PID reply can be: a 32-bit PID is at most ten digits, and the +/// newline ends it. Also the pipe's buffer size, so the server's write never +/// blocks on a client that reads nothing. +#[cfg_attr(not(windows), allow(dead_code))] +const PID_REPLY_CAP: usize = 16; + +/// The server's PID out of its reply: ASCII decimal up to a newline. +/// Anything else — truncation, garbage, an empty read — is `None`, which +/// skips the foreground grant rather than failing the activation. +#[cfg_attr(not(windows), allow(dead_code))] +fn parse_pid_reply(reply: &[u8]) -> Option { + let line = reply.split(|&b| b == b'\n').next()?; + std::str::from_utf8(line).ok()?.parse().ok() +} + /// The socket identifying the instance that `config_path` configures. /// /// **Keyed by the config file, not the index.** The index path is a setting @@ -190,9 +208,15 @@ mod imp { /// Runs until the process exits; a connection is one activation. fn serve(ctx: &egui::Context, listener: UnixListener) { for stream in listener.incoming() { - let Ok(stream) = stream else { continue }; - match answer(stream) { - Ok(()) => fire(ctx), + let Ok(mut stream) = stream else { continue }; + match request(&mut stream) { + // Fire *before* the ack: a client that saw the reply may act + // on "delivered", and delivered means the window was already + // asked to come forward. + Ok(()) => { + fire(ctx); + let _ = acknowledge(&mut stream); + } // One stalled or truncated peer must not stop the loop, and // must not raise the window on a request it never finished. Err(e) => quicksearch_core::log_warn!("a search shortcut request: {}", e), @@ -200,13 +224,13 @@ mod imp { } } - /// Read the request and acknowledge it. + /// Read the request. /// /// Hostile input is the norm rather than the exception: any process of /// this user can connect. The read is bounded in both bytes and time, so /// a peer that connects and stalls cannot wedge the one thread that /// answers every activation. - pub(super) fn answer(mut stream: UnixStream) -> std::io::Result<()> { + pub(super) fn request(stream: &mut UnixStream) -> std::io::Result<()> { let timeout = std::time::Duration::from_secs(5); stream.set_read_timeout(Some(timeout))?; stream.set_write_timeout(Some(timeout))?; @@ -214,8 +238,12 @@ mod imp { // One byte is the whole request; the cap is what keeps a peer from // holding this thread for as long as it cares to send. let mut scratch = [0u8; 1]; - stream.read_exact(&mut scratch)?; + stream.read_exact(&mut scratch) + } + /// The reply that lets the sender tell a live instance from a leftover + /// socket file. + pub(super) fn acknowledge(stream: &mut UnixStream) -> std::io::Result<()> { stream.write_all(b"\n")?; stream.flush() } @@ -224,11 +252,17 @@ mod imp { /// The same handshake over a named pipe, which is what Windows has instead /// of a unix socket. /// -/// **One deliberate difference: there is no reply.** A named pipe exists only -/// while a server holds an instance open — there is no file left behind — so -/// a successful `CreateFileW` already proves a live instance accepted us, and -/// the reply the unix side needs to tell a listener from a leftover socket -/// would be dead weight here. +/// **One deliberate difference: the reply is the foreground grant, not a +/// liveness check.** A named pipe exists only while a server holds an +/// instance open — there is no file left behind — so a successful +/// `CreateFileW` already proves a live instance accepted us. What Windows +/// *does* need is permission: `SetForegroundWindow` is refused to a process +/// the user did not just interact with, and the running instance is exactly +/// that. The `--toggle` process was launched by the user's keypress and so +/// holds the right — and may donate it with `AllowSetForegroundWindow`, given +/// the server's PID. The server therefore opens every connection by writing +/// its PID, and the sender grants before sending the request, so the grant +/// is always in force by the time the server raises. #[cfg(windows)] mod imp { use super::*; @@ -247,6 +281,8 @@ mod imp { ConnectNamedPipe, CreateNamedPipeW, DisconnectNamedPipe, PIPE_READMODE_BYTE, PIPE_TYPE_BYTE, PIPE_UNLIMITED_INSTANCES, PIPE_WAIT, }; + use windows_sys::Win32::System::Threading::GetCurrentProcessId; + use windows_sys::Win32::UI::WindowsAndMessaging::AllowSetForegroundWindow; /// `\\.\pipe\quicksearch-`, keyed exactly as the unix socket is, so /// the two processes agree by the same rule on both platforms. @@ -300,6 +336,29 @@ mod imp { return false; } let pipe = Handle(handle); + // The server opens with its PID; hand it the foreground right we + // hold from the user's keypress *before* asking it to raise. A + // reply that does not parse skips the grant — the raise then + // degrades to a taskbar flash rather than the request being lost. + let mut reply = [0u8; PID_REPLY_CAP]; + let mut got = 0u32; + // SAFETY: `reply` and `got` are live for the call; the buffer + // length passed is the buffer's real length. + let ok = unsafe { + ReadFile( + pipe.0, + reply.as_mut_ptr(), + reply.len() as u32, + &mut got, + std::ptr::null_mut(), + ) + }; + if ok != 0 { + if let Some(pid) = parse_pid_reply(&reply[..got as usize]) { + // SAFETY: no pointers; any PID value is acceptable input. + unsafe { AllowSetForegroundWindow(pid) }; + } + } let mut written = 0u32; // SAFETY: a one-byte buffer and an output slot, both live here. let ok = unsafe { @@ -349,8 +408,8 @@ mod imp { PIPE_ACCESS_DUPLEX, PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT, PIPE_UNLIMITED_INSTANCES, - 16, - 16, + PID_REPLY_CAP as u32, + PID_REPLY_CAP as u32, 0, std::ptr::null(), ) @@ -368,6 +427,28 @@ mod imp { // the read below is what decides, so it is not checked here. let _ = connected; + // Our PID first, before reading anything: the client turns it + // into a foreground grant and only then sends the request, so + // the grant precedes the raise however the two threads interleave. + // SAFETY: reads this process's own id, always valid. + let pid = format!("{}\n", unsafe { GetCurrentProcessId() }); + let mut written = 0u32; + // SAFETY: the buffer and output slot are live; the length passed + // is the buffer's real length. + let wrote = unsafe { + WriteFile( + pipe.0, + pid.as_ptr(), + pid.len() as u32, + &mut written, + std::ptr::null_mut(), + ) + }; + if wrote != 0 { + // SAFETY: the handle is open for the length of this call. + unsafe { FlushFileBuffers(pipe.0) }; + } + let mut scratch = [0u8; 1]; let mut read = 0u32; // SAFETY: a one-byte buffer and an output slot, both live here. @@ -380,12 +461,14 @@ mod imp { std::ptr::null_mut(), ) }; - // SAFETY: the handle is open for the length of this call. - unsafe { DisconnectNamedPipe(pipe.0) }; // A peer that connected and said nothing is not an activation. + // Fired before the disconnect, so a client watching the pipe + // close can already rely on the window having been asked. if ok != 0 && read == 1 { fire(ctx); } + // SAFETY: the handle is open for the length of this call. + unsafe { DisconnectNamedPipe(pipe.0) }; } } } @@ -436,6 +519,25 @@ mod tests { assert_ne!(a, b); } + /// The Windows reply parser, which faces whatever a squatting process + /// cares to write into the well-known pipe name. + #[test] + fn the_pid_reply_parses_strictly() { + assert_eq!(parse_pid_reply(b"12345\n"), Some(12345)); + assert_eq!(parse_pid_reply(b"1\nrest ignored"), Some(1)); + for garbage in [ + &b""[..], + b"\n", + b"-4\n", + b"12345678901234567890\n", // overflows a u32 + b"abc\n", + b"12 34\n", + b"\xff\xfe\n", + ] { + assert_eq!(parse_pid_reply(garbage), None, "{:?}", garbage); + } + } + #[test] fn a_pending_activation_is_consumed_once() { let _serial = pending_guard(); @@ -482,12 +584,13 @@ mod tests { let sender = std::thread::spawn(move || signal(&db)); - let stream = listener + let mut stream = listener .incoming() .next() .expect("a connection") .expect("accepted"); - imp::answer(stream).expect("a well-formed request"); + imp::request(&mut stream).expect("a well-formed request"); + imp::acknowledge(&mut stream).expect("acknowledged"); assert!(sender.join().expect("sender"), "the client saw the reply"); } @@ -525,12 +628,15 @@ mod tests { let peer = UnixStream::connect(&path).expect("connect"); drop(peer); - let stream = listener + let mut stream = listener .incoming() .next() .expect("a connection") .expect("accepted"); - assert!(imp::answer(stream).is_err(), "an empty request is refused"); + assert!( + imp::request(&mut stream).is_err(), + "an empty request is refused" + ); } /// A config path unique to this test. Never opened as a file — only @@ -585,6 +691,16 @@ mod tests { std::thread::sleep(std::time::Duration::from_millis(20)); } assert!(delivered, "the listener never answered"); - assert!(take_pending(), "and the window was asked to come forward"); + // Unlike unix there is no ack after the fire: the server reads the + // request after the client's write returns, so give it a moment. + let mut fired = false; + for _ in 0..100 { + if take_pending() { + fired = true; + break; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + assert!(fired, "the window was never asked to come forward"); } } diff --git a/crates/quicksearch-gui/src/activate/raise.rs b/crates/quicksearch-gui/src/activate/raise.rs index f3c1229..c0797f5 100644 --- a/crates/quicksearch-gui/src/activate/raise.rs +++ b/crates/quicksearch-gui/src/activate/raise.rs @@ -20,8 +20,14 @@ //! which shows up as a highlighted task entry. Closing that gap means //! patching both winit (to activate a live surface) and eframe (to carry //! the token), the way `vendor/` already patches two other crates. -//! * **Windows**: `SetForegroundWindow` is refused to background processes, -//! so the same limit applies to a process that did not just receive input. +//! * **Windows**: `SetForegroundWindow` is refused to background processes — +//! but the `--toggle` sender was launched by the user's keypress and +//! donates its right over the pipe with `AllowSetForegroundWindow` (see +//! `crate::activate`'s Windows module), after which [`win32_activate`]'s +//! `SetForegroundWindow` is honoured. The in-app hotkey path needs no +//! grant: the press was delivered to this process. Only the portal-less +//! case with no grant — e.g. some other process poking the pipe — degrades +//! to a taskbar flash. /// Whether this is a Wayland session, where an already-open window cannot be /// raised. The Settings tab says so rather than letting the shortcut look @@ -50,7 +56,13 @@ pub fn raise(ctx: &egui::Context, frame: &eframe::Frame) { return; } } - #[cfg(not(all(unix, not(target_os = "macos"))))] + #[cfg(windows)] + { + if win32_activate(frame) { + return; + } + } + #[cfg(not(any(all(unix, not(target_os = "macos")), windows)))] { let _ = frame; } @@ -60,6 +72,56 @@ pub fn raise(ctx: &egui::Context, frame: &eframe::Frame) { ctx.send_viewport_cmd(egui::ViewportCommand::Focus); } +/// Restore and foreground our window with the Win32 calls themselves. +/// `false` falls back to winit's viewport commands. +/// +/// Not `ViewportCommand::Focus`: winit's `focus_window` routes through the +/// same `SetForegroundWindow`, but only after the event loop wakes and with +/// its own preconditions, and it neither restores a minimised window nor +/// reports failure. Calling the API here keeps restore-then-foreground in +/// one place, immediately, while the `AllowSetForegroundWindow` grant from +/// the `--toggle` sender is fresh. +#[cfg(windows)] +fn win32_activate(frame: &eframe::Frame) -> bool { + use raw_window_handle::{HasWindowHandle, RawWindowHandle}; + use windows_sys::Win32::UI::WindowsAndMessaging::{ + IsIconic, SetForegroundWindow, ShowWindow, SW_RESTORE, + }; + + let handle = match frame.window_handle() { + Ok(handle) => handle, + Err(e) => { + quicksearch_core::log_warn!("raising the window: no window handle: {}", e); + return false; + } + }; + let hwnd = match handle.as_raw() { + RawWindowHandle::Win32(win32) => win32.hwnd.get() as _, + other => { + quicksearch_core::log_warn!("raising the window: not a Win32 window: {:?}", other); + return false; + } + }; + // SAFETY: `hwnd` is this process's live window for the whole call; these + // APIs accept any window handle and merely fail on a bad one. + unsafe { + if IsIconic(hwnd) != 0 { + ShowWindow(hwnd, SW_RESTORE); + } + if SetForegroundWindow(hwnd) == 0 { + // No grant in force (see the module docs): the most Windows + // allows from here is a taskbar flash, which the winit fallback + // produces. Logged so a shortcut that only flashes is traceable. + quicksearch_core::log_warn!( + "raising the window: SetForegroundWindow was refused; \ + flashing the taskbar instead" + ); + return false; + } + } + true +} + // The X connection used for activation, kept open across presses. // Thread-local because `raise` only ever runs on the UI thread, and held // rather than reconnected because a connect per keypress is both wasteful diff --git a/crates/quicksearch-gui/src/app.rs b/crates/quicksearch-gui/src/app.rs index f0ae0ac..aa59354 100644 --- a/crates/quicksearch-gui/src/app.rs +++ b/crates/quicksearch-gui/src/app.rs @@ -391,6 +391,8 @@ impl QuickSearchApp { // Only when moved: on Wayland re-registering opens a new portal // session, which some desktops confirm with the user. crate::hotkey::apply(&new.ui.search_hotkey); + // And the system-wide binding follows, where one is written. + crate::shortcut_setup::hotkey_changed(&new.ui.search_hotkey); } if new.ui.color_scheme != self.cfg.ui.color_scheme { apply_theme(ctx, &new.ui.color_scheme); diff --git a/crates/quicksearch-gui/src/app/modals.rs b/crates/quicksearch-gui/src/app/modals.rs index c659824..b49af4a 100644 --- a/crates/quicksearch-gui/src/app/modals.rs +++ b/crates/quicksearch-gui/src/app/modals.rs @@ -213,20 +213,20 @@ impl QuickSearchApp { if actions.focus_search { self.search.request_focus(); } - // Live, like the Settings slider on Apply — but saved only when the - // drag ends, so crossing the slider does not rewrite the config file - // on every frame. + // Only ever emitted by the page's Apply button, so applying and + // saving belong together — the same pair the Settings tab's Apply + // performs. if let Some(scale) = actions.set_scale { self.cfg.ui.scale = scale; ctx.set_zoom_factor(super::clamp_scale(scale)); - if actions.save_scale { - self.save_cfg(); - } + self.save_cfg(); } // Registered as it is captured, not on an Apply the tour has no // button for — the page says it takes effect at once. if let Some(hotkey) = actions.set_hotkey { crate::hotkey::apply(&hotkey); + // The system-wide binding follows, where one is written. + crate::shortcut_setup::hotkey_changed(&hotkey); self.cfg.ui.search_hotkey = hotkey; self.save_cfg(); } diff --git a/crates/quicksearch-gui/src/help_tab.rs b/crates/quicksearch-gui/src/help_tab.rs index d3fccf6..39adadb 100644 --- a/crates/quicksearch-gui/src/help_tab.rs +++ b/crates/quicksearch-gui/src/help_tab.rs @@ -232,34 +232,50 @@ fn ranking_section(ui: &mut egui::Ui) { .spacing([CELL_SPACING, 5.0]) .striped(true) .show(ui, |ui| { - let row = |ui: &mut egui::Ui, tier: &str, what: &str| { - ui.strong(tier); + // Each tier's chip wears the colour its results wear in the Rank + // column, keyed by the *first* cascade stage the collapsed tier + // covers (see the rank table in `search::cascade`): exact name + // 1–2, name contains 3–4, text inside 5–6, fuzzy 7–8, path 9–11. + let row = |ui: &mut egui::Ui, stage: u8, tier: &str, what: &str| { + ui.label( + egui::RichText::new(format!(" {} ", tier)) + .strong() + .background_color(crate::color::rank_tier_color(stage)) + // The same near-black the Search tab's chips carry, + // which every ramp colour holds contrast against. + .color(egui::Color32::from_rgb(32, 32, 32)), + ); cell(ui, prose, what); ui.end_row(); }; row( ui, + 1, "Exact name", "the file is called exactly what you typed", ); row( ui, + 3, "Name contains", "what you typed appears somewhere in the file's name", ); row( ui, + 5, "Text inside", "the words are in the file's contents, most mentions first", ); row( ui, + 7, "Close spelling", "a name or some text within a typo or two of what you typed, \ only while Fuzzy is ticked", ); row( ui, + 9, "Path only", "nothing in the name or the text matched, but a folder along \ the way did", @@ -601,8 +617,9 @@ mod tests { #[test] fn a_window_narrower_than_the_column_reflows_rather_than_clipping() { // 640 is the smallest window the app allows, and the UI scale - // divides it: 400 is roughly that window at 1.6x. - for width in [400.0_f32, 480.0, 560.0, 620.0] { + // divides it: 400 is roughly that window at 1.6x, and 250 is it at + // the 2.5x ceiling — the narrowest layout the app can produce. + for width in [250.0_f32, 320.0, 400.0, 480.0, 560.0, 620.0] { let ctx = crate::test_ui::ctx(); let input = crate::test_ui::raw_input(egui::vec2(width, 6000.0), vec![]); let out = ctx.run(input, |ctx| { @@ -624,6 +641,39 @@ mod tests { } } + /// Every tier chip wears the Rank column's own colour for its first + /// cascade stage — the chips exist to demonstrate the blue→red ramp the + /// paragraph under the table describes. + #[test] + fn the_ranking_tiers_wear_the_rank_colors() { + let ctx = crate::test_ui::ctx(); + let input = crate::test_ui::raw_input(egui::vec2(1000.0, 4000.0), vec![]); + let out = ctx.run(input, |ctx| { + egui::CentralPanel::default().show(ctx, |ui| { + super::ui(ui); + }); + }); + // A RichText background is a section format in the galley, not a + // separate rect shape. + let mut backgrounds = Vec::new(); + for clipped in &out.shapes { + if let egui::epaint::Shape::Text(text) = &clipped.shape { + for section in &text.galley.job.sections { + backgrounds.push(section.format.background); + } + } + } + for stage in [1u8, 3, 5, 7, 9] { + let color = crate::color::rank_tier_color(stage); + assert!( + backgrounds.contains(&color), + "no chip painted in stage {}'s colour {:?}", + stage, + color + ); + } + } + /// The near-miss column is half of what the examples teach, so the /// narrow layout has to keep it rather than dropping to pattern-only. #[test] diff --git a/crates/quicksearch-gui/src/hotkey/binding.rs b/crates/quicksearch-gui/src/hotkey/binding.rs index a50acdc..bb28a7b 100644 --- a/crates/quicksearch-gui/src/hotkey/binding.rs +++ b/crates/quicksearch-gui/src/hotkey/binding.rs @@ -156,6 +156,24 @@ impl Binding { .expect("every Binding key comes from KEYS") } + /// The accelerator in GTK's syntax — `f` — which is what a + /// GNOME custom keybinding's `binding` key stores. GTK keyval names are + /// the X11 keysym names, so the keysym column serves both spellings. + pub fn gtk_accelerator(&self) -> String { + let mut out = String::new(); + for (held, name) in [ + (self.ctrl, ""), + (self.alt, ""), + (self.shift, ""), + ] { + if held { + out.push_str(name); + } + } + out.push_str(self.row().1); + out + } + /// The trigger in the XDG shortcuts spec's syntax: uppercase modifiers /// and an xkbcommon keysym, joined with `+`. pub fn portal_trigger(&self) -> String { @@ -250,6 +268,17 @@ mod tests { let binding: Binding = cfg.search_hotkey.parse().expect("the default is valid"); assert_eq!(binding.to_string(), "Ctrl+Shift+F"); assert_eq!(binding.portal_trigger(), "CTRL+SHIFT+f"); + assert_eq!(binding.gtk_accelerator(), "f"); + } + + /// The keysym column doubles as the GTK keyval, so a named key must come + /// out under GTK's name for it, not egui's. + #[test] + fn gtk_accelerators_use_keysym_names() { + let binding: Binding = "Ctrl+Alt+PageUp".parse().unwrap(); + assert_eq!(binding.gtk_accelerator(), "Prior"); + let binding: Binding = "Shift+Enter".parse().unwrap(); + assert_eq!(binding.gtk_accelerator(), "Return"); } #[test] diff --git a/crates/quicksearch-gui/src/main.rs b/crates/quicksearch-gui/src/main.rs index 108c41a..b11c780 100644 --- a/crates/quicksearch-gui/src/main.rs +++ b/crates/quicksearch-gui/src/main.rs @@ -30,6 +30,7 @@ mod platform; mod query_highlight; mod search_tab; mod settings_tab; +mod shortcut_setup; mod spotlight; #[cfg(test)] mod test_ui; @@ -118,8 +119,17 @@ fn main() { // Losing the race to an instance that came up between the signal // above and here, or a plain second launch: either way the user // asked to see QuickSearch, and there is one to show them. - if activate::signal(&Config::config_path()) { - return; + // + // Retried, not tried once: the winner holds the lock the moment + // `main` reaches it but only listens once eframe's creation + // closure has run, so a `--toggle` landing in that gap would see + // the lock held and no socket. Two seconds outlasts that gap by + // orders of magnitude; a wedged instance still gets the dialog. + for _ in 0..20 { + if activate::signal(&Config::config_path()) { + return; + } + std::thread::sleep(std::time::Duration::from_millis(100)); } let who = match pid { Some(pid) => format!(" (process {})", pid), diff --git a/crates/quicksearch-gui/src/manage_tab.rs b/crates/quicksearch-gui/src/manage_tab.rs index 358c63f..65014c6 100644 --- a/crates/quicksearch-gui/src/manage_tab.rs +++ b/crates/quicksearch-gui/src/manage_tab.rs @@ -447,8 +447,8 @@ impl ManageTab { egui::TextEdit::multiline(&mut self.ext_filter_text) .desired_rows(4) .desired_width(f32::INFINITY) - .hint_text("#EXAMPLE WHITELISTED FILE EXTENSIONS FOR FULL-TEXT-SEARCH:\n#MOUSE \ - OVER FOR MORE INFO\n#------------------------------------------------\ntxt\nmd\n \ + .hint_text("#EXAMPLE WHITELISTED FILE EXTENSIONS FOR FULL-TEXT-SEARCH\n#MOUSE \ + OVER FOR MORE INFO\ntxt\nmd\n \ pdf # comments allowed\n(none)"), ) .tip(&tips::EXT_WHITELIST); diff --git a/crates/quicksearch-gui/src/settings_tab.rs b/crates/quicksearch-gui/src/settings_tab.rs index d84d792..ca338a8 100644 --- a/crates/quicksearch-gui/src/settings_tab.rs +++ b/crates/quicksearch-gui/src/settings_tab.rs @@ -293,7 +293,7 @@ impl SettingsTab { ); }); hotkey_note(ui, &draft.ui.search_hotkey, ¤t.ui.search_hotkey); - shortcut_note(ui); + shortcut_note(ui, ¤t.ui.search_hotkey); ui.separator(); // Security acts on the live config, not the draft; the KDF @@ -378,7 +378,7 @@ pub(crate) fn hotkey_edit( let p = crate::color::palette(ui.visuals().dark_mode); ui.horizontal(|ui| { let label = if *capturing { - "Press a key combination...".to_string() + "Press a key combination…".to_string() } else if setting.trim().is_empty() { "None".to_string() } else { @@ -488,26 +488,123 @@ fn hotkey_note(ui: &mut egui::Ui, draft: &str, live: &str) { /// /// The shortcut above is ours and needs no setup, but it cannot fire while /// QuickSearch is not running — see `crate::activate`. Only the desktop can -/// bind a key that launches something, so this says what to bind and opens -/// the place to bind it. Writing the desktop's own configuration instead was -/// considered and rejected: it differs per desktop and between versions of -/// the same one, and a shortcut we wrote and the user cannot see is worse -/// than one they created. +/// bind a key that launches something. Where the desktop's own configuration +/// has a known home for that binding (`crate::shortcut_setup`), one button +/// writes it — into the place the desktop's settings UI lists and edits, so +/// it stays the user's to see and change. Everywhere else this says what to +/// bind and opens the place to bind it. +/// +/// `hotkey_setting` is the shortcut in force, which the one-click binding +/// mirrors system-wide. /// /// Shared with the tour's shortcut page, which puts it under the same /// shortcut button this sentence says is "above". -pub(crate) fn shortcut_note(ui: &mut egui::Ui) { +pub(crate) fn shortcut_note(ui: &mut egui::Ui, hotkey_setting: &str) { + // Tests reach this through the tour's shortcut page; probing the real + // desktop there would make them depend on the machine they run on and + // spawn `gsettings`. Tests that want a desktop pin one through + // `shortcut_note_for`. + let desktop = if cfg!(test) { + crate::shortcut_setup::Desktop::Unsupported + } else { + crate::shortcut_setup::detect() + }; + shortcut_note_for(ui, hotkey_setting, desktop); +} + +/// One frame's answer from [`crate::shortcut_setup`], cached: `installed` +/// probes the desktop with a subprocess, which must not run per frame. +#[derive(Clone)] +struct SystemShortcutState { + installed: bool, + /// The last install/remove outcome, `(succeeded, what to say)`. + feedback: Option<(bool, String)>, +} + +/// The desktop split out of the environment so tests can pick one. +fn shortcut_note_for(ui: &mut egui::Ui, hotkey_setting: &str, desktop: crate::shortcut_setup::Desktop) { let command = format!("{} --toggle", crate::activate::command_name()); crate::ui_util::stable_section(ui, |ui| { - ui.label( - egui::RichText::new( - "The shortcut above works while QuickSearch is open. To have a key \ - start it as well, bind this command in your desktop's keyboard \ - settings:", - ) - .small() - .weak(), - ); + // One-click only with a key to write: an unset or unparseable + // shortcut leaves nothing to bind system-wide. + let binding = crate::hotkey::parse_setting(hotkey_setting).ok().flatten(); + let one_click = binding.filter(|_| desktop != crate::shortcut_setup::Desktop::Unsupported); + + if let Some(binding) = one_click { + // One id for the settings tab and the tour: they show one fact. + let id = egui::Id::new("system-shortcut-state"); + let mut state = ui + .data_mut(|d| d.get_temp::(id)) + .unwrap_or_else(|| SystemShortcutState { + installed: crate::shortcut_setup::installed(), + feedback: None, + }); + ui.label( + egui::RichText::new( + "The shortcut above works while QuickSearch is open. Your \ + desktop can also bind it to start QuickSearch when it is \ + not:", + ) + .small() + .weak(), + ); + ui.horizontal_wrapped(|ui| { + if state.installed { + ui.label(egui::RichText::new("The system shortcut is set up.").small()); + if ui.add(egui::Button::new("Remove").small()).clicked() { + match crate::shortcut_setup::remove() { + Ok(()) => { + state.installed = false; + state.feedback = + Some((true, "System shortcut removed.".to_string())); + } + Err(e) => state.feedback = Some((false, e)), + } + } + } else if ui + .add(egui::Button::new(format!("Set up {} system-wide", binding)).small()) + .clicked() + { + match crate::shortcut_setup::install(&binding) { + Ok(()) => { + state.installed = true; + state.feedback = Some(( + true, + "Added to your desktop's keyboard shortcuts. If the \ + key does not answer right away, it will after the \ + next login." + .to_string(), + )); + } + Err(e) => state.feedback = Some((false, e)), + } + } + }); + if let Some((ok, text)) = &state.feedback { + let rich = egui::RichText::new(text).small(); + ui.label(if *ok { + rich.weak() + } else { + rich.color(crate::color::palette(ui.visuals().dark_mode).orange) + }); + } + ui.data_mut(|d| d.insert_temp(id, state)); + ui.label( + egui::RichText::new("Or bind this command there yourself:") + .small() + .weak(), + ); + } else { + ui.label( + egui::RichText::new( + "The shortcut above works while QuickSearch is open. To have a key \ + start it as well, bind this command in your desktop's keyboard \ + settings:", + ) + .small() + .weak(), + ); + } ui.horizontal_wrapped(|ui| { ui.label(egui::RichText::new(&command).small().monospace()); if ui.add(egui::Button::new("Copy").small()).clicked() { @@ -580,30 +677,30 @@ fn security_ui( } ui.horizontal(|ui| { if ui - .button("Change password…") + .button("Change password") .tip(&tips::CHANGE_PASSWORD) .clicked() { action = Some(SecurityAction::ChangePassword); } if ui - .button("Disable protection…") + .button("Disable protection") .tip(&tips::DISABLE_PASSWORD) .clicked() { action = Some(SecurityAction::Disable); } + // The raw key is for someone recovering the file by hand; the + // password controls beside it are for everyone. + if form.advanced + && ui + .button("Show database key") + .tip(&tips::SHOW_KEY) + .clicked() + { + action = Some(SecurityAction::ShowKey); + } }); - // The raw key is for someone recovering the file by hand; the password - // controls above it are for everyone. - if form.advanced - && ui - .button("Show database key…") - .tip(&tips::SHOW_KEY) - .clicked() - { - action = Some(SecurityAction::ShowKey); - } let mut remember = current.security.use_keychain; if ui .checkbox(&mut remember, "Remember on this device") @@ -615,7 +712,7 @@ fn security_ui( } else { ui.label("The index is not encrypted."); if ui - .button("Enable password protection…") + .button("Enable password protection") .tip(&tips::ENABLE_PASSWORD) .clicked() { diff --git a/crates/quicksearch-gui/src/settings_tab/tests.rs b/crates/quicksearch-gui/src/settings_tab/tests.rs index 8865076..7bd6266 100644 --- a/crates/quicksearch-gui/src/settings_tab/tests.rs +++ b/crates/quicksearch-gui/src/settings_tab/tests.rs @@ -559,7 +559,7 @@ fn the_key_button_appears_only_while_the_index_is_encrypted() { let (_, full) = run_security(&ctx, &cfg, vec![]); assert!( - painted_text_center(&full, "Show database key…").is_none(), + painted_text_center(&full, "Show database key").is_none(), "offered the key of an unencrypted index: {:?}", painted_text(&full) ); @@ -567,7 +567,7 @@ fn the_key_button_appears_only_while_the_index_is_encrypted() { cfg.security.password_protected = true; let (_, full) = run_security(&ctx, &cfg, vec![]); assert!( - painted_text_center(&full, "Show database key…").is_some(), + painted_text_center(&full, "Show database key").is_some(), "no key button while encrypted: {:?}", painted_text(&full) ); @@ -587,7 +587,7 @@ fn clicking_the_key_button_reports_show_key() { let (quiet, full) = run_security(&ctx, &cfg, vec![]); assert!(quiet.is_none(), "reported an action nobody clicked"); - let target = painted_text_center(&full, "Show database key…").expect("no key button"); + let target = painted_text_center(&full, "Show database key").expect("no key button"); let (action, _) = run_security(&ctx, &cfg, click_at(target)); assert_eq!(action, Some(SecurityAction::ShowKey)); @@ -730,13 +730,20 @@ fn showing_advanced_settings_is_not_an_unsaved_edit() { } /// The panel that tells a user how to get a shortcut that also starts -/// QuickSearch has to actually show the command they must bind. +/// QuickSearch has to actually show the command they must bind — with or +/// without a one-click desktop to lean on. #[test] fn the_shortcut_note_names_the_command_to_bind() { let ctx = crate::test_ui::ctx(); let input = crate::test_ui::raw_input(egui::vec2(700.0, 300.0), vec![]); let out = ctx.run(input, |ctx| { - egui::CentralPanel::default().show(ctx, |ui| super::shortcut_note(ui)); + egui::CentralPanel::default().show(ctx, |ui| { + super::shortcut_note_for( + ui, + "Ctrl+Shift+F", + crate::shortcut_setup::Desktop::Unsupported, + ) + }); }); let painted = painted_text(&out).join("\n"); assert!( @@ -745,3 +752,70 @@ fn the_shortcut_note_names_the_command_to_bind() { ); assert!(painted.contains("Copy"), "no way to copy it: {painted}"); } + +/// On a desktop we can write, the note leads with the one-click button (the +/// probe is skipped by seeding the cached state, so the test stays +/// hermetic), and the manual command stays as the fallback. +#[test] +fn a_supported_desktop_gets_the_one_click_button() { + let ctx = crate::test_ui::ctx(); + ctx.data_mut(|d| { + d.insert_temp( + egui::Id::new("system-shortcut-state"), + super::SystemShortcutState { + installed: false, + feedback: None, + }, + ) + }); + let input = crate::test_ui::raw_input(egui::vec2(700.0, 300.0), vec![]); + let out = ctx.run(input, |ctx| { + egui::CentralPanel::default().show(ctx, |ui| { + super::shortcut_note_for(ui, "Ctrl+Shift+F", crate::shortcut_setup::Desktop::Gnome) + }); + }); + let painted = painted_text(&out).join("\n"); + assert!( + painted.contains("Set up Ctrl+Shift+F system-wide"), + "no one-click button: {painted}" + ); + assert!(painted.contains("--toggle"), "the fallback vanished: {painted}"); + + // Already installed: the button flips to removal. + ctx.data_mut(|d| { + d.insert_temp( + egui::Id::new("system-shortcut-state"), + super::SystemShortcutState { + installed: true, + feedback: None, + }, + ) + }); + let input = crate::test_ui::raw_input(egui::vec2(700.0, 300.0), vec![]); + let out = ctx.run(input, |ctx| { + egui::CentralPanel::default().show(ctx, |ui| { + super::shortcut_note_for(ui, "Ctrl+Shift+F", crate::shortcut_setup::Desktop::Gnome) + }); + }); + let painted = painted_text(&out).join("\n"); + assert!(painted.contains("Remove"), "no removal offered: {painted}"); +} + +/// No usable binding means nothing to write: the one-click flow bows out +/// even on a supported desktop. +#[test] +fn no_binding_means_no_one_click_button() { + let ctx = crate::test_ui::ctx(); + let input = crate::test_ui::raw_input(egui::vec2(700.0, 300.0), vec![]); + let out = ctx.run(input, |ctx| { + egui::CentralPanel::default().show(ctx, |ui| { + super::shortcut_note_for(ui, "", crate::shortcut_setup::Desktop::Gnome) + }); + }); + let painted = painted_text(&out).join("\n"); + assert!( + !painted.contains("system-wide"), + "offered to bind nothing: {painted}" + ); + assert!(painted.contains("--toggle"), "the manual flow vanished: {painted}"); +} diff --git a/crates/quicksearch-gui/src/shortcut_setup.rs b/crates/quicksearch-gui/src/shortcut_setup.rs new file mode 100644 index 0000000..c10e56e --- /dev/null +++ b/crates/quicksearch-gui/src/shortcut_setup.rs @@ -0,0 +1,421 @@ +//! Writing the system-wide search shortcut into the desktop's own keyboard +//! configuration, so "bind this command yourself" becomes one click. +//! +//! Auto-writing was once rejected here on the grounds that a shortcut the +//! user cannot see is worse than one they created. The answer is to write it +//! exactly where the desktop's own settings UI lists and edits it — GNOME's +//! custom shortcuts, KDE's global shortcuts — so the binding stays the +//! user's to inspect, change or delete. Desktops without a known home for a +//! binding keep the manual copy-the-command flow in +//! `crate::settings_tab::shortcut_note`. +//! +//! On Windows the closed-app binding lives in the Start-menu `.lnk` the +//! installer creates, so there is nothing to *create* from here; what this +//! module does is keep that `.lnk`'s hotkey in step with the in-app one via +//! [`hotkey_changed`]. + +use crate::hotkey::Binding; + +/// Where a binding can be written. `Unsupported` hides the one-click button +/// and leaves the manual flow. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Desktop { + Gnome, + Kde, + Unsupported, +} + +pub fn detect() -> Desktop { + #[cfg(all(unix, not(target_os = "macos")))] + { + desktop_for(&std::env::var("XDG_CURRENT_DESKTOP").unwrap_or_default()) + } + #[cfg(not(all(unix, not(target_os = "macos"))))] + { + Desktop::Unsupported + } +} + +/// The mapping itself, split from the environment so it can be tested +/// without one; the same shape as `platform::keyboard_settings_for`. +#[cfg(all(unix, not(target_os = "macos")))] +fn desktop_for(desktops: &str) -> Desktop { + for desktop in desktops.split(':') { + match desktop.to_ascii_uppercase().as_str() { + "GNOME" | "UNITY" => return Desktop::Gnome, + "KDE" => return Desktop::Kde, + _ => {} + } + } + Desktop::Unsupported +} + +/// Whether the binding this module writes is currently present. Asks the +/// desktop, so callers should cache rather than poll every frame. +pub fn installed() -> bool { + match detect() { + Desktop::Gnome => gnome::installed(), + Desktop::Kde => kde::installed(), + Desktop::Unsupported => false, + } +} + +/// Write `binding` → `quicksearch --toggle` into the desktop's keyboard +/// configuration. Idempotent: a second install rewrites the same entry. +pub fn install(binding: &Binding) -> Result<(), String> { + let command = format!("{} --toggle", crate::activate::command_name()); + match detect() { + Desktop::Gnome => gnome::install(binding, &command), + Desktop::Kde => kde::install(binding), + Desktop::Unsupported => Err("this desktop is not supported".to_string()), + } +} + +/// Delete the entry [`install`] wrote; a no-op if it is already gone. +pub fn remove() -> Result<(), String> { + match detect() { + Desktop::Gnome => gnome::remove(), + Desktop::Kde => kde::remove(), + Desktop::Unsupported => Err("this desktop is not supported".to_string()), + } +} + +/// The in-app shortcut was rebound: keep the system-wide binding in step. +/// Best-effort — a failure is logged, not surfaced, because the change the +/// user asked for (the in-app key) already succeeded. +pub fn hotkey_changed(setting: &str) { + let Ok(Some(binding)) = crate::hotkey::parse_setting(setting) else { + // Cleared or unparseable: leave the system binding alone rather than + // guess; the Settings tab's own controls are the way to remove it. + return; + }; + #[cfg(windows)] + { + if let Err(e) = lnk::update_hotkey(&binding) { + quicksearch_core::log_warn!("updating the Start menu shortcut key: {}", e); + } + } + #[cfg(not(windows))] + { + if installed() { + if let Err(e) = install(&binding) { + quicksearch_core::log_warn!("updating the system search shortcut: {}", e); + } + } + } +} + +/// Run a program to completion and fail loudly, with its stderr as the why. +#[cfg(all(unix, not(target_os = "macos")))] +fn run(program: &str, args: &[&str]) -> Result { + let out = std::process::Command::new(program) + .args(args) + .output() + .map_err(|e| format!("running {}: {}", program, e))?; + if !out.status.success() { + let stderr = String::from_utf8_lossy(&out.stderr); + return Err(format!("{} failed: {}", program, stderr.trim())); + } + Ok(String::from_utf8_lossy(&out.stdout).into_owned()) +} + +/// GNOME: a custom keybinding under a path of our own. The fixed path is +/// what makes install idempotent and removal exact, and the entry shows up +/// in Settings → Keyboard → Custom Shortcuts under the name given here. +#[cfg(all(unix, not(target_os = "macos")))] +mod gnome { + use super::*; + + const LIST_SCHEMA: &str = "org.gnome.settings-daemon.plugins.media-keys"; + const LIST_KEY: &str = "custom-keybindings"; + const ENTRY_SCHEMA: &str = "org.gnome.settings-daemon.plugins.media-keys.custom-keybinding"; + pub(super) const ENTRY_PATH: &str = + "/org/gnome/settings-daemon/plugins/media-keys/custom-keybindings/quicksearch-search/"; + + pub(super) fn installed() -> bool { + run("gsettings", &["get", LIST_SCHEMA, LIST_KEY]) + .map(|list| parse_string_list(&list).iter().any(|p| p == ENTRY_PATH)) + .unwrap_or(false) + } + + pub(super) fn install(binding: &Binding, command: &str) -> Result<(), String> { + let entry = format!("{}:{}", ENTRY_SCHEMA, ENTRY_PATH); + for (key, value) in [ + ("name", "QuickSearch".to_string()), + ("command", command.to_string()), + ("binding", binding.gtk_accelerator()), + ] { + run("gsettings", &["set", &entry, key, &value])?; + } + // The entry only takes effect once its path is on the list; last, so + // a failure above cannot leave a listed entry with no command. + let list = run("gsettings", &["get", LIST_SCHEMA, LIST_KEY])?; + let mut paths = parse_string_list(&list); + if !paths.iter().any(|p| p == ENTRY_PATH) { + paths.push(ENTRY_PATH.to_string()); + let list = format_string_list(&paths); + run("gsettings", &["set", LIST_SCHEMA, LIST_KEY, &list])?; + } + Ok(()) + } + + pub(super) fn remove() -> Result<(), String> { + let list = run("gsettings", &["get", LIST_SCHEMA, LIST_KEY])?; + let paths: Vec = parse_string_list(&list) + .into_iter() + .filter(|p| p != ENTRY_PATH) + .collect(); + let list = format_string_list(&paths); + run("gsettings", &["set", LIST_SCHEMA, LIST_KEY, &list])?; + let entry = format!("{}:{}", ENTRY_SCHEMA, ENTRY_PATH); + run("gsettings", &["reset-recursively", &entry])?; + Ok(()) + } +} + +/// The GVariant `as` (array of strings) spelling `gsettings get` prints and +/// `gsettings set` accepts: `['a', 'b']`, or `@as []` when empty. +/// +/// The parser accepts exactly what gsettings emits — single-quoted strings +/// with `\'` and `\\` escapes — and drops anything malformed rather than +/// guessing: a path we misread would be written back verbatim into the +/// user's configuration. +#[cfg(all(unix, not(target_os = "macos")))] +fn parse_string_list(raw: &str) -> Vec { + let mut paths = Vec::new(); + let mut current = None; + let mut escaped = false; + for ch in raw.chars() { + match current.as_mut() { + None => { + if ch == '\'' { + current = Some(String::new()); + } + } + Some(path) => { + if escaped { + path.push(ch); + escaped = false; + } else if ch == '\\' { + escaped = true; + } else if ch == '\'' { + paths.push(current.take().expect("current is Some in this arm")); + } else { + path.push(ch); + } + } + } + } + paths +} + +#[cfg(all(unix, not(target_os = "macos")))] +fn format_string_list(paths: &[String]) -> String { + if paths.is_empty() { + // A bare `[]` has no type; this is the empty list gsettings prints. + return "@as []".to_string(); + } + let quoted: Vec = paths + .iter() + .map(|p| format!("'{}'", p.replace('\\', "\\\\").replace('\'', "\\'"))) + .collect(); + format!("[{}]", quoted.join(", ")) +} + +/// KDE: the global-shortcuts entry for the `Search` action that +/// `packaging/quicksearch.desktop` declares (`Exec=quicksearch --toggle`). +/// kglobalaccel launches desktop-file actions itself, so no command is +/// written here — only the key, in the file KDE's own Shortcuts settings +/// page reads and edits. +#[cfg(all(unix, not(target_os = "macos")))] +mod kde { + use super::*; + + const FILE: &str = "kglobalshortcutsrc"; + const GROUP: &str = "quicksearch.desktop"; + + /// Plasma 6's tool first; 5's second. The first present wins. + fn config_tool(names: [&'static str; 2]) -> &'static str { + let on_path = |name: &str| { + std::env::var_os("PATH").is_some_and(|path| { + std::env::split_paths(&path).any(|dir| dir.join(name).is_file()) + }) + }; + if on_path(names[0]) { + names[0] + } else { + names[1] + } + } + + /// The entry format is `active,default,description`. + pub(super) fn entry(binding: &Binding) -> String { + format!("{},none,Search", binding) + } + + pub(super) fn installed() -> bool { + let tool = config_tool(["kreadconfig6", "kreadconfig5"]); + run(tool, &["--file", FILE, "--group", GROUP, "--key", "Search"]) + .map(|out| { + let active = out.trim().split(',').next().unwrap_or(""); + !active.is_empty() && active != "none" + }) + .unwrap_or(false) + } + + pub(super) fn install(binding: &Binding) -> Result<(), String> { + let tool = config_tool(["kwriteconfig6", "kwriteconfig5"]); + let entry = entry(binding); + for (key, value) in [("_k_friendly_name", "QuickSearch"), ("Search", &entry)] { + run( + tool, + &["--file", FILE, "--group", GROUP, "--key", key, value], + )?; + } + reload(); + Ok(()) + } + + pub(super) fn remove() -> Result<(), String> { + let tool = config_tool(["kwriteconfig6", "kwriteconfig5"]); + for key in ["Search", "_k_friendly_name"] { + run( + tool, + &["--file", FILE, "--group", GROUP, "--key", key, "--delete"], + )?; + } + reload(); + Ok(()) + } + + /// Ask kglobalaccel to re-read its file. Best-effort: without it the + /// binding takes effect at the next login, which install's caller says. + fn reload() { + for qdbus in ["qdbus6", "qdbus"] { + if run( + qdbus, + &[ + "org.kde.kglobalaccel", + "/kglobalaccel", + "org.kde.KGlobalAccel.reloadConfig", + ], + ) + .is_ok() + { + return; + } + } + } +} + +/// Windows: the hotkey field of the Start-menu `.lnk` is the closed-app +/// binding (see `packaging/quicksearch.nsi`), rewritten through +/// `WScript.Shell` — whose `Hotkey` property takes exactly the +/// `Ctrl+Shift+F` spelling [`Binding`] displays — rather than through a COM +/// vtable of our own. +#[cfg(windows)] +mod lnk { + use super::*; + use std::path::PathBuf; + + fn start_menu_lnk(env: &str) -> Option { + let base = std::env::var_os(env)?; + let path = PathBuf::from(base).join(r"Microsoft\Windows\Start Menu\Programs\QuickSearch.lnk"); + path.is_file().then_some(path) + } + + pub(super) fn update_hotkey(binding: &Binding) -> Result<(), String> { + // The installer elevates and writes the all-users Start menu; a + // per-user one is checked first because that is the one this + // unelevated process can rewrite. + let lnk = start_menu_lnk("APPDATA") + .or_else(|| start_menu_lnk("ProgramData")) + .ok_or("no Start menu shortcut exists; re-run the installer")?; + let script = format!( + "$s = (New-Object -ComObject WScript.Shell).CreateShortcut('{}'); \ + $s.Hotkey = '{}'; $s.Save()", + lnk.display().to_string().replace('\'', "''"), + binding, + ); + let out = std::process::Command::new("powershell") + .args(["-NoProfile", "-NonInteractive", "-Command", &script]) + .output() + .map_err(|e| format!("running powershell: {}", e))?; + if !out.status.success() { + let stderr = String::from_utf8_lossy(&out.stderr); + // The common failure: the .lnk is the elevated installer's. + return Err(format!( + "could not rewrite {} ({}); if QuickSearch was installed for \ + all users, re-run the installer to change the key", + lnk.display(), + stderr.trim(), + )); + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[cfg(all(unix, not(target_os = "macos")))] + #[test] + fn the_desktop_is_detected_from_the_list_case_insensitively() { + assert_eq!(desktop_for("GNOME"), Desktop::Gnome); + assert_eq!(desktop_for("ubuntu:GNOME"), Desktop::Gnome); + assert_eq!(desktop_for("kde"), Desktop::Kde); + assert_eq!(desktop_for("Unity"), Desktop::Gnome); + assert_eq!(desktop_for(""), Desktop::Unsupported); + assert_eq!(desktop_for("i3:sway"), Desktop::Unsupported); + } + + /// Round trip through the exact spellings gsettings prints. + #[cfg(all(unix, not(target_os = "macos")))] + #[test] + fn the_gvariant_list_round_trips() { + assert_eq!(parse_string_list("@as []"), Vec::::new()); + assert_eq!(parse_string_list("[]"), Vec::::new()); + let two = parse_string_list("['/a/path/', '/b/path/']"); + assert_eq!(two, ["/a/path/", "/b/path/"]); + assert_eq!(format_string_list(&two), "['/a/path/', '/b/path/']"); + assert_eq!(format_string_list(&[]), "@as []"); + } + + /// A path with a quote in it must survive both directions, or the write + /// back would corrupt the user's other bindings. + #[cfg(all(unix, not(target_os = "macos")))] + #[test] + fn escaped_quotes_round_trip() { + let paths = vec!["/it's/".to_string(), "/back\\slash/".to_string()]; + let formatted = format_string_list(&paths); + assert_eq!(parse_string_list(&formatted), paths); + } + + /// Malicious or truncated gsettings output must never panic; at worst it + /// yields fewer paths. + #[cfg(all(unix, not(target_os = "macos")))] + #[test] + fn garbage_lists_parse_to_something_harmless() { + for garbage in ["", "[", "['unterminated", "not a list", "['a'", "\\"] { + let _ = parse_string_list(garbage); + } + } + + #[cfg(all(unix, not(target_os = "macos")))] + #[test] + fn the_kde_entry_carries_the_binding_first() { + let binding: Binding = "Ctrl+Shift+F".parse().unwrap(); + assert_eq!(kde::entry(&binding), "Ctrl+Shift+F,none,Search"); + } + + /// The fixed GNOME path is load-bearing twice over: idempotence and + /// exact removal both key on it. + #[cfg(all(unix, not(target_os = "macos")))] + #[test] + fn the_gnome_entry_path_is_fixed_and_well_formed() { + assert!(gnome::ENTRY_PATH.starts_with('/')); + assert!(gnome::ENTRY_PATH.ends_with('/')); + assert!(gnome::ENTRY_PATH.contains("quicksearch")); + } +} diff --git a/crates/quicksearch-gui/src/tips.rs b/crates/quicksearch-gui/src/tips.rs index 63c632b..e7dfb0b 100644 --- a/crates/quicksearch-gui/src/tips.rs +++ b/crates/quicksearch-gui/src/tips.rs @@ -506,9 +506,9 @@ pub static REMEMBER_KEYCHAIN: Tip = Tip { body: "Hands the key to the password store your system already has, such \ as GNOME Keyring, KWallet, or Windows Credential Manager, so that \ QuickSearch can unlock the index without asking at startup.\n\n\ - The password itself is never stored, only the key worked out from \ - it, and only on this machine. Off, you type the password each time \ - QuickSearch starts.", + The password itself is never stored, only key securely created from \ + it, and only on this machine. When off you must type the password each \ + time QuickSearch starts.", examples: &[], caution: None, }; diff --git a/crates/quicksearch-gui/src/tutorial.rs b/crates/quicksearch-gui/src/tutorial.rs index 70b4471..6ea5394 100644 --- a/crates/quicksearch-gui/src/tutorial.rs +++ b/crates/quicksearch-gui/src/tutorial.rs @@ -345,6 +345,9 @@ fn note(ui: &egui::Ui, text: impl Into) -> egui::RichText { struct Live<'a> { /// `ctx.zoom_factor()`, read before the window is laid out. zoom: f32, + /// The slider's position while it differs from `zoom` — chosen but not + /// yet applied. `None` once Apply is clicked or nothing is pending. + staged_scale: &'a mut Option, /// The shortcut in force, as the config spells it. hotkey: &'a str, capturing_hotkey: &'a mut bool, @@ -357,27 +360,24 @@ fn extra_ui(ui: &mut egui::Ui, extra: Extra, live: &mut Live, actions: &mut Tour ui.separator(); match extra { Extra::Scale => { - let mut scale = live.zoom; + // Staged until Apply, like the Settings tab's slider: applying + // mid-drag rescales the slider under the pointer, so the handle + // chases its own tail and the value cannot be chosen. + let mut scale = live.staged_scale.unwrap_or(live.zoom); ui.horizontal(|ui| { ui.label("UI scale"); // Scoped to this row, which is the whole of its use. ui.spacing_mut().slider_width = 220.0; - let slider = ui.add( + ui.add( egui::Slider::new(&mut scale, crate::app::SCALE_RANGE) .step_by(0.05) .fixed_decimals(2), ); - // Applied on every frame it moves, saved once it settles: a - // drag would otherwise rewrite the config file dozens of - // times on its way across. The release frame is not itself a - // change — the value stopped moving — so it is asked about - // separately, and carries the value with it so the app has - // one thing to act on. - let moved = slider.changed(); - let settled = slider.drag_stopped() || (moved && !slider.dragged()); - if moved || settled { + let pending = (scale - live.zoom).abs() > f32::EPSILON; + *live.staged_scale = pending.then_some(scale); + if ui.add_enabled(pending, egui::Button::new("Apply")).clicked() { actions.set_scale = Some(scale); - actions.save_scale = settled; + *live.staged_scale = None; } }); ui.label(note( @@ -399,7 +399,7 @@ fn extra_ui(ui: &mut egui::Ui, extra: Extra, live: &mut Live, actions: &mut Tour if setting != live.hotkey { actions.set_hotkey = Some(setting); } - crate::settings_tab::shortcut_note(ui); + crate::settings_tab::shortcut_note(ui, live.hotkey); } } } @@ -453,10 +453,10 @@ pub struct TourActions { pub goto_tab: Option, pub set_query: Option, pub focus_search: bool, - /// A new UI scale from the welcome page's slider, to apply live. + /// A new UI scale from the welcome page's slider, applied and saved on + /// its Apply button — never mid-drag, which would rescale the slider + /// under the pointer. pub set_scale: Option, - /// The drag ended, so the scale above is worth writing to the config. - pub save_scale: bool, /// A shortcut captured on the shortcut page, to register and save. pub set_hotkey: Option, } @@ -473,6 +473,8 @@ pub struct Tutorial { /// The shortcut button is armed and the next key combination is the /// answer — the Settings tab's own capture, and its own flag. capturing_hotkey: bool, + /// The welcome page's scale slider, between moving and Apply. + staged_scale: Option, } impl Tutorial { @@ -483,6 +485,7 @@ impl Tutorial { typing: None, moved: false, capturing_hotkey: false, + staged_scale: None, } } @@ -526,6 +529,7 @@ impl Tutorial { // Lifted out of `self` for the window's closure, and put back after: // the closure already holds the page and the actions. let mut capturing = self.capturing_hotkey; + let mut staged_scale = self.staged_scale; let mut dismissed = false; let mut window = egui::Window::new(page.title) .id(egui::Id::new(WINDOW_ID)) @@ -555,6 +559,7 @@ impl Tutorial { if let Some(extra) = page.extra { let mut live = Live { zoom, + staged_scale: &mut staged_scale, hotkey, capturing_hotkey: &mut capturing, }; @@ -604,6 +609,7 @@ impl Tutorial { self.moved = true; } self.capturing_hotkey = capturing; + self.staged_scale = staged_scale; // The widgets this page names, in the colours its keywords were given. let dark_mode = ctx.style().visuals.dark_mode; diff --git a/crates/quicksearch-gui/src/tutorial/tests.rs b/crates/quicksearch-gui/src/tutorial/tests.rs index f88885c..70e53c8 100644 --- a/crates/quicksearch-gui/src/tutorial/tests.rs +++ b/crates/quicksearch-gui/src/tutorial/tests.rs @@ -22,6 +22,7 @@ fn at(page: usize) -> Tutorial { typing: None, moved: false, capturing_hotkey: false, + staged_scale: None, } } @@ -34,6 +35,7 @@ fn entering(page: usize) -> Tutorial { typing: None, moved: false, capturing_hotkey: false, + staged_scale: None, } } @@ -59,7 +61,6 @@ fn merge(a: TourActions, b: TourActions) -> TourActions { set_query: b.set_query.or(a.set_query), focus_search: a.focus_search || b.focus_search, set_scale: b.set_scale.or(a.set_scale), - save_scale: a.save_scale || b.save_scale, set_hotkey: b.set_hotkey.or(a.set_hotkey), } } @@ -302,9 +303,11 @@ fn drag( } /// The whole point of putting it on the first page: someone who cannot read -/// the window can fix that without finding the Settings tab first. +/// the window can fix that without finding the Settings tab first. Staged +/// until Apply: applying mid-drag would rescale the slider under the +/// pointer, and the handle would chase its own tail. #[test] -fn the_welcome_page_slider_sets_the_ui_scale() { +fn the_welcome_page_slider_applies_only_on_its_button() { let ctx = crate::test_ui::ctx(); let scale_page = page_with(Extra::Scale); let mut tour = at(scale_page); @@ -316,28 +319,36 @@ fn the_welcome_page_slider_sets_the_ui_scale() { .1; // The rail runs to the right of its label, on the same row. let from = egui::pos2(label.right() + 30.0, label.center().y); - let [_, moved, release] = drag(&ctx, &mut tour, from, from + egui::vec2(120.0, 0.0)); - - let dragged = moved - .set_scale - .expect("dragging the slider changed nothing"); + let [press, moved, release] = drag(&ctx, &mut tour, from, from + egui::vec2(120.0, 0.0)); + for (what, actions) in [("press", &press), ("move", &moved), ("release", &release)] { + assert_eq!( + actions.set_scale, None, + "the {what} applied the scale without Apply being clicked" + ); + } + let staged = tour.staged_scale.expect("the drag staged nothing"); assert!( - crate::app::SCALE_RANGE.contains(&dragged), - "{dragged} is outside the range the slider offers" + crate::app::SCALE_RANGE.contains(&staged), + "{staged} is outside the range the slider offers" ); - assert_ne!(dragged, 1.0, "the drag did not move the value"); - assert!(!moved.save_scale, "the config was written mid-drag"); - assert_eq!( - release.set_scale, - Some(dragged), - "the release did not hand the settled value over to be saved" - ); - assert!(release.save_scale, "the drag ended without being saved"); + assert_ne!(staged, 1.0, "the drag did not move the value"); - // And nothing happens on a frame nobody touched it. - let (_, quiet) = pass(&ctx, &mut tour, Vec::new(), 2.0); + // The staged value survives idle frames, then Apply hands it over once. + let (out, quiet) = pass(&ctx, &mut tour, Vec::new(), 2.0); assert_eq!(quiet.set_scale, None); - assert!(!quiet.save_scale); + assert_eq!(tour.staged_scale, Some(staged)); + let apply = painted(&out) + .into_iter() + .find(|(text, _)| text == "Apply") + .expect("no Apply button beside the slider") + .1; + let (_, applied) = pass(&ctx, &mut tour, click_at(apply.center()), 2.1); + assert_eq!( + applied.set_scale, + Some(staged), + "Apply did not hand the staged value over" + ); + assert_eq!(tour.staged_scale, None, "Apply left the value staged"); } /// The slider shows the size the window is already at — the config's, or diff --git a/crates/quicksearch-gui/src/unlock.rs b/crates/quicksearch-gui/src/unlock.rs index 1cce567..097d640 100644 --- a/crates/quicksearch-gui/src/unlock.rs +++ b/crates/quicksearch-gui/src/unlock.rs @@ -56,14 +56,18 @@ impl Gate { /// it or a `--toggle` process relayed the desktop's. Handled here because /// while locked the unlock screen *is* the window. fn handle_activation(&mut self, ctx: &egui::Context, frame: &eframe::Frame) { + if let Gate::Running(app) = self { + // The shortcut must not reshuffle the window under a key capture. + // The flag is left set, not consumed: capture ends on a key + // event, which repaints, and the press is acted on that frame. + if app.capturing_hotkey() { + return; + } + } if !crate::activate::take_pending() { return; } if let Gate::Running(app) = self { - // The shortcut must not reshuffle the window under a key capture. - if app.capturing_hotkey() { - return; - } app.activate_search(ctx); } crate::activate::raise(ctx, frame); diff --git a/packaging/quicksearch.nsi b/packaging/quicksearch.nsi index f11a5f9..4eddb59 100644 --- a/packaging/quicksearch.nsi +++ b/packaging/quicksearch.nsi @@ -174,21 +174,23 @@ Section "Start Menu shortcut" SecStartMenu CreateShortcut "$SMPROGRAMS\${APP}.lnk" "$INSTDIR\quicksearch.exe" "--toggle" "$INSTDIR\quicksearch.ico" SectionEnd -Section "Search hotkey (Ctrl+Alt+F)" SecHotkey +Section "Search hotkey (Ctrl+Shift+F)" SecHotkey ; The .lnk "shortcut key" field is the only thing on Windows that binds a ; key to a command, and it is what makes the shortcut work while ; QuickSearch is closed - nothing an application registers for itself can ; fire when it is not running. Windows only honours the field on a - ; shortcut in the Start menu or on the desktop, and only for combinations - ; including Ctrl+Alt, which is why this is Ctrl+Alt+F and not the - ; Ctrl+Shift+F the Settings tab offers. The in-application shortcut takes - ; any combination but only answers while the window is already open, so - ; the two are complementary rather than duplicates. + ; shortcut in the Start menu or on the desktop. Modifier combinations + ; like Ctrl+Shift are accepted as-is; only a bare key gets Ctrl+Alt added + ; for it. Ctrl+Shift+F matches the in-application default, which answers + ; the key instantly while the window is open; this .lnk binding is the + ; slower launch path Explorer takes when it is not. Changing the shortcut + ; on the Settings tab rewrites this .lnk to match (per-user installs + ; only; this all-users file needs elevation). ; ; Rewrites the same shortcut the section above creates: NSIS cannot add a ; hotkey to an existing .lnk, and creating it twice is harmless. CreateShortcut "$SMPROGRAMS\${APP}.lnk" "$INSTDIR\quicksearch.exe" "--toggle" \ - "$INSTDIR\quicksearch.ico" 0 SW_SHOWNORMAL ALT|CONTROL|F \ + "$INSTDIR\quicksearch.ico" 0 SW_SHOWNORMAL CONTROL|SHIFT|F \ "Search your files with ${APP}" SectionEnd @@ -212,9 +214,8 @@ SectionEnd !insertmacro MUI_DESCRIPTION_TEXT ${SecStartMenu} \ "Add ${APP} to the Start menu for all users." !insertmacro MUI_DESCRIPTION_TEXT ${SecHotkey} \ - "Press Ctrl+Alt+F anywhere to search, starting ${APP} if it is not \ - already running. Windows allows this only on Ctrl+Alt combinations; \ - the Settings tab has one that takes any keys while ${APP} is open." + "Press Ctrl+Shift+F anywhere to search, starting ${APP} if it is not \ + already running. The Settings tab can rebind it." !insertmacro MUI_DESCRIPTION_TEXT ${SecDesktop} \ "Add a ${APP} shortcut to the desktop." !insertmacro MUI_FUNCTION_DESCRIPTION_END