diff --git a/core/dr-catalog/src/collections.rs b/core/dr-catalog/src/collections.rs index 30d642a..0f4a5e0 100644 --- a/core/dr-catalog/src/collections.rs +++ b/core/dr-catalog/src/collections.rs @@ -682,7 +682,7 @@ fn require_exists(conn: &Connection, id: CollectionId) -> Result<(), CatalogErro /// same reasoning as the connector's date parsing. Version 4 layout, seeded /// from the OS via `getrandom` through `rusqlite`'s existing dependency-free /// path — see below. -fn new_uuid() -> String { +pub(crate) fn new_uuid() -> String { let b = random_bytes(); // Version 4, variant 1, per RFC 4122 §4.4. let v6 = (b[6] & 0x0F) | 0x40; diff --git a/core/dr-catalog/src/error.rs b/core/dr-catalog/src/error.rs index b6d6f08..0a8f0da 100644 --- a/core/dr-catalog/src/error.rs +++ b/core/dr-catalog/src/error.rs @@ -42,6 +42,15 @@ pub enum CatalogError { #[error("no such collection: {0}")] NoSuchCollection(u64), + /// A keyword the caller named is gone — deleted, or fused into another by a + /// merge while its id sat in a UI model. + /// + /// Its own variant rather than a silent no-op because the two are different + /// answers to the user: a rename that quietly did nothing looks exactly like + /// a rename that did not take. + #[error("no such keyword: {0}")] + NoSuchKeyword(u64), + /// Images were dropped onto a smart collection. /// /// A smart collection's membership *is* its selector, so member rows would diff --git a/core/dr-catalog/src/keywords.rs b/core/dr-catalog/src/keywords.rs new file mode 100644 index 0000000..c3676ba --- /dev/null +++ b/core/dr-catalog/src/keywords.rs @@ -0,0 +1,1220 @@ +//! TRACES: FR-CAT-5 | FR-CAT-6 | FR-CAT-13 | NFR-R5 +//! Keywords: the vocabulary, the assignments, and the edits the UI performs. +//! +//! The read half of this shipped with the catalog and the write half did not. +//! [`crate::query`] has compiled [`dr_types::Selector::Keyword`] against the +//! `keywords` table since v1, and [`dr_types::Selector::Text`] substring-matches +//! it, so the library could always be searched by keyword — but nothing could +//! ever *put* one there. A user could filter to a word they had no way to +//! apply. This module is the missing half. +//! +//! # Two tables, and which one is the fact +//! +//! **`keywords`** is the assignment: `(version_id, keyword)`, where the keyword +//! is the *word itself* as text. This is the durable fact. It is what +//! [`crate::query`] matches, what a sidecar carries, and what XMP `dc:subject` +//! interoperates on (FR-CAT-13) — every one of which speaks in strings. +//! +//! **`keyword_terms`** is the identity: uuid, revision, tombstone. It exists so +//! that a *rename* and a *deletion* have something a cross-device merge can key +//! on, and so that a keyword can exist in the vocabulary before any photograph +//! carries it. It is not referenced by the assignment rows; see the V6 +//! migration in [`crate::schema`] for why pointing at it would be a mistake. +//! +//! A rename therefore writes both: the term rows (so the edit merges) and every +//! assignment carrying the old text (so the search, the sidecar and the XMP all +//! agree). Those writes are one transaction, because a catalog holding half a +//! rename would show the keyword under one name and find it under the other. +//! [`rename`] explains why the old identity is retired rather than relabelled. +//! +//! # Which version an assignment lands on +//! +//! The schema hangs keywords off `versions`, not `images`, and that is right: +//! FR-NC-8 makes the sidecar a keyed set of versions, so everything durable +//! about a photograph is stored per version. But a *write* from the UI has to +//! land somewhere definite, and it lands on the **default** version — the same +//! choice [`crate::rating`] makes, for the same reason. +//! +//! Reading is deliberately asymmetric: [`crate::query`] matches a keyword on +//! *any* version of an image (`EXISTS ... WHERE kv.image_id = images.id`), and +//! [`for_images`] does the same. A keyword names what is in the frame, so a +//! word applied to a virtual copy must still find the photograph — but there +//! must be exactly one place a UI write goes, or two copies of one frame come +//! to disagree about their own subject. +//! +//! # Uniqueness is converged upon, not constrained +//! +//! `keyword_terms.name` carries no unique index. Two devices that each type +//! "Iceland" both create a term, with different uuids, and neither is wrong +//! until they meet. A constraint would abort the merge transaction at that +//! moment — which is the ordinary case, not a corner one. +//! +//! Instead: [`create`] resolves an existing name rather than adding a second +//! row, and [`fuse_duplicates`] collapses a cross-device pair onto the +//! lexicographically smaller uuid. That rule is deterministic and symmetric, so +//! both devices reach the same answer without a round trip. +//! +//! # Keywords are user text, and they only ever bind +//! +//! Every keyword here reaches SQLite as a bound parameter. `crate::query` has +//! the same rule and a test that asserts it; this module is the write side of +//! the same guarantee, and the reason it matters more here is that this is +//! where the text arrives from the user in the first place. + +use rusqlite::{Connection, OptionalExtension}; + +use dr_types::ImageId; + +use crate::error::CatalogError; + +/// A keyword's local row id in `keyword_terms`. +/// +/// Local to this catalog and meaningless on another device, exactly as +/// [`dr_types::CollectionId`] is — [`Keyword::uuid`] is what a merge keys on. +/// +/// Defined here rather than in `dr-types` because nothing outside the catalog +/// and the panel that draws it has any use for it: an id that names a row in a +/// rebuildable index is not a term of the shared vocabulary the way an +/// `ImageId` is. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct KeywordId(pub u64); + +/// One keyword, as the vocabulary list draws it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Keyword { + pub id: KeywordId, + /// Device-independent identity. The integer `id` is local and collides + /// across devices; this is what a merge keys on. + pub uuid: String, + pub name: String, + /// How many images in the whole library carry it. Shown beside the word so + /// the user can tell a keyword they use from one they typed once by + /// mistake. + pub image_count: usize, +} + +/// A keyword's standing across a *selection*, for the panel. +/// +/// The three-way distinction is the whole reason this type exists: applying a +/// keyword to forty photographs where thirty already carry it must not look +/// the same as applying it to forty that do not, and removing one that only +/// some of them carry must not silently claim to have removed it from all. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Coverage { + /// No image in the selection carries it. + None, + /// Some do and some do not — Lightroom's dash rather than a tick. + Some, + /// Every image in the selection carries it. + All, +} + +/// A keyword plus how much of the current selection it covers. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SelectionKeyword { + pub keyword: Keyword, + pub coverage: Coverage, + /// How many of the *selected* images carry it. + pub selected_count: usize, +} + +/// Longest keyword accepted. +/// +/// Not a storage limit — SQLite would take a megabyte — but a paste guard. The +/// field that feeds this is one line in a sheet, and a keyword the width of a +/// paragraph is a mis-paste rather than an intention. Truncating rather than +/// refusing, so the paste is recoverable by editing rather than lost. +pub const MAX_KEYWORD_LEN: usize = 128; + +/// Create a keyword, or return the one that already carries this name. +/// +/// **Resolving rather than refusing** is what separates this from +/// [`crate::collections::create`], which happily makes a second "Iceland". A +/// keyword *is* its text — that is what the assignment rows store and what XMP +/// carries — so two term rows with one name are two identities for one thing, +/// and the second would be a duplicate in the vocabulary list that no amount +/// of user care could distinguish from the first. +/// +/// The UUID is generated here rather than taken from the caller: it is the +/// merge identity, and a caller that reuses one fuses two keywords at the next +/// sync. +pub fn create(conn: &Connection, name: &str) -> Result { + let name = normalise(name)?; + + // A tombstoned row is deliberately *not* resurrected here: it carries a + // revision that says "deleted", and reusing it would make the new keyword + // inherit an argument it was never part of. A fresh uuid starts the + // conversation again, which is what the user asked for by typing the word. + if let Some(id) = live_id_for_name(conn, &name)? { + return Ok(id); + } + + let now = now_secs(); + conn.execute( + "INSERT INTO keyword_terms(uuid, name, created, revision, modified) + VALUES (?1, ?2, ?3, 1, ?3)", + rusqlite::params![new_uuid(), name, now], + )?; + Ok(KeywordId(conn.last_insert_rowid() as u64)) +} + +/// Rename a keyword, everywhere it appears. +/// +/// Returns the identity the word now has, which is **not** the one passed in. +/// +/// # A rename retires one word and raises another +/// +/// The obvious implementation — change `name` on the term row and keep its +/// uuid — is wrong here, and the reason is that assignments store *text*. Take +/// two keywords, "Icland" and "Iceland", and rename the first onto the second. +/// Something has to give, because two live rows may not both be "Iceland", and +/// every way of choosing between them by uuid or by revision loses the rename +/// on the device that chose the other way. The rename then silently fails to +/// propagate, which is the worst of the outcomes available. +/// +/// So a rename is modelled as what it actually is to a body of *text*: the old +/// word stops existing and the new one exists. The old term row is tombstoned +/// **keeping its old name**, the new word gets (or already has) a live row, and +/// every assignment is rewritten between them. That composes with the merge +/// rules already in place — the tombstone travels as a deletion and takes the +/// old word's assignments with it on every device, and the new word travels as +/// an ordinary keyword — rather than needing a rule of its own. +/// +/// One transaction over both tables. A catalog holding half a rename would show +/// a photograph under the new word and fail to find it under either. +/// +/// Renaming onto a name another keyword already holds therefore **fuses the +/// two**, which is almost always what was meant: the user is correcting +/// "Icland" to "Iceland" and there is already an "Iceland". Refusing would +/// leave them to do it by hand, image by image, with no bulk gesture to do it +/// with. +pub fn rename(conn: &Connection, id: KeywordId, name: &str) -> Result { + let name = normalise(name)?; + let old = name_of(conn, id)?; + if old == name { + // Not an error, and not an edit either: bumping the revision for a + // no-op rename would let an idle device win a merge against one that + // did real work. + return Ok(id); + } + + let tx = conn.unchecked_transaction()?; + + // Assignments first. If this fails, the term rows are untouched and the + // catalog is merely unchanged rather than internally inconsistent. + rewrite_assignments(&tx, &old, &name)?; + retire(&tx, id)?; + let now = create(&tx, &name)?; + + tx.commit()?; + Ok(now) +} + +/// Delete a keyword, leaving a tombstone. +/// +/// The term row survives with `deleted = 1` because a merge against a device +/// that still holds the keyword would otherwise resurrect it — the same rule +/// [`crate::collections::delete`] follows. +/// +/// The assignment rows go outright. They carry no independent identity: the +/// tombstone is what merges, and a photograph keyworded with a word that no +/// longer exists would be searchable by a term absent from every list. +/// +/// Returns how many assignments went. +pub fn delete(conn: &Connection, id: KeywordId) -> Result { + let name = name_of(conn, id)?; + + let tx = conn.unchecked_transaction()?; + let removed = tx.execute("DELETE FROM keywords WHERE keyword = ?1", [&name])?; + retire(&tx, id)?; + tx.commit()?; + Ok(removed) +} + +/// The whole vocabulary, most-used first and then alphabetical. +/// +/// Used-first because the list is a target for a thumb: the words a +/// photographer reaches for are the ones they already use, and burying them +/// under a one-off typo sorted to the top is what makes a keyword list stop +/// being used. Alphabetical *within* a count so the order is stable across +/// launches and across devices — row-id order would differ per device, which +/// is disorienting on the same library seen from two machines. +/// +/// One grouped aggregate rather than a count per row: the sheet redraws on +/// every assignment, and a query per keyword would be one statement per word +/// in the library. +pub fn list(conn: &Connection) -> Result, CatalogError> { + let mut stmt = conn.prepare( + // DISTINCT image, not row: a word on two versions of one frame is one + // photograph, and reporting two is the kind of small lie that makes a + // user stop trusting the counts. + "SELECT t.id, t.uuid, t.name, + (SELECT count(DISTINCT v.image_id) + FROM keywords k JOIN versions v ON v.id = k.version_id + WHERE k.keyword = t.name) + FROM keyword_terms t + WHERE t.deleted = 0 + ORDER BY 4 DESC, t.name COLLATE NOCASE, t.id", + )?; + let rows = stmt + .query_map([], |r| { + Ok(Keyword { + id: KeywordId(r.get::<_, i64>(0)? as u64), + uuid: r.get(1)?, + name: r.get(2)?, + image_count: r.get::<_, i64>(3)? as usize, + }) + })? + .collect::, _>>()?; + Ok(rows) +} + +/// Assign a keyword to images, creating the keyword if it is new. +/// +/// The bulk form is the *only* form, because keywording a selection is the +/// common case rather than the exception: a photographer picks out the frames +/// with the puffin in them and applies "puffin" once. One transaction, so a +/// crash partway through cannot leave half a selection keyworded — the same +/// reasoning as [`crate::rating::set_rating_many`]. +/// +/// Additive and idempotent. An image that already carries the word is left +/// alone rather than rewritten, because re-applying a keyword to a selection +/// that overlaps what is already tagged is a normal thing to do. +/// +/// Returns how many images genuinely gained it, which is what the UI reports — +/// "added to 3 of 12" is the honest message when nine already had it. +pub fn assign(conn: &Connection, images: &[ImageId], name: &str) -> Result { + let name = normalise(name)?; + let tx = conn.unchecked_transaction()?; + + // The term row first, so the word appears in the vocabulary even when the + // selection turns out to be empty — typing a keyword into the field and + // pressing return is a legitimate way to build a vocabulary ahead of the + // photographs it will be used on. + create(&tx, &name)?; + + let mut added = 0; + { + let mut stmt = tx.prepare( + "INSERT INTO keywords(version_id, keyword) VALUES (?1, ?2) + ON CONFLICT(version_id, keyword) DO NOTHING", + )?; + for image in images { + // Creates the version if the image has none — a library scanned + // before versions existed, or a scan interrupted between the image + // insert and the commit. Failing the keyword because of either + // would be the wrong answer: the user typed a word and expects it + // to stick. + let version = crate::rating::default_version_id(&tx, *image)?; + added += stmt.execute(rusqlite::params![version, name])?; + } + } + + tx.commit()?; + Ok(added) +} + +/// Take a keyword off images. +/// +/// Removes the assignment only. The keyword itself survives in the vocabulary +/// and on every other photograph that carries it — that asymmetry is the point +/// of a join table, and it is why this is "remove from these images" and never +/// "delete the keyword". [`delete`] is the other gesture, and it says so. +/// +/// Removes it from **every** version of each image, not only the default. A +/// user who unticks a word is saying the photograph is not of that thing, and +/// leaving it on a virtual copy would keep the frame in the search results +/// with nothing in the panel to explain why. +pub fn unassign(conn: &Connection, images: &[ImageId], name: &str) -> Result { + let name = normalise(name)?; + if images.is_empty() { + return Ok(0); + } + + let tx = conn.unchecked_transaction()?; + let mut removed = 0; + { + let mut stmt = tx.prepare( + "DELETE FROM keywords + WHERE keyword = ?2 + AND version_id IN (SELECT id FROM versions WHERE image_id = ?1)", + )?; + for image in images { + if stmt.execute(rusqlite::params![image.0 as i64, name])? > 0 { + removed += 1; + } + } + } + tx.commit()?; + Ok(removed) +} + +/// Every keyword on one image, alphabetically. +/// +/// Across all its versions, and de-duplicated: the panel shows what the +/// photograph is of, and a word on two virtual copies is still one subject. +pub fn for_image(conn: &Connection, image: ImageId) -> Result, CatalogError> { + let mut stmt = conn.prepare( + "SELECT DISTINCT k.keyword + FROM keywords k JOIN versions v ON v.id = k.version_id + WHERE v.image_id = ?1 + ORDER BY k.keyword COLLATE NOCASE", + )?; + let rows = stmt + .query_map([image.0 as i64], |r| r.get(0))? + .collect::, _>>()?; + Ok(rows) +} + +/// The vocabulary, annotated with how much of `images` each keyword covers. +/// +/// This is what the keywording panel draws: one list, in which a word the whole +/// selection already carries, a word only some of it carries, and a word none +/// of it carries are three visibly different things. +/// +/// Two statements regardless of the selection size — the vocabulary, and one +/// grouped count over the selection. A count per keyword would be one query +/// per word on every redraw, which is the cost `crate::rating::judgements` +/// exists to avoid for stars. +/// +/// A word that is on an image but has somehow lost its term row still appears, +/// synthesised with no identity. That happens to a catalog rebuilt from +/// sidecars between the rebuild and the next [`adopt_orphan_terms`], and a +/// panel that omitted the keywords the photograph visibly has would read as the +/// keywords having been lost. +pub fn for_images( + conn: &Connection, + images: &[ImageId], +) -> Result, CatalogError> { + let vocabulary = list(conn)?; + if images.is_empty() { + return Ok(vocabulary + .into_iter() + .map(|keyword| SelectionKeyword { + keyword, + coverage: Coverage::None, + selected_count: 0, + }) + .collect()); + } + + // Placeholders are generated from the *count* of ids, never from any text + // that came from outside — the same rule `rating::judgements` follows, and + // the reason a keyword can never reach SQL as anything but a parameter. + let placeholders = std::iter::repeat_n("?", images.len()) + .collect::>() + .join(","); + let sql = format!( + "SELECT k.keyword, count(DISTINCT v.image_id) + FROM keywords k JOIN versions v ON v.id = k.version_id + WHERE v.image_id IN ({placeholders}) + GROUP BY k.keyword" + ); + let params: Vec = images + .iter() + .map(|i| rusqlite::types::Value::Integer(i.0 as i64)) + .collect(); + + let mut counts: std::collections::HashMap = std::collections::HashMap::new(); + { + let mut stmt = conn.prepare(&sql)?; + let rows = stmt.query_map(rusqlite::params_from_iter(params.iter()), |r| { + Ok((r.get::<_, String>(0)?, r.get::<_, i64>(1)?)) + })?; + for (word, n) in rows.flatten() { + counts.insert(word, n as usize); + } + } + + let mut out: Vec = Vec::with_capacity(vocabulary.len()); + let mut seen = std::collections::HashSet::new(); + for keyword in vocabulary { + let n = counts.get(&keyword.name).copied().unwrap_or(0); + seen.insert(keyword.name.clone()); + out.push(SelectionKeyword { + coverage: coverage_of(n, images.len()), + selected_count: n, + keyword, + }); + } + + // Words the selection carries that the vocabulary has no row for. Sorted + // and appended rather than interleaved, so the list the user has been + // looking at does not reshuffle around them. + let mut orphans: Vec<(String, usize)> = counts + .into_iter() + .filter(|(word, _)| !seen.contains(word)) + .collect(); + orphans.sort_by(|a, b| a.0.to_lowercase().cmp(&b.0.to_lowercase())); + for (name, n) in orphans { + out.push(SelectionKeyword { + keyword: Keyword { + // Zero, because there is no row. The panel treats it as a word + // it can assign and unassign but not rename — which is exactly + // true until the next backfill gives it an identity. + id: KeywordId(0), + uuid: String::new(), + image_count: n, + name, + }, + coverage: coverage_of(n, images.len()), + selected_count: n, + }); + } + + Ok(out) +} + +fn coverage_of(carrying: usize, selected: usize) -> Coverage { + if carrying == 0 { + Coverage::None + } else if carrying >= selected { + Coverage::All + } else { + Coverage::Some + } +} + +/// Give a term row to every word some image carries without one. +/// +/// Runs from [`crate::schema::backfill`] on every open. Cheap on the common +/// path: one anti-joined scan of an indexed column that inserts nothing once +/// the vocabulary is complete. +/// +/// Returns how many terms were adopted, so an import or a rebuild can log it +/// rather than silently writing thousands of rows. +pub fn adopt_orphan_terms(conn: &Connection) -> Result { + let orphans: Vec = { + let mut stmt = conn.prepare( + // A tombstone counts as "has a term row". A word the user deleted + // whose assignment somehow outlived the deletion must not be + // quietly readmitted to the vocabulary; it stays visible as an + // orphan in [`for_images`] instead, which is a state someone can + // see and act on rather than one that silently undoes a deletion. + "SELECT DISTINCT k.keyword FROM keywords k + WHERE NOT EXISTS (SELECT 1 FROM keyword_terms t + WHERE t.name = k.keyword)", + )?; + let found = stmt + .query_map([], |r| r.get(0))? + .collect::, _>>()?; + found + }; + if orphans.is_empty() { + return Ok(0); + } + + // One transaction for the batch. An import from Lightroom can bring in + // hundreds of keywords, and per-statement commits would be hundreds of + // fsyncs for what is conceptually one adoption. + let tx = conn.unchecked_transaction()?; + let now = now_secs(); + { + let mut stmt = tx.prepare( + "INSERT INTO keyword_terms(uuid, name, created, revision, modified) + VALUES (?1, ?2, ?3, 1, ?3)", + )?; + for name in &orphans { + stmt.execute(rusqlite::params![new_uuid(), name, now])?; + } + } + tx.commit()?; + Ok(orphans.len()) +} + +/// Collapse term rows that describe the same word onto one identity. +/// +/// The keyword *is* its text, so two live rows named "Iceland" are two +/// identities for one thing. That happens for one ordinary reason: two devices +/// each typed the word before they had ever synced, and each minted a uuid. +/// +/// The survivor is the **lexicographically smallest uuid**, and the losers are +/// deleted outright rather than tombstoned. Both halves of that matter: +/// +/// - *Smallest uuid* is a rule both devices apply to the same pair and reach +/// the same answer from, with no round trip and no ordering dependency. Any +/// rule that consulted a revision or a timestamp would let two devices pick +/// different survivors and never converge. +/// - *Deleted, not tombstoned*, because the word itself has not been deleted — +/// the redundant row is being retired, and a tombstone would propagate as +/// "the user removed this keyword" and take the assignments with it. +/// +/// Assignment rows need no repair: they store the text, which is the same on +/// both sides, which is precisely why fusing is possible at all. +/// +/// Returns how many rows were retired. +pub fn fuse_duplicates(conn: &Connection) -> Result { + let n = conn.execute( + "DELETE FROM keyword_terms + WHERE deleted = 0 + AND uuid > (SELECT min(o.uuid) FROM keyword_terms o + WHERE o.deleted = 0 AND o.name = keyword_terms.name)", + [], + )?; + Ok(n) +} + +/// Point every assignment at a new spelling of the same word. +/// +/// `INSERT OR IGNORE` then `DELETE` rather than a bare `UPDATE`, because a +/// version may already carry the destination word — renaming "Icland" to +/// "Iceland" on a frame that has both — and the primary key would reject the +/// update for that row and abort the rename for every other. +fn rewrite_assignments(conn: &Connection, from: &str, to: &str) -> Result<(), CatalogError> { + conn.execute( + "INSERT OR IGNORE INTO keywords(version_id, keyword) + SELECT version_id, ?2 FROM keywords WHERE keyword = ?1", + rusqlite::params![from, to], + )?; + conn.execute("DELETE FROM keywords WHERE keyword = ?1", [from])?; + Ok(()) +} + +/// The stored form of a keyword the user typed. +/// +/// Trimmed, inner whitespace collapsed, and length-capped. Trimming matters +/// more than it looks: `keywords.keyword` is compared with `=` by +/// [`crate::query`], so " puffin" and "puffin" would be two keywords that look +/// identical in every list and never match each other's searches. +/// +/// Case is deliberately **preserved**. "Iceland" is a place and "iceland" is a +/// typo of it, and a photographer who capitalises their proper nouns should +/// find them capitalised. The lists sort `COLLATE NOCASE` so the two still land +/// beside each other where both exist. +/// +/// Public because a caller that is about to *tell the user* what it did needs +/// the word as it will be stored, not as it was typed. Reporting `Added " +/// puffin "` for something the list then shows as `puffin` is a small +/// inconsistency, but it is the kind that makes a user wonder whether the +/// leading space mattered — and the only way to answer that from the outside +/// is to duplicate this rule, which is how the two come to disagree. +pub fn normalise(name: &str) -> Result { + let collapsed = name.split_whitespace().collect::>().join(" "); + if collapsed.is_empty() { + return Err(CatalogError::BadName( + "A keyword needs a word in it.".into(), + )); + } + // Truncated on a *character* boundary: `String::truncate` panics mid-code + // point, and a keyword is as likely to be "Þingvellir" as "puffin". + Ok(collapsed.chars().take(MAX_KEYWORD_LEN).collect()) +} + +/// The live term row holding this exact name, if there is one. +fn live_id_for_name(conn: &Connection, name: &str) -> Result, CatalogError> { + let id: Option = conn + .query_row( + "SELECT id FROM keyword_terms WHERE name = ?1 AND deleted = 0 ORDER BY uuid LIMIT 1", + [name], + |r| r.get(0), + ) + .optional()?; + Ok(id.map(|v| KeywordId(v as u64))) +} + +fn name_of(conn: &Connection, id: KeywordId) -> Result { + conn.query_row( + "SELECT name FROM keyword_terms WHERE id = ?1 AND deleted = 0", + [id.0 as i64], + |r| r.get(0), + ) + .optional()? + .ok_or(CatalogError::NoSuchKeyword(id.0)) +} + +/// Tombstone a term row, bumping its revision. +/// +/// **The name is deliberately left as it was.** A tombstone is read by the +/// merge as "the user removed *this word*", and the other device acts on it by +/// deleting the assignments carrying that text — so a tombstone renamed to the +/// word it was merged into would delete the assignments of the surviving +/// keyword instead of the retired one. See [`rename`]. +/// +/// The revision bump is not optional. [`crate::merge`] compares revisions, so a +/// deletion that updated `modified` alone is invisible to it — the other device +/// keeps its live row and the keyword comes back at the next sync. +fn retire(conn: &Connection, id: KeywordId) -> Result<(), CatalogError> { + conn.execute( + "UPDATE keyword_terms + SET deleted = 1, revision = revision + 1, modified = ?2 + WHERE id = ?1", + rusqlite::params![id.0 as i64, now_secs()], + )?; + Ok(()) +} + +/// A random UUID, formatted as the canonical 8-4-4-4-12. +/// +/// Shares [`crate::collections`]'s implementation rather than repeating it: a +/// second generator is a second thing to get wrong, and the merge identity is +/// the one place a weak one is unrecoverable. +fn new_uuid() -> String { + crate::collections::new_uuid() +} + +fn now_secs() -> i64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs() as i64) + .unwrap_or(0) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::Catalog; + use dr_types::Selector; + + /// A catalog with six images, each with a default version. + fn seeded() -> Catalog { + let cat = Catalog::in_memory().unwrap(); + let c = cat.connection(); + c.execute( + "INSERT INTO roots(id, kind, label) VALUES (1, 'local', 'lib')", + [], + ) + .unwrap(); + for i in 1..=6i64 { + c.execute( + "INSERT INTO images(id, root_id, source_ref, added_at) + VALUES (?1, 1, ?2, 0)", + rusqlite::params![i, format!("IMG_{i}.CR3")], + ) + .unwrap(); + } + crate::rating::ensure_default_versions(c).unwrap(); + cat + } + + fn img(n: u64) -> ImageId { + ImageId(n) + } + + fn names(cat: &Catalog) -> Vec { + list(cat.connection()) + .unwrap() + .into_iter() + .map(|k| k.name) + .collect() + } + + #[test] + fn a_keyword_can_be_applied_and_then_found() { + // The whole point: the search path existed and nothing could feed it. + let cat = seeded(); + assign(cat.connection(), &[img(1), img(2)], "puffin").unwrap(); + + let q = crate::Query { + filter: Selector::Keyword("puffin".into()), + ..Default::default() + }; + assert_eq!(cat.count(&q, 0).unwrap(), 2); + } + + #[test] + fn assigning_to_a_selection_is_one_gesture() { + let cat = seeded(); + let all: Vec = (1..=6).map(img).collect(); + assert_eq!(assign(cat.connection(), &all, "iceland").unwrap(), 6); + } + + #[test] + fn reapplying_to_an_overlapping_selection_reports_only_the_new_ones() { + // "Added to 1 of 3" is the honest message, and the UI shows it. + let cat = seeded(); + assign(cat.connection(), &[img(1), img(2)], "puffin").unwrap(); + let added = assign(cat.connection(), &[img(1), img(2), img(3)], "puffin").unwrap(); + assert_eq!(added, 1); + } + + #[test] + fn assignment_is_idempotent_and_stores_one_row() { + let cat = seeded(); + assign(cat.connection(), &[img(1)], "puffin").unwrap(); + assign(cat.connection(), &[img(1)], "puffin").unwrap(); + assert_eq!(for_image(cat.connection(), img(1)).unwrap(), ["puffin"]); + } + + #[test] + fn unassigning_leaves_the_keyword_on_everything_else() { + // The join-table asymmetry: removing a word from one photograph is not + // deleting the word. + let cat = seeded(); + assign(cat.connection(), &[img(1), img(2)], "puffin").unwrap(); + assert_eq!(unassign(cat.connection(), &[img(1)], "puffin").unwrap(), 1); + + assert!(for_image(cat.connection(), img(1)).unwrap().is_empty()); + assert_eq!(for_image(cat.connection(), img(2)).unwrap(), ["puffin"]); + assert_eq!(names(&cat), ["puffin"], "the vocabulary still has it"); + } + + #[test] + fn a_word_on_a_virtual_copy_is_removed_with_the_frame() { + // Unticking says "this photograph is not of that", and a word left on + // a virtual copy would keep the frame in the results with nothing in + // the panel to explain it. + let cat = seeded(); + let c = cat.connection(); + c.execute( + "INSERT INTO versions(image_id, uuid, name, is_default) VALUES (1, 'copy', 'Crop', 0)", + [], + ) + .unwrap(); + let copy: i64 = c.last_insert_rowid(); + assign(c, &[img(1)], "puffin").unwrap(); + c.execute( + "INSERT INTO keywords(version_id, keyword) VALUES (?1, 'puffin')", + [copy], + ) + .unwrap(); + + unassign(c, &[img(1)], "puffin").unwrap(); + let n: i64 = c + .query_row("SELECT count(*) FROM keywords", [], |r| r.get(0)) + .unwrap(); + assert_eq!(n, 0); + } + + #[test] + fn a_keyword_on_a_virtual_copy_still_finds_the_photograph() { + // The read side is deliberately asymmetric with the write side: a word + // names what is in the frame, whichever copy carries it. + let cat = seeded(); + let c = cat.connection(); + c.execute( + "INSERT INTO versions(image_id, uuid, name, is_default) VALUES (3, 'copy', 'Crop', 0)", + [], + ) + .unwrap(); + c.execute( + "INSERT INTO keywords(version_id, keyword) VALUES (?1, 'gannet')", + [c.last_insert_rowid()], + ) + .unwrap(); + + let q = crate::Query { + filter: Selector::Keyword("gannet".into()), + ..Default::default() + }; + assert_eq!(cat.count(&q, 0).unwrap(), 1); + } + + #[test] + fn creating_the_same_word_twice_is_one_keyword() { + // A keyword is its text. Two rows with one name would be a duplicate + // in the list that no user could tell apart. + let cat = seeded(); + let a = create(cat.connection(), "iceland").unwrap(); + let b = create(cat.connection(), "iceland").unwrap(); + assert_eq!(a, b); + assert_eq!(names(&cat), ["iceland"]); + } + + #[test] + fn a_keyword_can_exist_before_any_photograph_carries_it() { + // Building the vocabulary ahead of the shoot is a real workflow, and + // it is the reason the term table exists at all. + let cat = seeded(); + create(cat.connection(), "puffin").unwrap(); + assert_eq!(names(&cat), ["puffin"]); + assert_eq!(list(cat.connection()).unwrap()[0].image_count, 0); + } + + #[test] + fn whitespace_around_a_keyword_is_not_part_of_it() { + // `query` compares with `=`, so " puffin" would be a second keyword + // that looked identical in every list and matched nothing the first + // matched. + let cat = seeded(); + assign(cat.connection(), &[img(1)], " puffin ").unwrap(); + assert_eq!(for_image(cat.connection(), img(1)).unwrap(), ["puffin"]); + } + + #[test] + fn inner_whitespace_collapses() { + let cat = seeded(); + assign(cat.connection(), &[img(1)], "black\tguillemot").unwrap(); + assert_eq!( + for_image(cat.connection(), img(1)).unwrap(), + ["black guillemot"] + ); + } + + #[test] + fn normalise_is_what_a_caller_can_show_the_user() { + // Public so a status line can quote the word as stored rather than as + // typed. If these two ever disagree, the UI is reporting a keyword the + // list will not show. + let cat = seeded(); + assign(cat.connection(), &[img(1)], " black \t guillemot ").unwrap(); + assert_eq!( + for_image(cat.connection(), img(1)).unwrap(), + [normalise(" black \t guillemot ").unwrap()] + ); + } + + #[test] + fn a_blank_keyword_is_refused_rather_than_stored() { + let cat = seeded(); + assert!(matches!( + assign(cat.connection(), &[img(1)], " "), + Err(CatalogError::BadName(_)) + )); + } + + #[test] + fn an_overlong_keyword_is_cut_on_a_character_boundary() { + // A mis-paste, not an intention. Truncating on a byte would panic on + // the multi-byte characters an Icelandic place name is full of. + let cat = seeded(); + let long = "Þingvellir".repeat(40); + assign(cat.connection(), &[img(1)], &long).unwrap(); + let stored = &for_image(cat.connection(), img(1)).unwrap()[0]; + assert_eq!(stored.chars().count(), MAX_KEYWORD_LEN); + } + + #[test] + fn case_is_preserved() { + // "Iceland" is a place; "iceland" is a typo of it. + let cat = seeded(); + assign(cat.connection(), &[img(1)], "Iceland").unwrap(); + assert_eq!(for_image(cat.connection(), img(1)).unwrap(), ["Iceland"]); + } + + #[test] + fn renaming_moves_every_assignment_with_it() { + // The failure this guards against: the vocabulary shows the new word + // and the search only finds the old one. + let cat = seeded(); + assign(cat.connection(), &[img(1), img(2)], "Icland").unwrap(); + let id = list(cat.connection()).unwrap()[0].id; + + rename(cat.connection(), id, "Iceland").unwrap(); + + assert_eq!(names(&cat), ["Iceland"]); + assert_eq!(for_image(cat.connection(), img(1)).unwrap(), ["Iceland"]); + let q = crate::Query { + filter: Selector::Keyword("Iceland".into()), + ..Default::default() + }; + assert_eq!(cat.count(&q, 0).unwrap(), 2); + } + + #[test] + fn renaming_onto_an_existing_keyword_fuses_the_two() { + // Correcting a typo when the correct spelling already exists. Refusing + // would leave the user to fix it photograph by photograph. + let cat = seeded(); + assign(cat.connection(), &[img(1)], "Icland").unwrap(); + assign(cat.connection(), &[img(2)], "Iceland").unwrap(); + let typo = list(cat.connection()) + .unwrap() + .into_iter() + .find(|k| k.name == "Icland") + .unwrap() + .id; + + rename(cat.connection(), typo, "Iceland").unwrap(); + + assert_eq!(names(&cat), ["Iceland"]); + assert_eq!(list(cat.connection()).unwrap()[0].image_count, 2); + } + + #[test] + fn renaming_a_frame_that_carries_both_spellings_does_not_abort() { + // The primary key would reject a bare UPDATE for that one row and take + // the whole rename down with it. + let cat = seeded(); + assign(cat.connection(), &[img(1)], "Icland").unwrap(); + assign(cat.connection(), &[img(1)], "Iceland").unwrap(); + let typo = list(cat.connection()) + .unwrap() + .into_iter() + .find(|k| k.name == "Icland") + .unwrap() + .id; + + rename(cat.connection(), typo, "Iceland").unwrap(); + assert_eq!(for_image(cat.connection(), img(1)).unwrap(), ["Iceland"]); + } + + #[test] + fn a_no_op_rename_is_not_an_edit() { + // A revision bumped for nothing lets an idle device win a merge + // against one that did real work. + let cat = seeded(); + let id = create(cat.connection(), "puffin").unwrap(); + let before = revision_of(&cat, id); + + assert_eq!(rename(cat.connection(), id, " puffin ").unwrap(), id); + assert_eq!(revision_of(&cat, id), before); + assert_eq!(deleted_of(&cat, id), 0, "and it does not retire the word"); + } + + #[test] + fn a_rename_retires_the_old_word_under_its_old_name() { + // The tombstone is what carries the rename to the other device, and it + // is read as "delete the assignments spelt this way" — so it has to + // keep the *old* spelling or it would delete the new word's. + let cat = seeded(); + assign(cat.connection(), &[img(1)], "Icland").unwrap(); + let old = list(cat.connection()).unwrap()[0].id; + + let new = rename(cat.connection(), old, "Iceland").unwrap(); + assert_ne!(new, old); + assert_eq!(deleted_of(&cat, old), 1); + assert_eq!(name_row(&cat, old), "Icland"); + assert!( + revision_of(&cat, old) > 1, + "the tombstone must out-revision the live row the other device holds" + ); + } + + fn revision_of(cat: &Catalog, id: KeywordId) -> i64 { + scalar(cat, "revision", id) + } + + fn deleted_of(cat: &Catalog, id: KeywordId) -> i64 { + scalar(cat, "deleted", id) + } + + /// One integer column of a term row. The column name is a literal from this + /// file and never user text — the same rule the module follows. + fn scalar(cat: &Catalog, column: &str, id: KeywordId) -> i64 { + cat.connection() + .query_row( + &format!("SELECT {column} FROM keyword_terms WHERE id = ?1"), + [id.0 as i64], + |r| r.get(0), + ) + .unwrap() + } + + fn name_row(cat: &Catalog, id: KeywordId) -> String { + cat.connection() + .query_row( + "SELECT name FROM keyword_terms WHERE id = ?1", + [id.0 as i64], + |r| r.get(0), + ) + .unwrap() + } + + #[test] + fn deleting_a_keyword_leaves_a_tombstone_and_takes_its_assignments() { + let cat = seeded(); + assign(cat.connection(), &[img(1), img(2)], "blurry").unwrap(); + let id = list(cat.connection()).unwrap()[0].id; + + assert_eq!(delete(cat.connection(), id).unwrap(), 2); + assert!(names(&cat).is_empty()); + assert!(for_image(cat.connection(), img(1)).unwrap().is_empty()); + + // The row survives, or a merge with a device that still holds the + // keyword would bring it straight back. + let deleted: i64 = cat + .connection() + .query_row( + "SELECT deleted FROM keyword_terms WHERE id = ?1", + [id.0 as i64], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(deleted, 1); + } + + #[test] + fn retyping_a_deleted_keyword_starts_a_fresh_identity() { + // Reusing the tombstoned row would make the new keyword inherit a + // revision that says "deleted" and lose an argument it was never in. + let cat = seeded(); + let first = create(cat.connection(), "puffin").unwrap(); + delete(cat.connection(), first).unwrap(); + + let second = create(cat.connection(), "puffin").unwrap(); + assert_ne!(first, second); + assert_eq!(names(&cat), ["puffin"]); + } + + #[test] + fn coverage_distinguishes_all_from_some() { + // The dash rather than the tick. Applying a word to forty frames where + // thirty already have it must not look like applying it to forty that + // do not. + let cat = seeded(); + assign(cat.connection(), &[img(1), img(2)], "puffin").unwrap(); + assign(cat.connection(), &[img(1)], "nest").unwrap(); + + let rows = for_images(cat.connection(), &[img(1), img(2)]).unwrap(); + let puffin = rows.iter().find(|r| r.keyword.name == "puffin").unwrap(); + let nest = rows.iter().find(|r| r.keyword.name == "nest").unwrap(); + + assert_eq!(puffin.coverage, Coverage::All); + assert_eq!(nest.coverage, Coverage::Some); + assert_eq!(nest.selected_count, 1); + } + + #[test] + fn a_keyword_no_one_in_the_selection_has_still_appears() { + // It is a target, not a report: the list is what the user assigns + // from, so hiding the unused words would hide the point of it. + let cat = seeded(); + create(cat.connection(), "puffin").unwrap(); + let rows = for_images(cat.connection(), &[img(1)]).unwrap(); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].coverage, Coverage::None); + } + + #[test] + fn the_vocabulary_leads_with_the_words_actually_used() { + let cat = seeded(); + create(cat.connection(), "unused").unwrap(); + assign(cat.connection(), &[img(1), img(2)], "puffin").unwrap(); + assert_eq!(names(&cat), ["puffin", "unused"]); + } + + #[test] + fn a_word_on_two_versions_counts_as_one_photograph() { + // A count that double-counts virtual copies is the kind of small lie + // that makes a user stop trusting the numbers. + let cat = seeded(); + let c = cat.connection(); + assign(c, &[img(1)], "puffin").unwrap(); + c.execute( + "INSERT INTO versions(image_id, uuid, name, is_default) VALUES (1, 'copy', 'Crop', 0)", + [], + ) + .unwrap(); + c.execute( + "INSERT INTO keywords(version_id, keyword) VALUES (?1, 'puffin')", + [c.last_insert_rowid()], + ) + .unwrap(); + + assert_eq!(list(c).unwrap()[0].image_count, 1); + } + + #[test] + fn an_image_with_no_version_gains_one_rather_than_losing_the_keyword() { + // A library scanned before versions existed. The user typed a word and + // expects it to stick. + let cat = seeded(); + let c = cat.connection(); + c.execute( + "INSERT INTO images(id, root_id, source_ref, added_at) + VALUES (99, 1, 'IMG_99.CR3', 0)", + [], + ) + .unwrap(); + + assign(c, &[img(99)], "puffin").unwrap(); + assert_eq!(for_image(c, img(99)).unwrap(), ["puffin"]); + } + + #[test] + fn a_hostile_keyword_is_stored_rather_than_executed() { + // The write side of `query`'s injection guard. It goes in as a + // parameter, comes back out unchanged, and the table is still there. + let cat = seeded(); + let evil = "'; DROP TABLE images; --"; + assign(cat.connection(), &[img(1)], evil).unwrap(); + + assert_eq!(for_image(cat.connection(), img(1)).unwrap(), [evil]); + let n: i64 = cat + .connection() + .query_row("SELECT count(*) FROM images", [], |r| r.get(0)) + .unwrap(); + assert_eq!(n, 6); + } + + #[test] + fn words_that_predate_the_vocabulary_are_adopted() { + // A catalog rebuilt from sidecars, or keyworded by an older build. + let cat = seeded(); + let c = cat.connection(); + let version = crate::rating::default_version_id(c, img(1)).unwrap(); + c.execute( + "INSERT INTO keywords(version_id, keyword) VALUES (?1, 'seabird')", + [version], + ) + .unwrap(); + + assert_eq!(adopt_orphan_terms(c).unwrap(), 1); + assert_eq!(names(&cat), ["seabird"]); + // It runs on every open, so a second pass must find nothing to do. + assert_eq!(adopt_orphan_terms(c).unwrap(), 0); + } + + #[test] + fn an_orphaned_word_is_still_shown_before_it_is_adopted() { + let cat = seeded(); + let c = cat.connection(); + let version = crate::rating::default_version_id(c, img(1)).unwrap(); + c.execute( + "INSERT INTO keywords(version_id, keyword) VALUES (?1, 'seabird')", + [version], + ) + .unwrap(); + + let rows = for_images(c, &[img(1)]).unwrap(); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].keyword.name, "seabird"); + assert_eq!(rows[0].keyword.id, KeywordId(0), "no identity yet"); + } + + #[test] + fn duplicate_identities_for_one_word_fuse_onto_the_smaller_uuid() { + // Two devices each typed "Iceland" before they had ever synced. + let cat = seeded(); + let c = cat.connection(); + c.execute( + "INSERT INTO keyword_terms(uuid, name, created, revision, modified) + VALUES ('aaaa', 'Iceland', 0, 1, 1), ('zzzz', 'Iceland', 0, 9, 9)", + [], + ) + .unwrap(); + + assert_eq!(fuse_duplicates(c).unwrap(), 1); + let survivor: String = c + .query_row("SELECT uuid FROM keyword_terms", [], |r| r.get(0)) + .unwrap(); + assert_eq!( + survivor, "aaaa", + "the rule must not consult the revision, or two devices pick differently" + ); + } + + #[test] + fn fusing_leaves_a_tombstone_alone() { + // A tombstone is the user's deletion. Retiring it as a duplicate would + // quietly undo it. + let cat = seeded(); + let c = cat.connection(); + c.execute( + "INSERT INTO keyword_terms(uuid, name, created, revision, modified, deleted) + VALUES ('aaaa', 'Iceland', 0, 1, 1, 0), ('zzzz', 'Iceland', 0, 9, 9, 1)", + [], + ) + .unwrap(); + + assert_eq!(fuse_duplicates(c).unwrap(), 0); + } + + #[test] + fn an_empty_selection_still_creates_the_keyword() { + // Typing a word into the field with nothing selected builds the + // vocabulary, which is a legitimate thing to do ahead of a shoot. + let cat = seeded(); + assert_eq!(assign(cat.connection(), &[], "puffin").unwrap(), 0); + assert_eq!(names(&cat), ["puffin"]); + } + + #[test] + fn renaming_a_keyword_that_is_gone_says_so() { + let cat = seeded(); + assert!(matches!( + rename(cat.connection(), KeywordId(404), "x"), + Err(CatalogError::NoSuchKeyword(404)) + )); + } +} diff --git a/core/dr-catalog/src/lib.rs b/core/dr-catalog/src/lib.rs index aca35d4..8a0994a 100644 --- a/core/dr-catalog/src/lib.rs +++ b/core/dr-catalog/src/lib.rs @@ -14,9 +14,10 @@ //! - [`walk`] — those decisions driven against real storage, local or SAF //! - [`query`] — selectors compiled to indexed SQL, windowed for the grid //! - [`collections`] — the collection tree and membership the UI edits +//! - [`keywords`] — the keyword vocabulary and what it is assigned to //! - [`jobs`] — the durable background work queue //! - [`trash`] — soft delete to a folder, then permanent delete -//! - [`merge`] / [`sync`] — cross-device collection merging +//! - [`merge`] / [`sync`] — cross-device merging of collections and keywords //! //! # The one thing everything is designed around //! @@ -35,6 +36,7 @@ pub mod cache; pub mod collections; pub mod error; pub mod jobs; +pub mod keywords; pub mod merge; pub mod query; pub mod rating; @@ -48,6 +50,7 @@ pub use cache::{Budget, Cache, DEFAULT_BUDGET_BYTES}; pub use collections::{Collection, CollectionKind, TreeRow}; pub use error::CatalogError; pub use jobs::{Job, JobKind, Priority}; +pub use keywords::{Coverage, Keyword, KeywordId, SelectionKeyword}; pub use merge::MergeReport; pub use query::{Query, Sort}; pub use rating::{Judgement, MAX_RATING}; diff --git a/core/dr-catalog/src/merge.rs b/core/dr-catalog/src/merge.rs index 710553b..7a1e50c 100644 --- a/core/dr-catalog/src/merge.rs +++ b/core/dr-catalog/src/merge.rs @@ -1,5 +1,5 @@ -//! TRACES: FR-CAT-7 | FR-NC-9 -//! Merging a remote catalog's collections into the local one. +//! TRACES: FR-CAT-7 | FR-CAT-5 | FR-NC-9 +//! Merging a remote catalog's collections and keywords into the local one. //! //! # Why this is a merge and not a copy //! @@ -33,6 +33,35 @@ //! against a device that still holds it would otherwise resurrect it. The //! tombstone carries a revision like any other edit, so deletion competes on //! the same footing as a rename. +//! +//! # Keywords merge on the same three rules +//! +//! [`merge_keywords`] reuses all of the above rather than inventing a second +//! set of rules, because a keyword is the same shape of problem as a +//! collection: a named thing with a device-independent identity, and a +//! many-to-many join to images. +//! +//! - The **vocabulary** (`keyword_terms`) is decided per row by [`verdict`], +//! exactly as collections are. +//! - The **assignments** (`keywords`) are a set union, exactly as membership +//! is: two devices each keywording different photographs "puffin" keep both +//! sets, and two devices each keywording the *same* photograph converge on +//! one row rather than one of them winning. +//! - **Deletion** tombstones, and takes the assignments with it. +//! +//! Two things are genuinely different, and both are consequences of assignments +//! storing the *word* rather than a row id: +//! +//! 1. A tombstone deletes assignments **by name**, so a deletion still lands on +//! a device that had minted its own identity for the same word. The union +//! then refuses to readmit a word a winning tombstone has just removed — +//! without that filter, the other device's live assignments would resurrect +//! it on the very same pass. +//! 2. Two devices that independently typed the same word arrive with two uuids +//! for one keyword. [`crate::keywords::fuse_duplicates`] collapses them onto +//! the lexicographically smaller one, which both devices compute identically. +//! A unique index on the name would instead abort the merge transaction at +//! that moment, which is the ordinary case rather than a corner one. use rusqlite::Connection; @@ -59,18 +88,44 @@ pub struct MergeReport { pub kept_local: usize, pub deleted: usize, pub members_added: usize, + + // Keywords are counted separately from collections rather than summed into + // the same fields. The report is shown to the user — "3 collections, 11 + // keywords" is a sentence; "14 things" is not — and a merge that went wrong + // is far easier to place when the counts say which half it went wrong in. + /// Keywords the remote had and this device did not. + pub keywords_inserted: usize, + /// Keywords the remote had renamed, or brought back from a tombstone. + pub keywords_updated: usize, + /// Keywords the remote deleted, and this device has now deleted too. + pub keywords_deleted: usize, + /// Keywords where this device's revision was at least as high. + pub keywords_kept_local: usize, + /// Redundant identities for one word, retired by + /// [`crate::keywords::fuse_duplicates`]. + pub keywords_fused: usize, + /// Keyword assignments taken from the remote. + pub keywords_assigned: usize, } impl MergeReport { /// Whether the local catalog changed, and so needs re-uploading. pub fn local_changed(&self) -> bool { - self.inserted > 0 || self.updated > 0 || self.deleted > 0 || self.members_added > 0 + self.inserted > 0 + || self.updated > 0 + || self.deleted > 0 + || self.members_added > 0 + || self.keywords_inserted > 0 + || self.keywords_updated > 0 + || self.keywords_deleted > 0 + || self.keywords_fused > 0 + || self.keywords_assigned > 0 } /// Whether the local catalog holds anything the remote did not, and so /// must be uploaded even if nothing was taken from the remote. pub fn should_upload(&self) -> bool { - self.kept_local > 0 || self.local_changed() + self.kept_local > 0 || self.keywords_kept_local > 0 || self.local_changed() } } @@ -108,16 +163,55 @@ pub fn verdict( } } -/// Merge collections and membership from an attached catalog. +/// Merge everything that syncs, from an attached catalog. /// /// The remote catalog must already be attached under the schema name -/// `remote_cat`; [`crate::Catalog::merge_attached_collections`] handles that. +/// `remote_cat`; [`crate::sync::merge_remote`] handles that. +/// +/// **One transaction over both halves.** Keywords and collections are +/// independent as data, but a merge that landed the collections and then failed +/// on the keywords would leave a catalog that has already taken the remote's +/// revisions for half of itself — and the next attempt, seeing those revisions, +/// would decline to take them again. Half a merge is not a state that can be +/// resumed, so it is not a state that can be reached. +pub fn merge_all(conn: &Connection) -> Result { + let tx = conn.unchecked_transaction()?; + let mut report = MergeReport::default(); + merge_collections_within(&tx, &mut report)?; + merge_keywords_within(&tx, &mut report)?; + tx.commit()?; + Ok(report) +} + +/// Merge collections and membership from an attached catalog. +/// +/// The collections half of [`merge_all`], on its own. Kept as a public entry +/// point because the two halves are genuinely independent, and because the +/// rules for this one are worth being able to exercise without a keyword in +/// sight. /// /// Runs in one transaction: a merge either lands whole or not at all. pub fn merge_collections(conn: &Connection) -> Result { let tx = conn.unchecked_transaction()?; let mut report = MergeReport::default(); + merge_collections_within(&tx, &mut report)?; + tx.commit()?; + Ok(report) +} +/// Merge the keyword vocabulary and its assignments from an attached catalog. +/// +/// The keywords half of [`merge_all`], on its own. See the module header for +/// the three rules and the two places keywords differ from collections. +pub fn merge_keywords(conn: &Connection) -> Result { + let tx = conn.unchecked_transaction()?; + let mut report = MergeReport::default(); + merge_keywords_within(&tx, &mut report)?; + tx.commit()?; + Ok(report) +} + +fn merge_collections_within(tx: &Connection, report: &mut MergeReport) -> Result<(), CatalogError> { // ---- collections ------------------------------------------------------ { let mut stmt = tx.prepare( @@ -310,8 +404,245 @@ pub fn merge_collections(conn: &Connection) -> Result )?; report.members_added = added; - tx.commit()?; - Ok(report) + Ok(()) +} + +/// Schema name the downloaded remote catalog is attached under. +/// +/// Repeated from [`crate::sync`] rather than shared, because the SQL below +/// spells it inline and a constant that only half the file used would be worse +/// than no constant at all. +const REMOTE: &str = "remote_cat"; + +/// The keyword half. See the module header. +fn merge_keywords_within(tx: &Connection, report: &mut MergeReport) -> Result<(), CatalogError> { + // ---- the vocabulary --------------------------------------------------- + // + // A remote written before schema v6 has no `keyword_terms` at all, and + // `remote_is_mergeable` deliberately admits it: the check is that the + // remote is not *newer* than us. So the table's absence is a normal state + // and not an error. Its assignments still merge below — those have been in + // the schema since v1 — and its words gain identities on that device the + // next time it opens the catalog and backfills. + if attached_has_table(tx, REMOTE, "keyword_terms")? { + struct Incoming { + uuid: String, + name: String, + /// What this device currently calls the same identity, if it has + /// it. A rename is applied to the assignment rows by rewriting this + /// text, so it has to be read before the term row is overwritten. + local_name: Option, + created: i64, + revision: i64, + modified: i64, + verdict: MergeVerdict, + } + + let rows: Vec = { + let mut stmt = tx.prepare( + "SELECT r.uuid, r.name, r.created, r.revision, r.modified, r.deleted, + l.name, l.revision, l.modified + FROM remote_cat.keyword_terms r + LEFT JOIN main.keyword_terms l ON l.uuid = r.uuid", + )?; + let found = stmt + .query_map([], |r| { + let deleted: i64 = r.get(5)?; + let local_rev: Option = r.get(7)?; + let local_mod: Option = r.get(8)?; + let revision: i64 = r.get(3)?; + let modified: i64 = r.get(4)?; + Ok(Incoming { + uuid: r.get(0)?, + name: r.get(1)?, + local_name: r.get(6)?, + created: r.get(2)?, + revision, + modified, + verdict: verdict( + local_rev.zip(local_mod), + (revision, modified), + deleted != 0, + ), + }) + })? + .collect::, _>>()?; + found + }; + + for row in rows { + match row.verdict { + MergeVerdict::KeptLocal => { + report.keywords_kept_local += 1; + } + MergeVerdict::InsertedFromRemote => { + tx.execute( + "INSERT INTO main.keyword_terms + (uuid, name, created, revision, modified, deleted) + VALUES (?1, ?2, ?3, ?4, ?5, 0)", + rusqlite::params![ + row.uuid, + row.name, + row.created, + row.revision, + row.modified, + ], + )?; + report.keywords_inserted += 1; + } + MergeVerdict::UpdatedFromRemote => { + // The assignments carry the *word*, so taking a new name + // for an identity we already hold means rewriting every row + // spelt the old way. Without this the vocabulary would show + // the new spelling and the search would only find the old. + if let Some(old) = row.local_name.filter(|n| *n != row.name) { + tx.execute( + "INSERT OR IGNORE INTO main.keywords(version_id, keyword) + SELECT version_id, ?2 FROM main.keywords WHERE keyword = ?1", + rusqlite::params![old, row.name], + )?; + tx.execute("DELETE FROM main.keywords WHERE keyword = ?1", [&old])?; + } + tx.execute( + "UPDATE main.keyword_terms + SET name = ?2, revision = ?3, modified = ?4, deleted = 0 + WHERE uuid = ?1", + rusqlite::params![row.uuid, row.name, row.revision, row.modified], + )?; + report.keywords_updated += 1; + } + MergeVerdict::DeletedByRemote => { + // Tombstone rather than DELETE, or a third device + // reintroduces the keyword through us. + tx.execute( + "INSERT INTO main.keyword_terms + (uuid, name, created, revision, modified, deleted) + VALUES (?1, ?2, ?3, ?4, ?5, 1) + ON CONFLICT(uuid) DO UPDATE SET + deleted = 1, revision = ?4, modified = ?5", + rusqlite::params![ + row.uuid, + row.name, + row.created, + row.revision, + row.modified, + ], + )?; + // **By name, not by identity.** This device may well have + // minted its own uuid for the same word before the two ever + // synced, in which case deleting by uuid would tombstone a + // row that nothing is assigned to and leave every + // photograph still carrying the word. + tx.execute("DELETE FROM main.keywords WHERE keyword = ?1", [&row.name])?; + report.keywords_deleted += 1; + } + } + } + + // Two devices that each typed "Iceland" now hold two identities for one + // word. Collapse them before the assignments arrive, so the vocabulary + // the user sees after a sync has one row per word. + report.keywords_fused = crate::keywords::fuse_duplicates(tx)?; + } + + // ---- assignments ------------------------------------------------------ + // + // Set union, and the union is the whole point: FR-NC-9's principle applied + // to metadata rather than to edit nodes. Two devices that keyworded + // different frames "puffin" both keep their work, and neither loses it to + // whichever synced second. + // + // A removal therefore does not propagate — the remote's assignment simply + // reappears. That is the same trade-off collection membership makes above, + // and for the same reason: an unwanted keyword is removed again in a + // second, and a silently lost afternoon of keywording is not recoverable at + // all. Making removal propagate needs a tombstone per assignment, which is + // a schema change and a merge rule of its own. + // + // An incoming keyword lands on the local default version, so an image that + // has not got one yet would silently drop it. That is not a rare state: the + // invariant is maintained by a backfill on open, and a scan that ran since + // has added rows it has not covered. Losing a word the user typed on + // another device, for a bookkeeping reason, would be the wrong answer — + // this is idempotent and writes nothing once the invariant holds. + crate::rating::ensure_default_versions_within(tx)?; + + // Two passes rather than one statement with an `OR`, because they resolve + // *different identities* for the same photograph and each wants its own + // index. See [`ASSIGN_BY_FILE_ID`] for why there are two at all. + for sql in [ASSIGN_BY_FILE_ID, ASSIGN_BY_CONTENT_HASH] { + report.keywords_assigned += tx.execute(sql, [])?; + } + + Ok(()) +} + +/// Take assignments for images both devices know by the server's file id. +/// +/// **Preferred over the content hash**, and the reason is that `content_hash` +/// is expensive — the schema says so, and it is computed only when import +/// dedup or a reconnect asks for it, which for most libraries is never. Keying +/// keywords on it alone would mean the union quietly did nothing for the +/// ordinary image, which is the exact failure this merge exists to prevent. +/// +/// `oc:fileid` is the opposite: it is recorded for every image the moment a +/// remote scan sees it, it is stable across server-side renames and moves, and +/// it is the same integer on every device pointed at the same Nextcloud — which +/// is precisely the situation where two devices are keywording one library. +/// +/// The keyword lands on the local image's **default version**, not on the +/// version it came from. Version uuids do not reconcile across devices in the +/// catalog: [`crate::rating::ensure_default_versions`] mints a fresh one per +/// device, so the same photograph's default versions have different uuids on +/// two machines and a uuid-keyed join would union nothing at all. Version +/// identity is reconciled in the *sidecar* (FR-NC-8), and until a merged +/// version arrives through there, the default version is both where +/// [`crate::keywords::assign`] writes and where the panel reads — so it is the +/// one place the word can land and be seen. +const ASSIGN_BY_FILE_ID: &str = " + INSERT OR IGNORE INTO main.keywords(version_id, keyword) + SELECT lv.id, rk.keyword + FROM remote_cat.keywords rk + JOIN remote_cat.versions rv ON rv.id = rk.version_id + JOIN remote_cat.remote rr ON rr.image_id = rv.image_id + JOIN main.remote lr ON lr.file_id = rr.file_id + JOIN main.versions lv ON lv.image_id = lr.image_id AND lv.is_default = 1 + WHERE NOT EXISTS (SELECT 1 FROM main.keyword_terms t + WHERE t.name = rk.keyword AND t.deleted = 1) + OR EXISTS (SELECT 1 FROM main.keyword_terms t + WHERE t.name = rk.keyword AND t.deleted = 0)"; + +/// The same union for a library with no server behind it. +/// +/// A local-only library has no `remote` rows at all, so [`ASSIGN_BY_FILE_ID`] +/// matches nothing and this is the only identity available — and it is the one +/// collection membership already uses, so a library where membership merges +/// has keywords that merge too. +const ASSIGN_BY_CONTENT_HASH: &str = " + INSERT OR IGNORE INTO main.keywords(version_id, keyword) + SELECT lv.id, rk.keyword + FROM remote_cat.keywords rk + JOIN remote_cat.versions rv ON rv.id = rk.version_id + JOIN remote_cat.images ri ON ri.id = rv.image_id + JOIN main.images li ON li.content_hash = ri.content_hash + JOIN main.versions lv ON lv.image_id = li.id AND lv.is_default = 1 + WHERE ri.content_hash IS NOT NULL + AND (NOT EXISTS (SELECT 1 FROM main.keyword_terms t + WHERE t.name = rk.keyword AND t.deleted = 1) + OR EXISTS (SELECT 1 FROM main.keyword_terms t + WHERE t.name = rk.keyword AND t.deleted = 0))"; + +/// Whether an attached database holds a table of this name. +/// +/// The schema name and the table name are both literals from this file, never +/// user text — but they are still bound rather than formatted where SQLite +/// allows it, because the habit is what keeps the one that eventually is user +/// text from being formatted by accident. +fn attached_has_table(conn: &Connection, schema: &str, table: &str) -> Result { + let sql = + format!("SELECT count(*) FROM {schema}.sqlite_master WHERE type = 'table' AND name = ?1"); + let n: i64 = conn.query_row(&sql, [table], |r| r.get(0))?; + Ok(n > 0) } #[cfg(test)] @@ -391,12 +722,21 @@ mod tests { // ---- integration over two real catalogs ------------------------------ fn two_catalogs() -> Connection { + attached_remote(schema::for_attached("remote_cat")) + } + + /// The same pair, but with the remote stopped at v1 — a device running a + /// build from before keywords had identities. + fn two_catalogs_with_a_v1_remote() -> Connection { + attached_remote(schema::v1_for_attached("remote_cat")) + } + + fn attached_remote(remote_schema: String) -> Connection { let c = Connection::open_in_memory().unwrap(); schema::configure(&c).unwrap(); schema::migrate(&c).unwrap(); // A second in-memory database standing in for the downloaded remote. c.execute_batch("ATTACH ':memory:' AS remote_cat").unwrap(); - let remote_schema = super::super::schema::v1_for_attached("remote_cat"); c.execute_batch(&remote_schema).unwrap(); c } @@ -656,6 +996,330 @@ mod tests { assert!(!second.local_changed(), "merge must be idempotent"); } + // ---- keywords -------------------------------------------------------- + + /// Give an image a default version, as every write path assumes it has. + fn add_version(c: &Connection, db: &str, image: i64, uuid: &str) -> i64 { + c.execute( + &format!( + "INSERT INTO {db}.versions(image_id, uuid, name, is_default) + VALUES (?1, ?2, 'Default', 1)" + ), + rusqlite::params![image, uuid], + ) + .unwrap(); + c.last_insert_rowid() + } + + /// Put a word on an image's default version, creating the version. + /// + /// The version uuid is derived from the database *and* the image, so the + /// two catalogs never accidentally agree on one — which is the real + /// situation, and the reason the assignment union cannot key on it. + fn keyword(c: &Connection, db: &str, image: i64, word: &str) { + let existing: Option = c + .query_row( + &format!("SELECT id FROM {db}.versions WHERE image_id = ?1 AND is_default = 1"), + [image], + |r| r.get(0), + ) + .ok(); + let version = + existing.unwrap_or_else(|| add_version(c, db, image, &format!("v-{db}-{image}"))); + c.execute( + &format!("INSERT OR IGNORE INTO {db}.keywords(version_id, keyword) VALUES (?1, ?2)"), + rusqlite::params![version, word], + ) + .unwrap(); + } + + fn add_term(c: &Connection, db: &str, uuid: &str, name: &str, rev: i64, deleted: i64) { + c.execute( + &format!( + "INSERT INTO {db}.keyword_terms(uuid, name, created, revision, modified, deleted) + VALUES (?1, ?2, 0, ?3, ?3, ?4)" + ), + rusqlite::params![uuid, name, rev, deleted], + ) + .unwrap(); + } + + /// Map an image to a server file id, as a remote scan does. + fn add_file_id(c: &Connection, db: &str, image: i64, file_id: i64) { + c.execute( + &format!("INSERT INTO {db}.remote(image_id, file_id) VALUES (?1, ?2)"), + rusqlite::params![image, file_id], + ) + .unwrap(); + } + + /// Every word on an image locally, sorted. + fn words_on(c: &Connection, image: i64) -> Vec { + let mut stmt = c + .prepare( + "SELECT DISTINCT k.keyword FROM main.keywords k + JOIN main.versions v ON v.id = k.version_id + WHERE v.image_id = ?1 ORDER BY k.keyword", + ) + .unwrap(); + let rows = stmt.query_map([image], |r| r.get(0)).unwrap(); + rows.collect::, _>>().unwrap() + } + + fn live_terms(c: &Connection) -> Vec { + let mut stmt = c + .prepare("SELECT name FROM main.keyword_terms WHERE deleted = 0 ORDER BY name") + .unwrap(); + let rows = stmt.query_map([], |r| r.get(0)).unwrap(); + rows.collect::, _>>().unwrap() + } + + #[test] + fn two_devices_keywording_different_photographs_both_survive() { + // FR-NC-9's principle applied to metadata: disjoint work merges to the + // union, and neither device loses an afternoon to whoever synced last. + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + add_image(&c, db, 2, "hash-b"); + } + keyword(&c, "main", 1, "puffin"); + keyword(&c, "remote_cat", 2, "gannet"); + + merge_keywords(&c).unwrap(); + + assert_eq!(words_on(&c, 1), ["puffin"]); + assert_eq!(words_on(&c, 2), ["gannet"]); + } + + #[test] + fn two_devices_keywording_one_photograph_keep_both_words() { + // The case the union is really for: the same frame, two different + // words, and last-writer-wins would silently drop one of them. + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + keyword(&c, "main", 1, "puffin"); + keyword(&c, "remote_cat", 1, "Iceland"); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_assigned, 1); + assert_eq!(words_on(&c, 1), ["Iceland", "puffin"]); + } + + #[test] + fn a_word_both_devices_already_had_is_not_duplicated() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + keyword(&c, db, 1, "puffin"); + } + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_assigned, 0); + assert_eq!(words_on(&c, 1), ["puffin"]); + } + + #[test] + fn keywords_reach_an_image_the_server_names_but_no_one_has_hashed() { + // `content_hash` is computed only when import dedup or a reconnect asks + // for it, so for most images it is NULL — and a union keyed on it alone + // would quietly do nothing for the ordinary photograph. The file id is + // recorded by every remote scan, which is exactly the situation where + // two devices are keywording one library. + let c = two_catalogs(); + c.execute( + "INSERT INTO main.roots(id, kind, label) VALUES (1, 'remote', 'r')", + [], + ) + .unwrap(); + c.execute( + "INSERT INTO remote_cat.roots(id, kind, label) VALUES (1, 'remote', 'r')", + [], + ) + .unwrap(); + // Different row ids for one photograph, and no hash on either side. + c.execute( + "INSERT INTO main.images(id, root_id, source_ref, added_at) + VALUES (77, 1, 'IMG_1.CR3', 0)", + [], + ) + .unwrap(); + c.execute( + "INSERT INTO remote_cat.images(id, root_id, source_ref, added_at) + VALUES (3, 1, 'IMG_1.CR3', 0)", + [], + ) + .unwrap(); + add_file_id(&c, "main", 77, 9001); + add_file_id(&c, "remote_cat", 3, 9001); + add_version(&c, "main", 77, "v-main"); + keyword(&c, "remote_cat", 3, "puffin"); + + merge_keywords(&c).unwrap(); + assert_eq!(words_on(&c, 77), ["puffin"]); + } + + #[test] + fn keywords_map_across_devices_by_content_hash_where_there_is_no_server() { + // A local-only library has no `remote` rows at all, so the hash is the + // only identity available — and it is the one membership already uses. + let c = two_catalogs(); + add_image(&c, "main", 77, "same-photo"); + add_image(&c, "remote_cat", 3, "same-photo"); + add_version(&c, "main", 77, "v-main"); + keyword(&c, "remote_cat", 3, "puffin"); + + merge_keywords(&c).unwrap(); + assert_eq!(words_on(&c, 77), ["puffin"]); + } + + #[test] + fn the_vocabulary_merges_by_uuid_and_a_skewed_clock_cannot_win() { + let c = two_catalogs(); + add_term(&c, "main", "u-1", "Iceland", 9, 0); + add_term(&c, "remote_cat", "u-1", "iceland", 2, 0); + add_term(&c, "remote_cat", "u-2", "puffin", 1, 0); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_kept_local, 1); + assert_eq!(report.keywords_inserted, 1); + assert_eq!(live_terms(&c), ["Iceland", "puffin"]); + } + + #[test] + fn a_remote_rename_moves_this_device_s_assignments_too() { + // The failure this exists to stop: the vocabulary shows the corrected + // spelling and the search still only finds the old one. + let c = two_catalogs(); + add_image(&c, "main", 1, "hash-a"); + add_term(&c, "main", "u-1", "Icland", 1, 0); + keyword(&c, "main", 1, "Icland"); + add_term(&c, "remote_cat", "u-1", "Iceland", 4, 0); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_updated, 1); + assert_eq!(live_terms(&c), ["Iceland"]); + assert_eq!(words_on(&c, 1), ["Iceland"]); + } + + #[test] + fn a_remote_deletion_takes_the_word_off_every_photograph() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + add_term(&c, "main", "u-1", "blurry", 1, 0); + keyword(&c, "main", 1, "blurry"); + add_term(&c, "remote_cat", "u-1", "blurry", 5, 1); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_deleted, 1); + assert!(live_terms(&c).is_empty()); + assert!(words_on(&c, 1).is_empty()); + } + + #[test] + fn a_deletion_is_not_undone_by_the_union_on_the_same_pass() { + // The remote deleted the word *and* still carries assignments for it — + // it has not yet had the chance to sweep them, or a third device put + // them there. Without the tombstone filter the union would put the word + // straight back on the photograph the deletion had just cleared. + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + add_term(&c, "main", "u-1", "blurry", 1, 0); + keyword(&c, "main", 1, "blurry"); + add_term(&c, "remote_cat", "u-1", "blurry", 5, 1); + keyword(&c, "remote_cat", 1, "blurry"); + + merge_keywords(&c).unwrap(); + assert!(words_on(&c, 1).is_empty(), "a deleted keyword came back"); + } + + #[test] + fn a_deletion_lands_even_when_the_two_devices_minted_different_uuids() { + // Both typed "blurry" before they ever synced, so this device's row has + // a uuid the remote has never heard of. Deleting by identity would + // tombstone nothing and leave every photograph still carrying the word. + let c = two_catalogs(); + add_image(&c, "main", 1, "hash-a"); + add_term(&c, "main", "mine", "blurry", 1, 0); + keyword(&c, "main", 1, "blurry"); + add_term(&c, "remote_cat", "theirs", "blurry", 5, 1); + + merge_keywords(&c).unwrap(); + assert!(words_on(&c, 1).is_empty()); + } + + #[test] + fn two_devices_that_typed_one_word_end_up_with_one_keyword() { + // Neither is wrong until they meet, which is why the name carries no + // unique index — a constraint would abort the merge at this moment. + let c = two_catalogs(); + add_term(&c, "main", "zzzz", "Iceland", 3, 0); + add_term(&c, "remote_cat", "aaaa", "Iceland", 1, 0); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_fused, 1); + assert_eq!(live_terms(&c), ["Iceland"]); + + let survivor: String = c + .query_row("SELECT uuid FROM main.keyword_terms", [], |r| r.get(0)) + .unwrap(); + assert_eq!( + survivor, "aaaa", + "both devices must pick the same survivor without asking each other" + ); + } + + #[test] + fn merging_keywords_twice_changes_nothing_the_second_time() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + add_term(&c, "remote_cat", "u-1", "puffin", 1, 0); + keyword(&c, "remote_cat", 1, "puffin"); + + let first = merge_keywords(&c).unwrap(); + assert!(first.local_changed()); + let second = merge_keywords(&c).unwrap(); + assert!(!second.local_changed(), "merge must be idempotent"); + } + + #[test] + fn a_remote_from_before_keyword_identities_still_contributes_its_words() { + // `remote_is_mergeable` admits an older remote on purpose — the check + // is that it is not *newer* than us. A missing table is therefore a + // normal state and must not fail the merge. + let c = two_catalogs_with_a_v1_remote(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + keyword(&c, "remote_cat", 1, "puffin"); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_assigned, 1); + assert_eq!(words_on(&c, 1), ["puffin"]); + } + + #[test] + fn merge_all_lands_both_halves() { + let c = two_catalogs(); + add_collection(&c, "remote_cat", 1, "u-coll", "Portugal", 1); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + keyword(&c, "remote_cat", 1, "puffin"); + + let report = merge_all(&c).unwrap(); + assert_eq!(report.inserted, 1); + assert_eq!(report.keywords_assigned, 1); + } + #[test] fn keeping_local_still_marks_the_catalog_for_upload() { // We hold something the remote does not, so the remote is stale even diff --git a/core/dr-catalog/src/rating.rs b/core/dr-catalog/src/rating.rs index 1c90595..139acd9 100644 --- a/core/dr-catalog/src/rating.rs +++ b/core/dr-catalog/src/rating.rs @@ -75,6 +75,24 @@ impl Judgement { /// The UUID is per row and generated here — it is the merge identity across /// devices (FR-NC-8), so two images must never share one. pub fn ensure_default_versions(conn: &Connection) -> Result { + // One transaction for the batch. A backfill over a 24k-image library is + // 24k inserts, and per-statement commits would make it minutes rather + // than seconds. + let tx = conn.unchecked_transaction()?; + let n = ensure_default_versions_within(&tx)?; + tx.commit()?; + Ok(n) +} + +/// [`ensure_default_versions`] without opening a transaction. +/// +/// Separate because SQLite has no nested `BEGIN`: [`crate::merge`] needs the +/// invariant restored *inside* the merge transaction — an incoming keyword +/// lands on a default version, so an image without one would silently drop it — +/// and calling the public form there fails at runtime with "cannot start a +/// transaction within a transaction". The same split, for the same reason, as +/// `collections::add_within`. +pub fn ensure_default_versions_within(conn: &Connection) -> Result { let ids: Vec = { let mut stmt = conn.prepare( "SELECT i.id FROM images i @@ -89,12 +107,8 @@ pub fn ensure_default_versions(conn: &Connection) -> Result return Ok(0); } - // One transaction for the batch. A backfill over a 24k-image library is - // 24k inserts, and per-statement commits would make it minutes rather - // than seconds. - let tx = conn.unchecked_transaction()?; { - let mut insert = tx.prepare( + let mut insert = conn.prepare( "INSERT INTO versions(image_id, uuid, name, is_default, rating, flag) VALUES (?1, ?2, ?3, 1, 0, 0)", )?; @@ -102,7 +116,6 @@ pub fn ensure_default_versions(conn: &Connection) -> Result insert.execute(rusqlite::params![id, new_uuid(), DEFAULT_VERSION_NAME])?; } } - tx.commit()?; Ok(ids.len()) } diff --git a/core/dr-catalog/src/schema.rs b/core/dr-catalog/src/schema.rs index da13480..271f772 100644 --- a/core/dr-catalog/src/schema.rs +++ b/core/dr-catalog/src/schema.rs @@ -15,7 +15,7 @@ use rusqlite::Connection; use crate::error::CatalogError; /// Schema version this build writes and understands. -pub const SCHEMA_VERSION: i64 = 5; +pub const SCHEMA_VERSION: i64 = 6; /// Apply migrations up to [`SCHEMA_VERSION`]. /// @@ -66,6 +66,12 @@ pub fn migrate(conn: &Connection) -> Result { tx.pragma_update(None, "user_version", 5)?; tx.commit()?; } + if from < 6 { + let tx = conn.unchecked_transaction()?; + tx.execute_batch(V6)?; + tx.pragma_update(None, "user_version", 6)?; + tx.commit()?; + } Ok(from) } @@ -103,6 +109,20 @@ pub fn backfill(conn: &Connection) -> Result, Catalog out.push(("default_versions", n)); } + // v6: a vocabulary row for every word some image already carries. + // + // Three ways a catalog arrives holding assignments with no term behind + // them, and all three are normal rather than exceptional: a library + // keyworded by a build that predates this table, a catalog rebuilt from + // sidecars (which carry the word and not the identity), and an import from + // Lightroom or darktable (FR-CAT-14). Without this the words are + // searchable but absent from the vocabulary list, which reads as the + // keywords having been lost. + let n = crate::keywords::adopt_orphan_terms(conn)?; + if n > 0 { + out.push(("keyword_terms", n)); + } + Ok(out) } @@ -134,15 +154,45 @@ pub fn configure(conn: &Connection) -> Result<(), CatalogError> { /// in [`V1`]: every `CREATE TABLE`/`CREATE INDEX` must name its object /// unqualified, which they do. pub fn v1_for_attached(schema_name: &str) -> String { - V1.replace("CREATE TABLE ", &format!("CREATE TABLE {schema_name}.")) + rewrite_for_attached(V1, schema_name) + // REFERENCES within an attached schema resolve to that schema already, + // so foreign keys need no rewriting — but the ON clause of an index + // does, and `CREATE INDEX x.name ON table` is the correct form. +} + +/// Every table this build knows about, rewritten to target an attached +/// database. +/// +/// [`v1_for_attached`] is kept alongside this rather than replaced by it: a +/// remote catalog written by an older build genuinely has only the v1 tables, +/// and the merge has to keep working against one (see +/// [`crate::merge::merge_keywords`]). Building that case in a test needs a way +/// to say "v1 and no more". +/// +/// Only the migrations that *create* objects appear here. V2 through V5 are +/// `ALTER TABLE ... ADD COLUMN`, and the columns they add are local index +/// state — shadowing, trashing, cache pinning — that a merge never reads +/// across the attachment. +pub fn for_attached(schema_name: &str) -> String { + format!( + "{}\n{}", + rewrite_for_attached(V1, schema_name), + rewrite_for_attached(V6, schema_name) + ) +} + +/// Qualify every object a `CREATE` statement names with `schema_name`. +/// +/// The rewrite is textual and therefore only as good as the naming discipline +/// in the batches it is given: every `CREATE TABLE`/`CREATE INDEX` must name +/// its object unqualified, which they do. +fn rewrite_for_attached(sql: &str, schema_name: &str) -> String { + sql.replace("CREATE TABLE ", &format!("CREATE TABLE {schema_name}.")) .replace("CREATE INDEX ", &format!("CREATE INDEX {schema_name}.")) .replace( "CREATE UNIQUE INDEX ", &format!("CREATE UNIQUE INDEX {schema_name}."), ) - // REFERENCES within an attached schema resolve to that schema already, - // so foreign keys need no rewriting — but the ON clause of an index - // does, and `CREATE INDEX x.name ON table` is the correct form. } /// Mark each JPEG that sits beside a RAW of the same name. @@ -226,6 +276,63 @@ fn stem_of(path: &str) -> &str { } } +const V6: &str = r#" +-- TRACES: FR-CAT-5 | FR-CAT-6 | FR-NC-9 +-- Keywords gain an identity, so that renaming and deleting one can cross +-- between devices. +-- +-- The v1 `keywords` table is the *assignment*: one row per (version, word), +-- and the word is stored as text. That stays exactly as it is, and this +-- migration adds nothing to it, for a reason that is easy to get backwards. +-- +-- # Why assignments keep the text rather than pointing at a row here +-- +-- The catalog is a rebuildable index (ARCH §6.12). What an image is keyworded +-- with is authoritative in the sidecar and in XMP `dc:subject` (FR-CAT-13), +-- and both of those carry a *string*. Rewriting the join to reference +-- `keyword_terms(id)` would mean a catalog rebuilt from sidecars had to invent +-- term rows before it could record a single assignment, and an integer that +-- means nothing on the other device would sit where the durable fact belongs. +-- It would also break `crate::query`, which matches `kw.keyword` directly and +-- must keep hitting `keywords_term` on a 50k library (FR-CAT-6). +-- +-- So the text is the fact and this table is the *identity*: it exists to give +-- a rename and a deletion something a merge can key on, and to let a keyword +-- exist in the vocabulary before any photograph carries it. +CREATE TABLE keyword_terms ( + id INTEGER PRIMARY KEY, + -- Device-independent identity, as `collections.uuid` is. The integer id is + -- local and collides across devices. + uuid TEXT NOT NULL UNIQUE, + -- The word itself, and the value written into every assignment row. + name TEXT NOT NULL, + created INTEGER NOT NULL, + -- Monotonic, bumped on every local edit. `crate::merge` compares these + -- rather than timestamps, so a clock-skewed device cannot silently win. + revision INTEGER NOT NULL DEFAULT 1, + modified INTEGER NOT NULL, + -- Tombstone, so a merge against a device that still holds the keyword does + -- not resurrect it. + deleted INTEGER NOT NULL DEFAULT 0 +); + +-- Deliberately **not** UNIQUE. +-- +-- Two devices that each type "Iceland" create two rows with two uuids, and +-- both are correct until they meet. A unique constraint would abort the merge +-- transaction at exactly that moment — the ordinary case, not a corner one. +-- Uniqueness is instead reached by convergence: `crate::keywords::create` +-- resolves an existing name locally, and `crate::keywords::fuse_duplicates` +-- collapses a cross-device pair onto the lexicographically smaller uuid, which +-- both devices compute identically without talking to each other. +-- +-- Partial on `deleted = 0` because every lookup here is a live one: the +-- vocabulary list, the resolve-by-name in `create`, and the fuse pass all +-- exclude tombstones, and including them would grow the index with every +-- keyword the library has ever had rather than with the ones it has. +CREATE INDEX keyword_terms_name ON keyword_terms(name) WHERE deleted = 0; +"#; + const V5: &str = r#" -- TRACES: FR-NC-6a | FR-CAT-9 | NFR-RES-4 -- Offline availability: what is kept, why it is kept, and where it lives. @@ -671,6 +778,77 @@ mod tests { assert_eq!(bytes, 100, "the existing row is untouched"); } + #[test] + fn a_v5_catalog_keeps_its_keywords_and_gains_their_identities() { + // TRACES: FR-CAT-5 + // The migration case that matters here: a library keyworded by an + // import or an older build already has assignment rows, and they must + // survive into the vocabulary rather than being left searchable but + // invisible. + let c = mem(); + for step in [V1, V2, V3, V4, V5] { + c.execute_batch(step).unwrap(); + } + c.pragma_update(None, "user_version", 5).unwrap(); + c.execute( + "INSERT INTO roots(id, kind, label) VALUES (1, 'local', 'lib')", + [], + ) + .unwrap(); + c.execute( + "INSERT INTO images(id, root_id, source_ref, added_at) VALUES (7, 1, 'IMG_7.CR3', 0)", + [], + ) + .unwrap(); + c.execute( + "INSERT INTO versions(id, image_id, uuid, name, is_default) + VALUES (1, 7, 'v-7', 'Default', 1)", + [], + ) + .unwrap(); + c.execute( + "INSERT INTO keywords(version_id, keyword) VALUES (1, 'puffin')", + [], + ) + .unwrap(); + + assert_eq!(migrate(&c).unwrap(), 5, "migrated from v5"); + assert_eq!(backfilled(&c, "keyword_terms"), 1); + + let name: String = c + .query_row("SELECT name FROM keyword_terms", [], |r| r.get(0)) + .unwrap(); + assert_eq!(name, "puffin"); + // The assignment is untouched — it is the durable fact, and the term + // row is only its identity. + let n: i64 = c + .query_row("SELECT count(*) FROM keywords", [], |r| r.get(0)) + .unwrap(); + assert_eq!(n, 1); + + // It runs on every open, so a second pass must find nothing to do. + assert_eq!(backfilled(&c, "keyword_terms"), 0); + } + + #[test] + fn two_devices_may_both_hold_a_term_of_the_same_name() { + // Deliberately not a unique index. Two devices each typing "Iceland" + // is the ordinary case, and a constraint would abort the merge + // transaction at exactly the moment they first sync. + let c = mem(); + migrate(&c).unwrap(); + c.execute( + "INSERT INTO keyword_terms(uuid, name, created, revision, modified) + VALUES ('a', 'Iceland', 0, 1, 1), ('b', 'Iceland', 0, 1, 1)", + [], + ) + .unwrap(); + let n: i64 = c + .query_row("SELECT count(*) FROM keyword_terms", [], |r| r.get(0)) + .unwrap(); + assert_eq!(n, 2); + } + #[test] fn stems_ignore_directories_containing_dots() { assert_eq!(stem_of("2026.08/IMG_1.CR2"), "IMG_1"); diff --git a/core/dr-catalog/src/sync.rs b/core/dr-catalog/src/sync.rs index 7cabf36..01d302d 100644 --- a/core/dr-catalog/src/sync.rs +++ b/core/dr-catalog/src/sync.rs @@ -16,10 +16,11 @@ //! //! # What is actually synced //! -//! Only collections merge (see [`crate::merge`]). The rest of the catalog is a -//! *local index* of *local* storage — folder mtimes, cache paths, job rows — -//! and copying another device's version of those in would be actively wrong. -//! The remote file is read for its collections and then discarded. +//! Only the *user's judgements about their library* merge: collections, and the +//! keyword vocabulary with its assignments (see [`crate::merge`]). The rest of +//! the catalog is a *local index* of *local* storage — folder mtimes, cache +//! paths, job rows — and copying another device's version of those in would be +//! actively wrong. The remote file is read for those two and then discarded. //! //! This is why the catalog remains disposable in the ARCH §6.12 sense: nothing //! here makes the local database authoritative for anything a rebuild could @@ -96,7 +97,7 @@ pub fn merge_remote(conn: &Connection, remote: &Path) -> Result, images: &[dr_types::ImageId]) { + let borrow = ctl.catalog.borrow(); + let Some(catalog) = borrow.as_ref() else { + return; + }; + + let rows = match dr_catalog::keywords::for_images(catalog.connection(), images) { + Ok(rows) => rows, + Err(e) => { + // The grid is entirely usable without the sheet, so this is logged + // rather than surfaced: a keyword read that failed must not put an + // error banner over a library the user is browsing. + log::debug!("reading keywords: {e}"); + return; + } + }; + + let model: Vec = rows + .into_iter() + .map(|row| KeywordRow { + id: row.keyword.id.0 as i32, + name: row.keyword.name.into(), + coverage: match row.coverage { + dr_catalog::Coverage::None => 0, + dr_catalog::Coverage::Some => 1, + dr_catalog::Coverage::All => 2, + }, + selected_count: row.selected_count as i32, + image_count: row.keyword.image_count as i32, + }) + .collect(); + + window.set_library_keywords(slint::ModelRc::new(slint::VecModel::from(model))); +} + +/// Put a keyword on the selection, or take it off. +/// +/// # Why this does not write a sidecar +/// +/// Every other judgement in this file — a star, a flag — is written to the +/// catalog and then queued to the image's sidecar, because the sidecar is what +/// makes it survive a catalog rebuild (ARCH §6.12). A keyword has no place in +/// the sidecar format yet: `dr_pipeline::sidecar::Version` carries `rating` and +/// `flag` and nothing else that is not an edit-graph parameter. +/// +/// So a keyword is, for now, catalog state that reaches the user's other +/// devices through the *catalog* merge ([`dr_catalog::merge`]) rather than +/// through the sidecar. That is a real limitation and not a silent one: a +/// deleted catalog loses keywords where it would keep ratings, until the +/// sidecar gains a `dc:subject` field (FR-CAT-13) and this grows the same +/// queued write the stars have. +fn apply_keyword(window: &AppWindow, ctl: &Rc, word: &str, assigning: bool) { + let Some(coll) = ctl.coll_ctl.borrow().as_ref().and_then(|c| c.upgrade()) else { + return; + }; + let images = coll.selected(); + + // The word as it will be *stored*, resolved before anything is written. + // The status line below quotes it back, and quoting what was typed would + // report a leading space the catalog is about to drop — leaving the user to + // wonder whether it mattered. + // + // This is also where a blank keyword is caught, which is why it happens + // before the selection check: "you typed nothing" is a better answer than + // "select an image first" to someone who pressed return on an empty field. + let word = match dr_catalog::keywords::normalise(word) { + Ok(word) => word, + Err(e) => { + // `BadName` carries text written to be read by the user rather than + // by a developer, so it is shown as it is. + window.set_library_error(format!("{e}").into()); + return; + } + }; + + // Assigning with nothing selected still means something — it puts the word + // in the vocabulary, ready for the photographs it was typed for — so only + // the removal half needs a selection to act on. + if images.is_empty() && !assigning { + window.set_library_status("Select an image first".into()); + return; + } + + let outcome = { + let borrow = ctl.catalog.borrow(); + let Some(catalog) = borrow.as_ref() else { + return; + }; + let conn = catalog.connection(); + if assigning { + dr_catalog::keywords::assign(conn, &images, &word) + } else { + dr_catalog::keywords::unassign(conn, &images, &word) + } + }; + + let n = match outcome { + Ok(n) => n, + Err(e) => { + window.set_library_error(format!("{e}").into()); + return; + } + }; + + window.set_library_error(slint::SharedString::new()); + window.set_library_status(keyword_summary(&word, n, images.len(), assigning).into()); + refresh_keywords(window, ctl, &images); + + // A filtered grid may no longer hold what was just keyworded — taking + // "puffin" off an image while showing only puffins means it belongs + // elsewhere now. The same reasoning as a rating that falls below the star + // filter. + if !ctl.filter.borrow().is_unfiltered() { + load_window(window, ctl); + } +} + +/// What the status line says about a keyword that just landed. +/// +/// The honest count, not the requested one: "added to 3 of 12" is what +/// happened when nine of them already carried the word, and a message that +/// claimed twelve would be teaching the user that the counts are decorative. +fn keyword_summary(word: &str, changed: usize, selected: usize, assigning: bool) -> String { + if selected == 0 { + return format!("Added “{word}” to the keyword list"); + } + let verb = if assigning { "Added" } else { "Removed" }; + let preposition = if assigning { "to" } else { "from" }; + if changed == 0 { + return if assigning { + format!("Every selected photograph already had “{word}”") + } else { + format!("None of the selected photographs had “{word}”") + }; + } + if changed == selected { + let what = if selected == 1 { + "1 photograph".to_string() + } else { + format!("{selected} photographs") + }; + return format!("{verb} “{word}” {preposition} {what}"); + } + format!("{verb} “{word}” {preposition} {changed} of {selected}") +} + /// Apply a judgement to a set of images: catalog first, then sidecars. /// /// # Order matters @@ -4152,6 +4316,40 @@ pub fn wire( }); } + // --- keywords (FR-CAT-5, FR-CAT-6) ------------------------------------ + // + // Three callbacks and no state of their own: the sheet's open/shut is local + // to the `.slint` file, and what a keyword applies to is the grid selection + // the collections controller already owns. A second copy of either here is + // a second thing that can disagree with the first. + { + let weak = window.as_weak(); + let ctl = ctl.clone(); + let coll_for_keywords = coll_ctl.clone(); + window.on_library_keywords_opened(move || { + let Some(w) = weak.upgrade() else { return }; + refresh_keywords(&w, &ctl, &coll_for_keywords.selected()); + }); + } + + { + let weak = window.as_weak(); + let ctl = ctl.clone(); + window.on_library_assign_keyword(move |word| { + let Some(w) = weak.upgrade() else { return }; + apply_keyword(&w, &ctl, word.as_str(), true); + }); + } + + { + let weak = window.as_weak(); + let ctl = ctl.clone(); + window.on_library_unassign_keyword(move |word| { + let Some(w) = weak.upgrade() else { return }; + apply_keyword(&w, &ctl, word.as_str(), false); + }); + } + // --- the filter bar --------------------------------------------------- // // Each of these narrows what the grid *queries*, so all three reset the @@ -5017,6 +5215,71 @@ mod tests { assert_eq!(paths, vec!["c.CR2", "a.CR2"]); } + // --- what the status line says about a keyword (FR-CAT-5) ------------- + // + // Split out from the callback for the same reason `decide_drop` is: the + // sheet cannot be driven from a test, and this is the part that can + // actually mislead someone. + + /// TRACES: FR-CAT-5 + #[test] + fn a_partly_applied_keyword_reports_the_honest_count() { + // Nine of the twelve already had it. Claiming twelve is how a user + // learns that the counts are decorative. + assert_eq!( + keyword_summary("puffin", 3, 12, true), + "Added “puffin” to 3 of 12" + ); + } + + /// TRACES: FR-CAT-5 + #[test] + fn a_keyword_that_changed_nothing_says_so_rather_than_claiming_success() { + assert_eq!( + keyword_summary("puffin", 0, 12, true), + "Every selected photograph already had “puffin”" + ); + assert_eq!( + keyword_summary("puffin", 0, 12, false), + "None of the selected photographs had “puffin”" + ); + } + + /// TRACES: FR-CAT-5 + #[test] + fn one_photograph_is_singular() { + // "Added to 1 photographs" is the kind of small wrongness that makes + // the rest of the interface look unfinished. + assert_eq!( + keyword_summary("puffin", 1, 1, true), + "Added “puffin” to 1 photograph" + ); + assert_eq!( + keyword_summary("puffin", 2, 2, true), + "Added “puffin” to 2 photographs" + ); + } + + /// TRACES: FR-CAT-5 + #[test] + fn removing_a_keyword_reads_as_removal() { + assert_eq!( + keyword_summary("blurry", 4, 4, false), + "Removed “blurry” from 4 photographs" + ); + } + + /// TRACES: FR-CAT-5 + #[test] + fn typing_a_word_with_nothing_selected_says_what_it_did_do() { + // It builds the vocabulary, which is a legitimate thing to do ahead of + // a shoot — so it must not report itself as having keyworded nothing. + assert_eq!( + keyword_summary("puffin", 0, 0, true), + "Added “puffin” to the keyword list" + ); + } + /// TRACES: FR-EXP-7 #[test] fn a_selection_outside_the_loaded_window_still_resolves() { diff --git a/ui/dr-ui/ui/app.slint b/ui/dr-ui/ui/app.slint index ce62ec6..e0b1f78 100644 --- a/ui/dr-ui/ui/app.slint +++ b/ui/dr-ui/ui/app.slint @@ -2,7 +2,7 @@ import { Theme } from "theme.slint"; import { AdjustPanel, GeometryPanel, ModeStrip, ParamRow, TransferPanel, ViewMode } from "adjust.slint"; import { GradientHandle, HandleRole, MaskPanel, MaskRow, SubjectRow } from "masks.slint"; import { LaunchScreen } from "launch.slint"; -import { LibraryGrid, LibraryCell, TimelineBar, PhotoRoll } from "library.slint"; +import { LibraryGrid, LibraryCell, TimelineBar, PhotoRoll, KeywordRow } from "library.slint"; import { Button, PanelHeading, Label, Value, Caption, Panel, EmptyState, ProgressBar, ActivityRow } from "widgets.slint"; import { CollectionsPanel, CollectionRow, OfflinePrompt } from "collections.slint"; import { HistogramPanel, HistogramView } from "histogram.slint"; @@ -589,6 +589,25 @@ export component AppWindow inherits Window { /// the target's id, and whether to take the images out of the collection /// currently being shown. callback library-file-in-collection(int, bool); + /// TRACES: FR-CAT-5 | FR-CAT-6 + /// Keywording the grid's selection. The catalog has been searchable by + /// keyword since it existed and there was nowhere to type one; this is it. + /// + /// The vocabulary arrives already answered against the selection — each row + /// says how many of the selected photographs carry that word — because only + /// Rust knows what is selected, and a `.slint` file counting it would need + /// the selection as a second model that could disagree with the first. + in property <[KeywordRow]> library-keywords; + /// The sheet is opening: recompute the rows against the selection as it + /// stands now. Pulled rather than pushed, because the selection changes on + /// every arrow key and the sheet is shut for almost all of them. + callback library-keywords-opened(); + /// Put a keyword on the selection, creating it if it is new. By name, so a + /// word typed into the field and a word tapped in the list are one path. + callback library-assign-keyword(string); + /// Take a keyword off the selection. Never deletes the keyword itself — + /// it stays in the vocabulary and on every other photograph that carries it. + callback library-unassign-keyword(string); /// TRACES: FR-UI-2 /// Whether a tap in the grid selects rather than opens, and the button /// that turns it on. The long press does the same thing without it. @@ -1268,6 +1287,10 @@ in property panel-visible: true; file-in-collection(id, moves) => { root.library-file-in-collection(id, moves); } + keywords: root.library-keywords; + keywords-opened() => { root.library-keywords-opened(); } + assign-keyword(word) => { root.library-assign-keyword(word); } + unassign-keyword(word) => { root.library-unassign-keyword(word); } cursor: root.library-cursor; move-cursor(delta, extend) => { root.library-move-cursor(delta, extend); diff --git a/ui/dr-ui/ui/library.slint b/ui/dr-ui/ui/library.slint index 5089a45..4e1a50f 100644 --- a/ui/dr-ui/ui/library.slint +++ b/ui/dr-ui/ui/library.slint @@ -9,12 +9,38 @@ // must not look identical (FR-NC-6c). import { Theme } from "theme.slint"; -import { Button, IconButton, Label, Value, Caption, EmptyState, FilterChip, ProgressBar, Icon } from "widgets.slint"; +import { Button, IconButton, Label, Value, Caption, EmptyState, FilterChip, ProgressBar, Icon, Field } from "widgets.slint"; // The filing sheet lists the same rows the sidebar draws, from the same model: // two lists of collections that could disagree about what exists is one list // too many. import { CollectionRow } from "collections.slint"; +// TRACES: FR-CAT-5 +// One keyword in the keywording sheet, already answered against the selection. +// +// The three-way `coverage` is the whole reason this is a struct rather than a +// list of strings. Applying a word to forty photographs where thirty already +// carry it must not look like applying it to forty that carry none, and +// removing one that only some of them carry must not silently claim to have +// taken it off all forty. Rust computes it, because only Rust knows how big the +// selection is and how many of it each word covers. +export struct KeywordRow { + // Row id in `keyword_terms`, or 0 for a word an image carries that the + // vocabulary has no identity for yet. The sheet acts on `name`, never on + // this, so a 0 costs nothing — it is here so a future rename gesture has + // something to name. + id: int, + name: string, + // 0 none of the selection, 1 some of it, 2 all of it. + coverage: int, + // How many of the selected photographs carry it, for the "3 of 12" that + // makes `coverage: 1` a number rather than a shrug. + selected-count: int, + // How many photographs in the whole library carry it. Lets a word in + // regular use be told from one typed once by mistake. + image-count: int, +} + // One bar of the capture-time histogram. export struct TimelineBar { // 0..1, relative to the tallest bucket. Square-rooted in Rust so a quiet @@ -656,6 +682,9 @@ component HeaderActions inherits HorizontalLayout { callback remove-from-collection(); /// Open the sheet that files the selection in a collection. callback add-to-collection(); + /// TRACES: FR-CAT-5 + /// Open the sheet that keywords the selection. + callback add-keyword(); callback toggle-select-mode(); callback change-library(); callback toggle-pin-scope(); @@ -694,6 +723,17 @@ component HeaderActions inherits HorizontalLayout { clicked => { root.add-to-collection(); } } + // TRACES: FR-CAT-5 | FR-CAT-6 + // Keyword the selection. Beside "Add to collection" because they are the + // same thought — these photographs are *of* something, and they belong + // *with* something — and appearing under the same condition, because + // neither means anything without a selection to act on. + if root.selected-count > 0: Button { + text: "Keywords"; + y: root.centred ? (root.row-height - self.height) / 2 : 0; + clicked => { root.add-keyword(); } + } + // TRACES: FR-DEV-6 // Batch-apply the copied settings. Shown only with both a selection and a // clipboard, because it is meaningless without either — and because a @@ -1105,6 +1145,34 @@ export component LibraryGrid inherits Rectangle { /// out of the one currently being shown. callback file-in-collection(int, bool); + // --- keywording the selection (FR-CAT-5, FR-CAT-6) ---------------------- + // + // The catalog has been searchable by keyword since it existed and there was + // never anywhere to type one. This sheet is that place, and it sits beside + // the filing sheet above because the two are the same gesture applied to + // two different kinds of label — pick the photographs, then say what they + // are — and a user who has learnt one should not have to learn the other. + // + // Assign and unassign travel by **name**, not by id. A word typed into the + // field and a word tapped in the list are then one path through Rust rather + // than two, and the sheet does not have to invent an id for a keyword that + // does not exist yet. + /// The vocabulary, already answered against the current selection. + in property <[KeywordRow]> keywords; + /// The sheet is opening: Rust answers by refreshing `keywords` against + /// whatever is selected *now*. + /// + /// Pulled on open rather than pushed on every selection change, because the + /// selection changes on every arrow key and the sheet is shut for almost + /// all of them — recomputing coverage over a forty-image selection for a + /// panel nobody is looking at is work the grid cannot afford. + callback keywords-opened(); + callback assign-keyword(string); + callback unassign-keyword(string); + /// Whether the sheet is up. Local, for the same reason `filing` is: it is a + /// disclosure rather than a preference, and what closes it is dismissing it. + property keywording: false; + // Cell geometry. Columns are derived from the available width so the grid // reflows with the window rather than fixing a count (FR-UI-1). // Zoomable, so the grid serves both jobs: fewer, larger images for @@ -1355,6 +1423,14 @@ export component LibraryGrid inherits Rectangle { // to is still there when the sheet closes. root.actions-open = false; } + add-keyword => { + // Ask for the vocabulary before showing the sheet, so + // it is answered against the selection as it stands now + // rather than as it stood when the grid last loaded. + root.keywords-opened(); + root.keywording = true; + root.actions-open = false; + } change-library => { root.change-library(); } toggle-pin-scope => { root.toggle-pin-scope(); } sync-now => { root.sync-now(); } @@ -1429,6 +1505,14 @@ export component LibraryGrid inherits Rectangle { // to is still there when the sheet closes. root.actions-open = false; } + add-keyword => { + // Ask for the vocabulary before showing the sheet, so + // it is answered against the selection as it stands now + // rather than as it stood when the grid last loaded. + root.keywords-opened(); + root.keywording = true; + root.actions-open = false; + } change-library => { root.change-library(); } toggle-pin-scope => { root.toggle-pin-scope(); } sync-now => { root.sync-now(); } @@ -1788,6 +1872,10 @@ export component LibraryGrid inherits Rectangle { // button, and a sheet it walked straight past would leave // the user out of the grid with their selection gone. if (event.text == Key.Back || event.text == Key.Escape) { + if (root.keywording) { + root.keywording = false; + return accept; + } if (root.filing) { root.filing = false; return accept; @@ -2486,4 +2574,184 @@ export component LibraryGrid inherits Rectangle { } } } + + // --- the keywording sheet (FR-CAT-5, FR-CAT-6) -------------------------- + // + // "These are of…". Deliberately the same card, scrim and dismissal as the + // filing sheet above: a user who has filed a selection already knows how + // this works, and a second idiom for the same gesture would be a second + // thing to learn for no gain. + // + // It stays open after each word, where the filing sheet closes. Filing is + // one choice; keywording is usually several — "puffin", "Látrabjarg", + // "2026" — and a sheet that shut after each one would have to be reopened, + // and the selection re-confirmed, three times over. + if root.keywording: Rectangle { + background: #000000CC; + + // Swallows the taps that miss the card, and closes. First, so the + // card's own controls sit above it. + TouchArea { + clicked => { root.keywording = false; } + } + + Rectangle { + width: min(420px, parent.width - 2 * Theme.gap-lg); + height: min(kw-sheet.preferred-height, parent.height - 2 * Theme.gap-lg); + x: (parent.width - self.width) / 2; + y: (parent.height - self.height) / 2; + background: Theme.surface; + border-radius: Theme.radius; + border-width: 1px; + border-color: Theme.rule; + + // Stops a press on the card reaching the scrim behind it. + TouchArea { } + + kw-sheet := VerticalLayout { + padding: Theme.gap-lg; + spacing: Theme.gap; + + Text { + text: root.selected-count == 1 + ? "Keywords for 1 photograph" + : "Keywords for " + root.selected-count + " photographs"; + color: Theme.ink; + font-size: Theme.text-lg; + font-weight: 600; + wrap: word-wrap; + } + + // Typing a word applies it, whether or not it already exists. + // One field for both, because "is this keyword new?" is a + // question about the catalog and not about what the user meant, + // and Rust can answer it without being asked. + // + // The field clears itself on accept so the next word can be + // typed straight after — keywording a shoot is a run of them. + new-keyword := Field { + placeholder: "Type a keyword and press return"; + accepted(text) => { + root.assign-keyword(text); + self.text = ""; + } + } + + Rectangle { height: 1px; background: Theme.rule; } + + Flickable { + vertical-stretch: 1; + // A floor, so the list is not squeezed out of existence by + // the field and the button around it on a short window. + min-height: 120px; + viewport-height: root.keywords.length * (Theme.touch-target + 2px); + + for word[i] in root.keywords: Rectangle { + y: i * (Theme.touch-target + 2px); + width: parent.width; + // A full touch target per row, for the same reason the + // filing sheet uses one: this is a place to hit once, + // with a thumb, holding a selection that took a minute + // to build (FR-UI-3). + height: Theme.touch-target; + background: kw-touch.pressed ? Theme.pressed + : (kw-touch.has-hover ? Theme.hover : transparent); + border-radius: Theme.radius-sm; + + HorizontalLayout { + padding-left: Theme.gap-sm; + padding-right: Theme.gap-sm; + spacing: Theme.gap-sm; + + // Tick, dash, or nothing — the three states of + // `coverage`, drawn as three different marks rather + // than as two. A half-applied keyword shown as + // applied is a lie about photographs the user + // cannot see from here. + Rectangle { + width: 16px; + y: (parent.height - self.height) / 2; + height: 16px; + border-radius: Theme.radius-sm; + border-width: 1px; + border-color: word.coverage == 0 ? Theme.rule : Theme.active; + background: word.coverage == 2 ? Theme.active : transparent; + + // The dash for "some of them". A bar rather + // than a tick, because a tick at half strength + // reads as a rendering artefact. + if word.coverage == 1: Rectangle { + width: 8px; + height: 2px; + x: (parent.width - self.width) / 2; + y: (parent.height - self.height) / 2; + background: Theme.active; + } + if word.coverage == 2: Icon { + name: "check"; + ink: Theme.surface; + size: 12px; + x: (parent.width - self.width) / 2; + y: (parent.height - self.height) / 2; + } + } + + Text { + text: word.name; + color: Theme.ink; + font-size: Theme.text; + vertical-alignment: center; + overflow: elide; + horizontal-stretch: 1; + } + + // "3 of 12" only where it says something the mark + // does not. For a word the whole selection carries, + // or none of it, the mark has already said it and + // the number would be noise on every row. + Text { + text: word.coverage == 1 + ? word.selected-count + " of " + root.selected-count + : (word.image-count > 0 ? word.image-count + "" : ""); + color: Theme.ink-faint; + font-size: Theme.text-sm; + vertical-alignment: center; + } + } + + // One target for both directions. A word the selection + // fully carries comes off; anything else goes on — so a + // partly-applied keyword is completed rather than + // removed, which is what a user tapping a dash means + // nine times in ten, and the tenth is one more tap + // away. + kw-touch := TouchArea { + clicked => { + if (word.coverage == 2) { + root.unassign-keyword(word.name); + } else { + root.assign-keyword(word.name); + } + } + } + } + + if root.keywords.length == 0: Text { + text: "No keywords yet. Type one above to make the first."; + color: Theme.ink-faint; + font-size: Theme.text-sm; + wrap: word-wrap; + width: parent.width; + } + } + + Rectangle { height: 1px; background: Theme.rule; } + + Button { + text: "Done"; + clicked => { root.keywording = false; } + } + } + } + } }