Merge master into film-simulation

Master gained the library's index paging while this branch was building
the film simulation, and the two met in library_ui.rs. Only the generated
traceability matrix conflicted; it is regenerated here rather than
hand-resolved, which is what it is for.

ui/dr-ui/src/library.rs

@@ -2953,10 +2953,8 @@ pub fn read_cells_scoped(
         .join(",");
     let rated = filter.sql();
     let sql = format!(
-        "SELECT i.id, i.source_ref, r.file_id, i.file_size,
-                i.metadata_state, i.captured_at
+        "SELECT {CELL_COLUMNS}
            FROM images i
-           LEFT JOIN remote r ON r.image_id = i.id
           WHERE {VISIBLE}{rated}
             AND i.id IN (SELECT image_id FROM collection_members
                           WHERE collection_id IN ({placeholders}))
@@ -2971,10 +2969,14 @@ pub fn read_cells_scoped(
     params.push(rusqlite::types::Value::Integer(limit as i64));
     params.push(rusqlite::types::Value::Integer(offset as i64));
 
-    let mut stmt = catalog.connection().prepare(&sql)?;
-    let rows = stmt
-        .query_map(rusqlite::params_from_iter(params.iter()), row_to_cell)?
-        .collect::<Result<Vec<_>, _>>()?;
+    let mut rows = {
+        let mut stmt = catalog.connection().prepare(&sql)?;
+        let read = stmt
+            .query_map(rusqlite::params_from_iter(params.iter()), row_to_cell)?
+            .collect::<Result<Vec<_>, _>>()?;
+        read
+    };
+    attach_file_ids(catalog, &mut rows);
     Ok(rows)
 }
 
@@ -2985,18 +2987,20 @@ fn read_cells_all(
     limit: usize,
 ) -> Result<Vec<LibraryCell>, dr_catalog::CatalogError> {
     let rated = filter.sql();
-    let mut stmt = catalog.connection().prepare(&format!(
-        "SELECT i.id, i.source_ref, r.file_id, i.file_size,
-                i.metadata_state, i.captured_at
-           FROM images i
-           LEFT JOIN remote r ON r.image_id = i.id
-          WHERE {VISIBLE}{rated}
-          {GRID_ORDER}
-          LIMIT ?1 OFFSET ?2"
-    ))?;
-    let rows = stmt
-        .query_map(rusqlite::params![limit as i64, offset as i64], row_to_cell)?
-        .collect::<Result<Vec<_>, _>>()?;
+    let mut rows = {
+        let mut stmt = catalog.connection().prepare(&format!(
+            "SELECT {CELL_COLUMNS}
+               FROM images i
+              WHERE {VISIBLE}{rated}
+              {GRID_ORDER}
+              LIMIT ?1 OFFSET ?2"
+        ))?;
+        let read = stmt
+            .query_map(rusqlite::params![limit as i64, offset as i64], row_to_cell)?
+            .collect::<Result<Vec<_>, _>>()?;
+        read
+    };
+    attach_file_ids(catalog, &mut rows);
     Ok(rows)
 }
 
@@ -3017,18 +3021,20 @@ pub fn read_trashed_cells(
     offset: usize,
     limit: usize,
 ) -> Result<Vec<LibraryCell>, dr_catalog::CatalogError> {
-    let mut stmt = catalog.connection().prepare(&format!(
-        "SELECT i.id, i.source_ref, r.file_id, i.file_size,
-                i.metadata_state, i.captured_at
-           FROM images i
-           LEFT JOIN remote r ON r.image_id = i.id
-          WHERE {TRASHED}
-          {TRASH_ORDER}
-          LIMIT ?1 OFFSET ?2"
-    ))?;
-    let rows = stmt
-        .query_map(rusqlite::params![limit as i64, offset as i64], row_to_cell)?
-        .collect::<Result<Vec<_>, _>>()?;
+    let mut rows = {
+        let mut stmt = catalog.connection().prepare(&format!(
+            "SELECT {CELL_COLUMNS}
+               FROM images i
+              WHERE {TRASHED}
+              {TRASH_ORDER}
+              LIMIT ?1 OFFSET ?2"
+        ))?;
+        let read = stmt
+            .query_map(rusqlite::params![limit as i64, offset as i64], row_to_cell)?
+            .collect::<Result<Vec<_>, _>>()?;
+        read
+    };
+    attach_file_ids(catalog, &mut rows);
     Ok(rows)
 }
 
@@ -3108,20 +3114,90 @@ pub fn read_ids_span(
     Ok(ids)
 }
 
+/// The columns every windowed read selects, in the order [`row_to_cell`] reads
+/// them.
+///
+/// Named rather than repeated so the three readers cannot drift — and so that
+/// the one column that is *not* here stays conspicuous. See
+/// [`attach_file_ids`] for why the server's file id is fetched separately.
+const CELL_COLUMNS: &str =
+    "i.id, i.source_ref, i.file_size, i.metadata_state, i.captured_at";
+
 /// Shared row mapping, so the scoped and unscoped queries cannot drift.
+///
+/// `file_id` is left empty here and filled by [`attach_file_ids`].
 fn row_to_cell(r: &rusqlite::Row) -> rusqlite::Result<LibraryCell> {
     let path: String = r.get(1)?;
     Ok(LibraryCell {
         image_id: r.get(0)?,
         name: path.rsplit(['/', ':']).next().unwrap_or(&path).to_string(),
         remote_path: path,
-        file_id: r.get::<_, Option<i64>>(2)?.map(|v| v as u64),
-        size: r.get::<_, Option<i64>>(3)?.unwrap_or(0) as u64,
-        metadata_state: r.get::<_, i64>(4)? as u8,
-        captured_at: r.get(5)?,
+        file_id: None,
+        size: r.get::<_, Option<i64>>(2)?.unwrap_or(0) as u64,
+        metadata_state: r.get::<_, i64>(3)? as u8,
+        captured_at: r.get(4)?,
     })
 }
 
+/// TRACES: NFR-P5
+/// Fill in each cell's server file id, in one query for the whole window.
+///
+/// # Why this is not a `LEFT JOIN` any more
+///
+/// It was, and it was the single most expensive thing the grid did while a
+/// finger was on it. A window is `ORDER BY ... LIMIT n OFFSET k`, and SQLite
+/// answers a join like that by joining *first* and paging after — so reading
+/// 280 cells at offset 20,000 meant an index seek into `remote` for all 24,000
+/// rows, 23,720 of which were then discarded. Measured at 15.2 ms, inside the
+/// scroll handler, against a 16.7 ms frame.
+///
+/// Paging over `images` alone is 0.36 ms with `images_grid_order` (schema V7),
+/// and this fetches the ids for the 280 rows that survived. The same shape the
+/// badge and rating reads already use: one query for the window, never one per
+/// cell.
+///
+/// Silent on failure, and cells keep `file_id: None`: that is the same state a
+/// photograph the scan has not reached the server for is in, and the callers
+/// already treat it as "no cached thumbnail to key on" rather than an error.
+fn attach_file_ids(catalog: &Catalog, cells: &mut [LibraryCell]) {
+    if cells.is_empty() {
+        return;
+    }
+    let placeholders = std::iter::repeat_n("?", cells.len())
+        .collect::<Vec<_>>()
+        .join(",");
+    let sql = format!("SELECT image_id, file_id FROM remote WHERE image_id IN ({placeholders})");
+    let params: Vec<rusqlite::types::Value> = cells
+        .iter()
+        .map(|c| rusqlite::types::Value::Integer(c.image_id))
+        .collect();
+
+    let mut stmt = match catalog.connection().prepare(&sql) {
+        Ok(s) => s,
+        Err(e) => {
+            log::debug!("reading file ids for the window: {e}");
+            return;
+        }
+    };
+    let rows = stmt.query_map(rusqlite::params_from_iter(params.iter()), |r| {
+        Ok((r.get::<_, i64>(0)?, r.get::<_, Option<i64>>(1)?))
+    });
+    let found: std::collections::HashMap<i64, Option<i64>> = match rows {
+        Ok(rows) => rows.flatten().collect(),
+        Err(e) => {
+            log::debug!("reading file ids for the window: {e}");
+            return;
+        }
+    };
+    for cell in cells {
+        cell.file_id = found
+            .get(&cell.image_id)
+            .copied()
+            .flatten()
+            .map(|v| v as u64);
+    }
+}
+
 /// Total images in the catalog, or in one collection and its descendants.
 ///
 /// Counts exactly what [`read_cells_scoped`] would list, filter included. The
@@ -3836,6 +3912,63 @@ mod tests {
         assert_eq!(page[0].name, "img010.CR2");
     }
 
+    /// TRACES: NFR-P5
+    /// The grid's window read must be answered by walking `images_grid_order`,
+    /// never by sorting the library into a temp b-tree.
+    ///
+    /// This asserts on the *query plan* rather than on a duration, because the
+    /// failure has no other symptom: a `GRID_ORDER` edited out of step with the
+    /// index in schema V7, or a column added back into the paging query that
+    /// drags `remote` in again, both still return the right cells. They just
+    /// return them after sorting 24,000 rows, inside the scroll handler — which
+    /// is the jitter this pair was introduced to remove, and it would come back
+    /// silently.
+    #[test]
+    fn the_window_read_walks_the_ordering_index() {
+        let catalog = with_images(20);
+        let plan: Vec<String> = catalog
+            .connection()
+            .prepare(&format!(
+                "EXPLAIN QUERY PLAN
+                 SELECT {CELL_COLUMNS} FROM images i
+                  WHERE {VISIBLE}
+                  {GRID_ORDER}
+                  LIMIT 10 OFFSET 5"
+            ))
+            .unwrap()
+            .query_map([], |r| r.get::<_, String>(3))
+            .unwrap()
+            .flatten()
+            .collect();
+        let plan = plan.join(" | ");
+
+        assert!(
+            plan.contains("images_grid_order"),
+            "the window read is not using the ordering index: {plan}"
+        );
+        assert!(
+            !plan.contains("TEMP B-TREE"),
+            "the window read is still sorting the whole library: {plan}"
+        );
+        assert!(
+            !plan.to_lowercase().contains("remote"),
+            "the window read is joining `remote` again, which pages the whole \
+             library before it discards it: {plan}"
+        );
+    }
+
+    /// The file ids still arrive, now that they come from a second query.
+    #[test]
+    fn a_window_still_carries_the_server_file_ids() {
+        let catalog = with_images(20);
+        let page = read_cells(&catalog, 5, 4).unwrap();
+        assert_eq!(page.len(), 4);
+        assert!(
+            page.iter().all(|c| c.file_id.is_some()),
+            "a cell lost its file id when the join was split out"
+        );
+    }
+
     /// A catalog with `n` images, ready to file into collections.
     fn with_images(n: usize) -> Catalog {
         let catalog = Catalog::in_memory().unwrap();

ui/dr-ui/src/library_ui.rs

@@ -61,6 +61,30 @@ const MIN_WINDOW: usize = 24;
 const MIN_CELL_SIZE: f32 = 90.0;
 const MAX_CELL_SIZE: f32 = 420.0;
 
+/// TRACES: NFR-P5
+/// What the grid's whole-library readouts are an answer about.
+///
+/// The timeline's bars, the filter chips' counts, the "on this device" count
+/// and whether the scoped collection is pinned all describe the *library*, not
+/// the window over it — and [`load_window`] recomputed every one of them each
+/// time the window moved, which is several times per screenful of scrolling.
+/// Together they are a `MIN`/`MAX`, a `GROUP BY`, two counts and, under a
+/// collection, three more queries: about 8 ms of SQLite on the thread that is
+/// trying to draw the frame, for four answers that scrolling cannot change.
+///
+/// So they are recomputed when this changes and not otherwise. `total` is in
+/// here as the change detector as much as anything else: it is already read on
+/// every load for the scrollbar, and a scan landing, a delete or a restore all
+/// move it. What it cannot see — a rating edited under an unchanged count — is
+/// covered because the paths that do that refresh the chips themselves.
+#[derive(Clone, Copy, PartialEq, Eq)]
+struct LibraryFacts {
+    scope: Option<dr_types::CollectionId>,
+    filter: library::RatingFilter,
+    trash: bool,
+    total: usize,
+}
+
 /// Library state for the running window.
 pub struct LibraryController {
     /// Shared with [`crate::collections_ui`], which edits collections against
@@ -77,6 +101,12 @@ pub struct LibraryController {
     /// Catalog row ids and whether each still needs its EXIF read.
     image_ids: RefCell<Vec<i64>>,
     needs_metadata: RefCell<Vec<bool>>,
+    /// When each row in the model was taken, parallel to it.
+    ///
+    /// Kept so the timeline marker can be moved from the window the grid has
+    /// already read rather than from a query per scroll event — see
+    /// [`capture_time_at`].
+    captured_at: RefCell<Vec<Option<i64>>>,
     /// The size class of the pixels each row is currently showing, `None` for a
     /// row still waiting.
     ///
@@ -109,6 +139,9 @@ pub struct LibraryController {
     /// window, wasteful on a narrow one. The grid measures itself and reports
     /// its screenful; this is [`SCREENFULS`] of them.
     window: RefCell<usize>,
+    /// What the whole-library readouts on screen were last computed for, so a
+    /// window that merely moved does not recompute them. See [`LibraryFacts`].
+    library_facts: std::cell::Cell<Option<LibraryFacts>>,
     /// How many cells the viewport shows at once, as the grid last reported.
     ///
     /// Kept beside `window` rather than divided back out of it, because
@@ -308,11 +341,13 @@ impl LibraryController {
             sizes: RefCell::new(Vec::new()),
             image_ids: RefCell::new(Vec::new()),
             needs_metadata: RefCell::new(Vec::new()),
+            captured_at: RefCell::new(Vec::new()),
             thumb_class: RefCell::new(Vec::new()),
             offset: RefCell::new(0),
             resume_at: std::cell::Cell::new(0),
             window: RefCell::new((INITIAL_VIEWPORT_CELLS * SCREENFULS).max(MIN_WINDOW)),
             viewport_cells: std::cell::Cell::new(INITIAL_VIEWPORT_CELLS),
+            library_facts: std::cell::Cell::new(None),
             requested: RefCell::new(Default::default()),
             scan_timer: RefCell::new(None),
             thumb_timer: RefCell::new(None),
@@ -2004,24 +2039,39 @@ fn load_window(window: &AppWindow, ctl: &Rc<LibraryController>) {
     // query rather than another predicate threaded through the scoped one.
     let trash = ctl.viewing_trash.get();
 
-    // TRACES: FR-NC-6a
-    // Whether the newly scoped collection is already pinned. Read here rather
-    // than remembered, because a pin outlives the session that made it — on
-    // reopening the library the button has to show what the catalog says, not
-    // what this run happens to have done.
-    window.set_library_scope_pinned(match scope {
-        Some(id) => dr_catalog::collections::descendants(catalog.connection(), id)
-            .map(|ids| collection_images(catalog, &ids))
-            .map(|images| scope_is_pinned(catalog, &images))
-            .unwrap_or(false),
-        None => false,
-    });
-
     let total = if trash {
         library::total_trashed(catalog).unwrap_or(0)
     } else {
         library::total_images_scoped(catalog, scope, &filter).unwrap_or(0)
     };
+
+    // What the whole-library readouts below describe. See [`LibraryFacts`].
+    let facts = LibraryFacts {
+        scope,
+        filter,
+        trash,
+        total,
+    };
+    let describes_something_new = ctl.library_facts.get() != Some(facts);
+    ctl.library_facts.set(Some(facts));
+
+    // TRACES: FR-NC-6a
+    // Whether the newly scoped collection is already pinned. Read here rather
+    // than remembered, because a pin outlives the session that made it — on
+    // reopening the library the button has to show what the catalog says, not
+    // what this run happens to have done.
+    //
+    // Three queries deep — descendants, their images, and whether every one of
+    // them is pinned — and the answer cannot change by scrolling.
+    if describes_something_new {
+        window.set_library_scope_pinned(match scope {
+            Some(id) => dr_catalog::collections::descendants(catalog.connection(), id)
+                .map(|ids| collection_images(catalog, &ids))
+                .map(|images| scope_is_pinned(catalog, &images))
+                .unwrap_or(false),
+            None => false,
+        });
+    }
     // Read before it is overwritten: the property still holds what the library
     // was last time this ran, and a library that has become shorter is the
     // signal that something was deleted out from under the view.
@@ -2036,8 +2086,17 @@ fn load_window(window: &AppWindow, ctl: &Rc<LibraryController>) {
     *ctl.offset.borrow_mut() = offset;
     window.set_library_offset(offset as i32);
 
-    // The date range this window covers, so the scrubber can label itself.
-    refresh_timeline(window, catalog, ctl);
+    // The axis the scrubber draws. A `MIN`/`MAX` and a `GROUP BY` over the
+    // whole scope — 5.7 ms on 24,000 images — and the bars do not change as the
+    // grid scrolls, only the marker on them does. The marker is moved by the
+    // scroll handler on every event, without coming through here.
+    //
+    // Every other thing that *does* change the bars — a zoom, a pan, a scrub,
+    // dates landing from the thumbnail worker — calls `refresh_timeline`
+    // itself, so this being skipped cannot leave a stale axis on screen.
+    if describes_something_new {
+        refresh_timeline(window, catalog, ctl);
+    }
 
     let cells = if trash {
         library::read_trashed_cells(catalog, offset, window_size)
@@ -2148,6 +2207,7 @@ fn load_window(window: &AppWindow, ctl: &Rc<LibraryController>) {
     *ctl.sizes.borrow_mut() = cells.iter().map(|c| c.size).collect();
     *ctl.image_ids.borrow_mut() = cells.iter().map(|c| c.image_id).collect();
     *ctl.needs_metadata.borrow_mut() = cells.iter().map(|c| c.metadata_state < 2).collect();
+    *ctl.captured_at.borrow_mut() = cells.iter().map(|c| c.captured_at).collect();
     *ctl.thumb_class.borrow_mut() = cells
         .iter()
         .map(|c| held.get(&c.image_id).and_then(|h| h.class))
@@ -2178,9 +2238,13 @@ fn load_window(window: &AppWindow, ctl: &Rc<LibraryController>) {
     if let Some(coll) = ctl.coll_ctl.borrow().as_ref().and_then(|w| w.upgrade()) {
         crate::collections_ui::sync_selection(window, &coll, &ids);
     }
-    // The filter chips' counts describe the whole library, not this window, so
-    // they are refreshed here rather than per cell.
-    refresh_rating_counts(window, catalog);
+    // The filter chips' counts describe the whole library, not this window —
+    // which is exactly why they are not recomputed for a window that moved.
+    // A judgement changes them and calls this itself (see `apply_judgement`),
+    // so a cull still watches its own chips move.
+    if describes_something_new {
+        refresh_rating_counts(window, catalog);
+    }
 
     // The library got shorter while the view was looking at it — a delete.
     //
@@ -3987,18 +4051,53 @@ fn catalog_span(catalog: &Catalog, ctl: &Rc<LibraryController>) -> Option<(i64,
     library::span_scoped(catalog, *ctl.scope.borrow(), &ctl.filter.borrow())
 }
 
+/// TRACES: NFR-P5
 /// Capture time of the image at row `ordinal` in the grid's own ordering.
 ///
-/// The inverse of the count in [`scrub_to`], and it must stay the inverse: the
-/// same `shadowed_by IS NULL` exclusion and the same ordering, or scrolling
-/// would report an instant the scrub would never produce for that row.
-///
 /// `None` for an ordinal that lands among the undated tail, which sorts last
 /// and has no place on a capture-time axis.
 ///
-/// Called on every scroll event, so it has to stay cheap: the `images_captured`
-/// index makes it a seek along already-ordered rows rather than a sort.
-fn capture_time_at(catalog: &Catalog, ordinal: usize) -> Option<i64> {
+/// # Answered from the loaded window, not from the catalog
+///
+/// This moves the timeline marker, so it is called on **every scroll event** —
+/// every row the view crosses, by design, or the marker would advance in jerks
+/// while the photographs beside it moved smoothly.
+///
+/// It used to be a query, and `LIMIT 1 OFFSET n` is not a seek: SQLite reaches
+/// row `n` by producing and discarding the `n` before it. So its cost grew with
+/// how far down the library the user had scrolled — 0.02 ms near the top, 0.7 ms
+/// at twenty thousand — and it was paid per row crossed, on the thread drawing
+/// the frame. A flick down a long library therefore got *choppier the further
+/// it went*, which is a strange enough symptom to be worth naming.
+///
+/// The window the grid has already read holds this answer. `window_move` keeps
+/// the whole view inside that window, so the lookup is a vector index at the
+/// position the ordinal has in it.
+///
+/// # The fallback, and why it is also more correct than what it replaces
+///
+/// Outside the loaded window — briefly, after a scrub or a keyboard jump, before
+/// the load lands — it falls back to the query. That query counts in a
+/// *dated-only* ordering while `ordinal` is a grid row, so the two disagree
+/// wherever undated images sit in between; it is kept because it is close
+/// enough for a marker that is about to be corrected, and because being wrong
+/// there is what it always did. The window path has no such disagreement: it
+/// reads the very cell the row belongs to.
+fn capture_time_at(ctl: &Rc<LibraryController>, catalog: &Catalog, ordinal: usize) -> Option<i64> {
+    {
+        let offset = *ctl.offset.borrow();
+        let loaded = ctl.captured_at.borrow();
+        if ordinal >= offset {
+            if let Some(when) = loaded.get(ordinal - offset) {
+                return *when;
+            }
+        }
+    }
+    capture_time_from_catalog(catalog, ordinal)
+}
+
+/// The fallback of [`capture_time_at`], for a row outside the loaded window.
+fn capture_time_from_catalog(catalog: &Catalog, ordinal: usize) -> Option<i64> {
     catalog
         .connection()
         .query_row(
@@ -4598,7 +4697,7 @@ pub fn wire<F>(
             {
                 let borrow = ctl.catalog.borrow();
                 if let Some(catalog) = borrow.as_ref() {
-                    if let Some(when) = capture_time_at(catalog, first_visible) {
+                    if let Some(when) = capture_time_at(&ctl, catalog, first_visible) {
                         *ctl.current_bucket.borrow_mut() = Some(when);
 
                         let zoom = *ctl.timeline_zoom.borrow();