diff --git a/ui/dr-ui/src/library_ui.rs b/ui/dr-ui/src/library_ui.rs index 9320495..8821fac 100644 --- a/ui/dr-ui/src/library_ui.rs +++ b/ui/dr-ui/src/library_ui.rs @@ -1663,6 +1663,61 @@ fn already_served( .collect() } +/// Where the loaded window should move to for a view at `first_visible`, or +/// `None` to leave it where it is. +/// +/// # Why this is a rule and not four lines in the scroll handler +/// +/// It decides how often the grid re-reads the catalog while a finger is on it, +/// and both of its ways of being wrong are invisible in the code and obvious +/// on a tablet: too eager and every scroll stutters, too lazy and the view +/// runs off the end of the loaded rows into blank ones. +/// +/// # The rule +/// +/// The window is centred on the view — a quarter of it behind, so scrolling +/// back has loaded rows to move into — and clamped to the last position where +/// it is still full. It moves only once the view comes within a quarter-window +/// of an edge of what is loaded, so a drag reloads a few times per screenful +/// rather than on every row. +/// +/// And it never moves to where it already is. That is the case the margin test +/// alone gets wrong: at either end of a scope the window is *pinned* — the +/// first screenful cannot be centred further back than zero, and the last +/// cannot start past `max_offset` — so the margin is unsatisfiable there and +/// every row crossed in the first or last quarter of a window re-read the +/// catalog, rebuilt the model and issued a thumbnail batch to arrive at the +/// offset it already had. On a library of twenty-odd thousand that is a +/// stutter at the top and the bottom of every collection, which is exactly +/// where a cull begins and ends. +fn window_move( + first_visible: usize, + current: usize, + window_size: usize, + total: usize, +) -> Option { + // The same clamp `load_window` applies, repeated here so this compares + // against where the window would come to rest rather than where it was + // asked to go. + let max_offset = total.saturating_sub(window_size.min(total)); + let desired = first_visible + .saturating_sub(window_size / 4) + .min(max_offset); + + if desired == current { + return None; + } + + let margin = window_size / 4; + let inside = + first_visible >= current + margin && first_visible + margin < current + window_size; + if inside { + return None; + } + + Some(desired) +} + /// Fill the model from the catalog and start fetching thumbnails. /// /// Reads the window starting at the controller's current offset, which the @@ -3392,7 +3447,34 @@ pub fn wire( if !w.get_show_library() { return; } + + // **The viewport has to be re-anchored, not just the window.** + // + // Cells are drawn at their absolute place in the library, so + // the row a photograph sits on is `index / columns` — and this + // callback is the news that `columns` just changed. Every cell + // therefore moved. The Flickable's `viewport-y` did not: the + // view was left pointing at a row that now holds entirely + // different photographs, thousands of images from the ones the + // loaded window covers. The grid goes blank, and stays blank + // until a scroll reports a first-visible row and drags the + // window back under the view. + // + // That is the "the gallery randomly goes blank and a scroll + // fixes it" report. Nothing about its triggers is rare: a + // resize, the collections sidebar opening, a zoom step, or + // turning the tablet over. + // + // `resume_at` is the last first-visible ordinal reported, so it + // names the photograph the user was looking at. Sending it back + // through `scroll-to` puts that same photograph at the top of + // the view, at whatever row it now occupies. + let anchor = ctl.resume_at.get(); + let window_size = *ctl.window.borrow(); + *ctl.offset.borrow_mut() = anchor.saturating_sub(window_size / 4); load_window(&w, &ctl); + w.set_library_scroll_to(anchor as i32); + w.set_library_scroll_token(w.get_library_scroll_token() + 1); } }); } @@ -3495,21 +3577,16 @@ pub fn wire( // Centre the window on the view, so scrolling either way has // loaded rows ahead of it rather than only below. - let window_size = *ctl.window.borrow(); - let desired = first_visible.saturating_sub(window_size / 4); - let current = *ctl.offset.borrow(); - - // Reload only once the view nears an edge of what is loaded. - // Reacting to every scroll event would re-query and re-fetch - // continuously during a drag; this fires a few times per screenful. - let margin = window_size / 4; - let inside = - first_visible >= current + margin && first_visible + margin < current + window_size; - if inside { + let Some(offset) = window_move( + first_visible, + *ctl.offset.borrow(), + *ctl.window.borrow(), + w.get_library_total().max(0) as usize, + ) else { return; - } + }; - *ctl.offset.borrow_mut() = desired; + *ctl.offset.borrow_mut() = offset; load_window(&w, &ctl); }); } @@ -4324,6 +4401,74 @@ mod tests { assert_eq!(img.size().height, 2); } + // --- moving the loaded window ------------------------------------------ + // + // 360 images loaded out of 24,000, centred on the view a quarter back. + + const W: usize = 360; + const TOTAL: usize = 24_000; + + #[test] + fn a_view_well_inside_the_loaded_window_does_not_move_it() { + // The whole point of loading a screenful either side: scrolling within + // it must not touch the catalog. + assert_eq!(window_move(5_000, 4_910, W, TOTAL), None); + assert_eq!(window_move(5_100, 4_910, W, TOTAL), None); + } + + #[test] + fn a_view_reaching_the_edge_of_the_loaded_window_moves_it() { + // Close enough to the bottom of what is loaded that scrolling on would + // run into rows nobody has read. + let moved = window_move(5_200, 4_910, W, TOTAL).expect("the window follows the view"); + assert_eq!(moved, 5_200 - W / 4, "centred a quarter behind the view"); + } + + #[test] + fn the_top_of_the_library_is_not_reloaded_on_every_row() { + // `first_visible` cannot be centred further back than zero, so the + // margin test can never be satisfied here. Before this rule every one + // of these re-read the catalog to arrive at the offset it already had, + // which is the stutter at the top of every scope. + for first_visible in [0, 6, 30, 89] { + assert_eq!( + window_move(first_visible, 0, W, TOTAL), + None, + "row {first_visible} asked for a move to offset 0, which is where it is" + ); + } + } + + #[test] + fn the_end_of_the_library_is_not_reloaded_on_every_row() { + // The mirror of the above, and the worse of the two: the window is + // clamped to `max_offset` while the view keeps travelling past it. + let pinned = TOTAL - W; + for first_visible in [TOTAL - W / 2, TOTAL - 30, TOTAL - 1] { + assert_eq!( + window_move(first_visible, pinned, W, TOTAL), + None, + "row {first_visible} asked for a move to the offset it already had" + ); + } + } + + #[test] + fn the_window_never_starts_past_the_last_full_screenful() { + // Otherwise a scrub to the very end loads a handful of cells and the + // rest of the window addresses images that do not exist. + let moved = window_move(TOTAL - 1, 0, W, TOTAL).expect("a scrub to the end moves"); + assert_eq!(moved, TOTAL - W); + } + + #[test] + fn a_library_smaller_than_the_window_stays_at_the_beginning() { + // `max_offset` is zero, so there is one valid position and the view + // must never ask for another. + assert_eq!(window_move(0, 0, W, 40), None); + assert_eq!(window_move(39, 0, W, 40), None); + } + // --- carrying thumbnails across a reload ------------------------------- // // The black flash: `load_window` rebuilds every row, and until these it