diff --git a/apps/readest.koplugin/library/bim_patch.lua b/apps/readest.koplugin/library/bim_patch.lua index 4966e772..3954b071 100644 --- a/apps/readest.koplugin/library/bim_patch.lua +++ b/apps/readest.koplugin/library/bim_patch.lua @@ -92,7 +92,7 @@ local function patch_bim(opts) end local function build_group_info(filepath, do_cover_image) - local group_by, value, cache_key, shape = group_covers.parse_uri(filepath) + local group_by, value, shape = group_covers.parse_uri(filepath) local meta = cloud_covers.get_meta(filepath) local info = { has_meta = true, @@ -103,12 +103,12 @@ local function patch_bim(opts) has_cover = false, _no_provider = true, } - if do_cover_image and group_by and value and cache_key then + if do_cover_image and group_by and value then local LibraryWidget = package.loaded["library.librarywidget"] local store = LibraryWidget and LibraryWidget._store local settings = M._opts and M._opts.settings or {} local bb = group_covers.serve_or_compose( - group_by, value, cache_key, shape, + group_by, value, shape, store, settings, _orig_get_book_info, BIM) if bb then local w, h = bb:getWidth(), bb:getHeight() diff --git a/apps/readest.koplugin/library/cloud_covers.lua b/apps/readest.koplugin/library/cloud_covers.lua index 06cd2303..06ffd077 100644 --- a/apps/readest.koplugin/library/cloud_covers.lua +++ b/apps/readest.koplugin/library/cloud_covers.lua @@ -179,35 +179,22 @@ function M.trigger_download(hash) process_queue() end --- Set of hashes on the current Menu page; only these may trigger --- downloads. Pass nil to disable the filter (e.g. when the Library --- closes and the patched BIM might still be invoked from elsewhere). -function M.set_visible_hashes(menu, cloud_only_flag) - if not menu then - logger.dbg("ReadestLibrary set_visible_hashes: cleared (menu nil)") +-- Set of hashes whose covers may trigger downloads on the current Menu +-- page. Pass nil to disable the filter (e.g. when the Library closes and +-- the patched BIM might still be invoked from elsewhere). Caller is +-- responsible for computing the set: cloud-only book entries contribute +-- their own hash; group entries contribute their children's hashes (so +-- the patched BIM can fetch covers for the mosaic composite). +function M.set_visible_hashes(set) + if set == nil then + logger.dbg("ReadestLibrary set_visible_hashes: cleared") _visible_hashes = nil return end - local set = {} - local count = 0 - local page = menu.page or 1 - local perpage = menu.perpage or 1 - local items = menu.item_table or {} - local first = (page - 1) * perpage + 1 - local last = math.min(first + perpage - 1, #items) - for i = first, last do - local entry = items[i] - if entry and entry[cloud_only_flag] and type(entry.file) == "string" then - local hash = M.hash_from_uri(entry.file) - set[hash] = true - count = count + 1 - end - end _visible_hashes = set - logger.info("ReadestLibrary set_visible_hashes: page=" .. page - .. " range=" .. first .. ".." .. last - .. " cloud_only_visible=" .. count - .. " (item_table size=" .. #items .. ")") + local count = 0 + for _ in pairs(set) do count = count + 1 end + logger.info("ReadestLibrary set_visible_hashes: count=" .. count) end return M diff --git a/apps/readest.koplugin/library/group_covers.lua b/apps/readest.koplugin/library/group_covers.lua index 5c8bcb2f..34c46b83 100644 --- a/apps/readest.koplugin/library/group_covers.lua +++ b/apps/readest.koplugin/library/group_covers.lua @@ -1,24 +1,21 @@ -- group_covers.lua -- macOS-style folder previews: a 2x2 mosaic of the first N child book -- covers, served as a synthetic readest-group:// URI through the --- patched BookInfoManager. Composites are cached on disk under --- /readest_group_covers/.png --- with the cache key derived from the actual first-N hashes, so any --- change to the group's content (sort flip, add/remove, reordering --- bump) auto-invalidates the cached PNG. +-- patched BookInfoManager. Composites are recomposed in memory on every +-- paint — no on-disk cache. Earlier versions cached PNGs under +-- /readest_group_covers/, content-fingerprinted by child +-- hashes, but any partial composite written while children's covers +-- were still downloading would freeze: the fingerprint stayed the same +-- once all four arrived, so the partial PNG kept serving forever. +-- Recomposing each paint is cheap (4 small covers + scale + blit) and +-- side-steps that cache-coherency surface entirely. -local logger = require("logger") local cloud_covers = require("library.cloud_covers") local M = {} M.URI_PREFIX = "readest-group://" --- Bump when the composite layout/dimensions change so existing on-disk --- composites get regenerated on next paint instead of serving the old --- aspect ratio forever. -local CACHE_VERSION = 3 - -- Layouts: -- "grid" — 2x2, 360x480 (3:4 — typical book-cover aspect). -- "list" — 2x2, 480x480 (square — matches ListMenu's rigid square @@ -29,15 +26,6 @@ M.LAYOUTS = { list = { target_w = 480, target_h = 480, cols = 2, rows = 2 }, } -local function group_covers_dir() - local DataStorage = require("datastorage") - return DataStorage:getSettingsDir() .. "/readest_group_covers" -end - -local function group_cover_path(cache_key) - return group_covers_dir() .. "/" .. cache_key .. "_v" .. CACHE_VERSION .. ".png" -end - -- "Asimov" → "417369..." — filesystem-safe regardless of slashes, -- colons, etc. in the original group value. local function hex_encode(s) @@ -55,9 +43,7 @@ function M.build_uri(group_by, value, shape) .. ":" .. (shape or "grid") .. ".png" end --- Returns group_by, value, cache_key, shape; nil if not a group URI. --- cache_key here is the static "identity" portion; the BIM patch --- appends the actual first-N hashes for content-based invalidation. +-- Returns group_by, value, shape; nil if not a group URI. function M.parse_uri(uri) if uri:sub(1, #M.URI_PREFIX) ~= M.URI_PREFIX then return nil end local body = uri:sub(#M.URI_PREFIX + 1) @@ -69,7 +55,7 @@ function M.parse_uri(uri) local hex = parts[2] local shape = parts[3] or "grid" local value = hex_decode(hex) - return group_by, value, group_by .. "_" .. hex .. "_" .. shape, shape + return group_by, value, shape end -- Pull a usable cover bb for a single child book during composition. @@ -78,7 +64,16 @@ end -- no extraction triggered) -- 2. cloud cover .png we previously downloaded -- Returns nil if neither path produces one. Caller owns the bb. +-- +-- When (2) misses for a cloud-present book, queue a cloud-cover download +-- so a subsequent paint can complete the mosaic. Without this hook a +-- freshly-pulled library renders every group as FakeCover until the user +-- drills into each one, because cloud_covers' download queue is only +-- primed for cloud-only book entries on the visible page — group cells +-- never appear there themselves, so their children's covers were never +-- requested. function M.child_cover_bb(book, orig_getBookInfo, BIM) + if not book then return nil end if book.local_present == 1 and book.file_path and orig_getBookInfo then local ok, info = pcall(orig_getBookInfo, BIM, book.file_path, true) if ok and info and info.has_cover and info.cover_bb then @@ -94,18 +89,26 @@ function M.child_cover_bb(book, orig_getBookInfo, BIM) return copy end end - return cloud_covers.load_cover_bb(book.hash) + if not book.hash or book.hash == "" then return nil end + local bb = cloud_covers.load_cover_bb(book.hash) + if bb then return bb end + if (book.cloud_present or 0) == 1 then + cloud_covers.trigger_download(book.hash) + end + return nil end --- Compose up to N child covers into a mosaic and write the PNG to --- disk. Returns the loaded composite as a fresh bb, or nil on total --- failure (no usable child covers, or PNG write failure). -local function compose(books, dest_path, shape, orig_getBookInfo, BIM) +-- Compose up to N child covers into a mosaic, returning a fresh bb that +-- the caller (ImageWidget) takes ownership of. Returns nil if no child +-- produced a cover. Intentionally regenerated on every paint — see the +-- module-level comment. +local function compose(books, shape, orig_getBookInfo, BIM) if #books == 0 then return nil end local layout = M.LAYOUTS[shape] or M.LAYOUTS.grid local target_w, target_h = layout.target_w, layout.target_h local cols, rows = layout.cols, layout.rows local max_cells = cols * rows + local expected = math.min(max_cells, #books) local Blitbuffer = require("ffi/blitbuffer") local target = Blitbuffer.new(target_w, target_h, Blitbuffer.TYPE_BBRGB32) @@ -116,7 +119,7 @@ local function compose(books, dest_path, shape, orig_getBookInfo, BIM) local cell_h = math.floor((target_h - (rows - 1) * gap) / rows) local placed = 0 - for i = 1, math.min(max_cells, #books) do + for i = 1, expected do local book = books[i] local cover = M.child_cover_bb(book, orig_getBookInfo, BIM) if cover then @@ -138,24 +141,7 @@ local function compose(books, dest_path, shape, orig_getBookInfo, BIM) target:free() return nil end - - local lfs = require("libs/libkoreader-lfs") - local dir = group_covers_dir() - if lfs.attributes(dir, "mode") ~= "directory" then lfs.mkdir(dir) end - - local ok_write = pcall(target.writeToFile, target, dest_path, "PNG") - target:free() - if not ok_write then - logger.warn("ReadestLibrary group cover write failed: " .. tostring(dest_path)) - return nil - end - -- Re-load the file so the bb we hand back is one ImageWidget can - -- safely take ownership of. - local ok_render, RenderImage = pcall(require, "ui/renderimage") - if not ok_render then return nil end - local ok_load, bb = pcall(RenderImage.renderImageFile, RenderImage, dest_path, false) - if not ok_load then return nil end - return bb + return target end -- Cells-per-mosaic for a given shape. Used by callers to know how many @@ -165,11 +151,11 @@ function M.cells_for(shape) return layout.cols * layout.rows end --- High-level: query the store, derive a content-based cache key from --- the actual first-N hashes, load from disk if present, else compose --- fresh. Returns (bb, books) — books is the resolved list (so callers --- can reuse it without a second query). -function M.serve_or_compose(group_by, value, cache_key, shape, +-- High-level: query the store, compose a fresh mosaic, return (bb, +-- books). books is the resolved list (so callers can reuse it without a +-- second query). bb is freshly composed every call — see the module +-- header for why we deliberately don't cache. +function M.serve_or_compose(group_by, value, shape, store, settings, orig_getBookInfo, BIM) if not store then return nil, {} end local n = M.cells_for(shape) @@ -177,27 +163,7 @@ function M.serve_or_compose(group_by, value, cache_key, shape, sort_by = settings and settings.library_sort_by, sort_asc = settings and settings.library_sort_ascending == true, }) - -- Fingerprint: short prefix of each hash, joined. Stable for the - -- same set in the same order; changes the moment any of those - -- shifts. - local parts = {} - for i = 1, #books do - parts[i] = (books[i].hash or ""):sub(1, 12) - end - local fingerprint = (#parts > 0) and table.concat(parts, "-") or "empty" - local content_key = cache_key .. "_" .. fingerprint - local cache_path = group_cover_path(content_key) - - local lfs = require("libs/libkoreader-lfs") - if lfs.attributes(cache_path, "mode") == "file" then - local ok, RenderImage = pcall(require, "ui/renderimage") - if ok then - local ok2, loaded = pcall(RenderImage.renderImageFile, - RenderImage, cache_path, false) - if ok2 then return loaded, books end - end - end - return compose(books, cache_path, shape, orig_getBookInfo, BIM), books + return compose(books, shape, orig_getBookInfo, BIM), books end return M diff --git a/apps/readest.koplugin/library/libraryitem.lua b/apps/readest.koplugin/library/libraryitem.lua index c3dfe3a9..89d44092 100644 --- a/apps/readest.koplugin/library/libraryitem.lua +++ b/apps/readest.koplugin/library/libraryitem.lua @@ -13,6 +13,7 @@ -- way the rendering pipeline expects, then delegates lifecycle hooks -- (install, set_visible_hashes) to the right submodule. +local logger = require("logger") local cloud_covers = require("library.cloud_covers") local group_covers = require("library.group_covers") local bim_patch = require("library.bim_patch") @@ -34,9 +35,71 @@ function M.install(opts) bim_patch.install(opts) end --- Limit cloud-cover downloads to entries on the current Menu page. +-- Limit cloud-cover downloads to hashes the current page actually needs. +-- Two contributors: +-- * cloud-only book entries — their own hash (cover painted directly) +-- * group entries — the first-N children's hashes (cover painted as a +-- 2x2/1x4 mosaic in build_group_info; up to 4 child .png downloads +-- get queued the first time the group is on screen). +-- Without the group expansion, group cells render as FakeCover until the +-- user drills into each group individually, since trigger_download's +-- visibility filter would reject the children. function M.set_visible_hashes(menu) - cloud_covers.set_visible_hashes(menu, M.CLOUD_ONLY_FLAG) + if not menu then + cloud_covers.set_visible_hashes(nil) + return + end + + local LibraryWidget = package.loaded["library.librarywidget"] + local store = LibraryWidget and LibraryWidget._store + local settings = (bim_patch._opts and bim_patch._opts.settings) or {} + local view_mode = (settings.library_view_mode == "list") and "list" or "mosaic" + local shape = (view_mode == "list") and "list" or "grid" + local n_cells = group_covers.cells_for(shape) + local sort_opts = { + sort_by = settings.library_sort_by, + sort_asc = settings.library_sort_ascending == true, + } + + local set = {} + local page = menu.page or 1 + local perpage = menu.perpage or 1 + local items = menu.item_table or {} + local first = (page - 1) * perpage + 1 + local last = math.min(first + perpage - 1, #items) + local cloud_only_count, group_child_count = 0, 0 + for i = first, last do + local entry = items[i] + if entry then + if entry[M.CLOUD_ONLY_FLAG] and type(entry.file) == "string" then + local hash = cloud_covers.hash_from_uri(entry.file) + if hash and hash ~= "" then + set[hash] = true + cloud_only_count = cloud_only_count + 1 + end + elseif entry._readest_group and store then + local group = entry._readest_group + local group_by = group._group_by + if group_by then + local children = store:listBooksInGroup( + group_by, group.name, n_cells, sort_opts) + for _j, c in ipairs(children) do + if c.hash and c.hash ~= "" then + set[c.hash] = true + group_child_count = group_child_count + 1 + end + end + end + end + end + end + + cloud_covers.set_visible_hashes(set) + logger.info("ReadestLibrary set_visible_hashes: page=" .. page + .. " range=" .. first .. ".." .. last + .. " cloud_only=" .. cloud_only_count + .. " group_children=" .. group_child_count + .. " (item_table size=" .. #items .. ")") end -- --------------------------------------------------------------------------- diff --git a/apps/readest.koplugin/spec/library/group_covers_spec.lua b/apps/readest.koplugin/spec/library/group_covers_spec.lua new file mode 100644 index 00000000..88e794d4 --- /dev/null +++ b/apps/readest.koplugin/spec/library/group_covers_spec.lua @@ -0,0 +1,296 @@ +-- group_covers_spec.lua +-- Pure-function tests for library/group_covers.lua. The Blitbuffer-driven +-- compose() pipeline calls into KOReader's FFI graphics stack and isn't +-- unit-testable here, so it's exercised by the manual matrix. These specs +-- lock down: +-- * URI builder/parser round-trip (incl. nested group paths with "/") +-- * child_cover_bb's cover-missing branch — must queue a cloud-cover +-- download for cloud-present children so a subsequent paint can +-- compose the mosaic. Without this hook, a freshly-pulled library +-- renders every group as FakeCover until the user drills into each +-- group individually (see https://github.com/readest/readest/… for +-- the bug report). + +require("spec_helper") +local lfs = require("lfs") + +-- Minimal real-lfs shim for production code that imports +-- "libs/libkoreader-lfs". The KOReader binding extends LuaFileSystem with +-- a few extras we don't need here. +package.preload["libs/libkoreader-lfs"] = function() return lfs end + +-- Stub renderimage so cloud_covers.load_cover_bb can be exercised without +-- pulling in the full KOReader graphics stack. We always return nil +-- (= "couldn't decode") so the trigger-download branch is reachable. +package.preload["ui/renderimage"] = function() + return { + renderImageFile = function() return nil end, + } +end + +describe("library.group_covers", function() + local group_covers, cloud_covers + + before_each(function() + require("spec_helper").reset() + package.loaded["library.cloud_covers"] = nil + package.loaded["library.group_covers"] = nil + cloud_covers = require("library.cloud_covers") + group_covers = require("library.group_covers") + end) + + -- ===================================================================== + -- build_uri / parse_uri round-trip + -- ===================================================================== + describe("build_uri / parse_uri", function() + it("round-trips a simple group name", function() + local uri = group_covers.build_uri("group_name", "Fantasy", "grid") + local g, v, s = group_covers.parse_uri(uri) + assert.are.equal("group_name", g) + assert.are.equal("Fantasy", v) + assert.are.equal("grid", s) + end) + + it("preserves slashes inside nested group paths", function() + local uri = group_covers.build_uri("group_name", "Selected/Very", "grid") + local g, v, s = group_covers.parse_uri(uri) + assert.are.equal("group_name", g) + assert.are.equal("Selected/Very", v) + assert.are.equal("grid", s) + end) + + it("defaults to grid shape when none is given", function() + local uri = group_covers.build_uri("author", "Asimov") + local _, _, s = group_covers.parse_uri(uri) + assert.are.equal("grid", s) + end) + + it("returns nil for non-group URIs", function() + assert.is_nil(group_covers.parse_uri("/local/path/file.epub")) + assert.is_nil(group_covers.parse_uri("readest-cloud://abc.epub")) + end) + end) + + -- ===================================================================== + -- cells_for: layout cell count + -- ===================================================================== + describe("cells_for", function() + it("returns 4 for both grid and list shapes", function() + assert.are.equal(4, group_covers.cells_for("grid")) + assert.are.equal(4, group_covers.cells_for("list")) + end) + + it("falls back to grid (4) for unknown shapes", function() + assert.are.equal(4, group_covers.cells_for("nonsense")) + end) + end) + + -- ===================================================================== + -- child_cover_bb: cloud-cover trigger when cover .png is missing + -- ===================================================================== + describe("child_cover_bb missing-cover behaviour", function() + local trigger_calls + + before_each(function() + -- Auth must be present for trigger_download to even consider + -- the call; otherwise it bails early (intentional safety net + -- in production). + cloud_covers.set_opts({ sync_auth = "fake-token" }) + + -- Record trigger calls and short-circuit so we never actually + -- hit the network or try to dequeue. + trigger_calls = {} + cloud_covers.trigger_download = function(hash, opts) + trigger_calls[#trigger_calls + 1] = { hash = hash, opts = opts } + end + end) + + it("queues a cloud-cover download for a cloud-only child whose .png isn't on disk", function() + local cover = group_covers.child_cover_bb( + { hash = "abc12345", cloud_present = 1, local_present = 0 }, + nil, nil) + assert.is_nil(cover) + assert.are.equal(1, #trigger_calls) + assert.are.equal("abc12345", trigger_calls[1].hash) + end) + + it("queues a download for a hybrid (cloud + local) book when its cover isn't extracted yet", function() + -- BIM hasn't seen this file yet — orig_getBookInfo returns nil. + local orig_getBookInfo = function() return nil end + local cover = group_covers.child_cover_bb( + { + hash = "hybrid01", + cloud_present = 1, + local_present = 1, + file_path = "/no/such/file.epub", + }, + orig_getBookInfo, {}) + assert.is_nil(cover) + assert.are.equal(1, #trigger_calls) + assert.are.equal("hybrid01", trigger_calls[1].hash) + end) + + it("does not queue a download for a local-only child (nothing to fetch)", function() + local orig_getBookInfo = function() return nil end + local cover = group_covers.child_cover_bb( + { + hash = "local001", + cloud_present = 0, + local_present = 1, + file_path = "/no/such/file.epub", + }, + orig_getBookInfo, {}) + assert.is_nil(cover) + assert.are.equal(0, #trigger_calls) + end) + + it("does not queue a download when the child has no hash (defensive)", function() + local cover = group_covers.child_cover_bb( + { cloud_present = 1, local_present = 0 }, nil, nil) + assert.is_nil(cover) + assert.are.equal(0, #trigger_calls) + end) + end) +end) + +-- ===================================================================== +-- libraryitem.set_visible_hashes — group children expansion +-- ===================================================================== +-- The patched BIM only triggers child cover downloads when the child's +-- hash is in cloud_covers' visibility set. Without expanding visible +-- groups → first-N children, trigger_download in child_cover_bb is a +-- no-op and the mosaic never composes. These tests pin that contract. +describe("library.libraryitem.set_visible_hashes", function() + local libraryitem, cloud_covers + local fake_widget + + before_each(function() + require("spec_helper").reset() + package.loaded["library.cloud_covers"] = nil + package.loaded["library.group_covers"] = nil + package.loaded["library.bim_patch"] = nil + package.loaded["library.cloud_icons"] = nil + package.loaded["library.list_strip"] = nil + package.loaded["library.libraryitem"] = nil + package.loaded["library.librarywidget"] = nil + + -- Fake LibraryWidget so libraryitem can pick up the store. + -- libraryitem reads package.loaded directly (not require), so a + -- preload won't suffice — populate the loaded slot itself. + fake_widget = { _store = nil } + package.loaded["library.librarywidget"] = fake_widget + + cloud_covers = require("library.cloud_covers") + libraryitem = require("library.libraryitem") + end) + + after_each(function() + package.loaded["library.librarywidget"] = nil + end) + + -- Capture whatever set libraryitem hands cloud_covers, regardless of + -- whether trigger_download itself is exercised. + local function capture_set() + local captured + cloud_covers.set_visible_hashes = function(set) captured = set end + return function() return captured end + end + + -- Minimal store stub: returns the books we tell it to, keyed by + -- (group_by, name). listBooksInGroup is the only method the wrapper + -- calls. + local function fake_store(by_group) + return { + listBooksInGroup = function(_self, group_by, name, _n, _opts) + local key = group_by .. ":" .. name + return by_group[key] or {} + end, + } + end + + it("clears the visibility set when called with nil menu", function() + local get = capture_set() + libraryitem.set_visible_hashes(nil) + assert.is_nil(get()) + end) + + it("includes hashes for cloud-only book entries on the visible page", function() + local get = capture_set() + local menu = { + page = 1, perpage = 3, + item_table = { + { [libraryitem.CLOUD_ONLY_FLAG] = true, file = "readest-cloud://aaa.epub" }, + { [libraryitem.CLOUD_ONLY_FLAG] = true, file = "readest-cloud://bbb.epub" }, + -- a local book — should NOT contribute + { is_file = true, file = "/local/book.epub" }, + }, + } + libraryitem.set_visible_hashes(menu) + local set = get() + assert.is_truthy(set["aaa"]) + assert.is_truthy(set["bbb"]) + local n = 0 + for _ in pairs(set) do n = n + 1 end + assert.are.equal(2, n) + end) + + it("expands visible group entries to include their first-N children's hashes", function() + local get = capture_set() + fake_widget._store = fake_store({ + ["group_name:Selected/Very"] = { + { hash = "child01" }, { hash = "child02" }, + { hash = "child03" }, { hash = "child04" }, + }, + ["group_name:Selected/Philosophy"] = { + { hash = "phil01" }, { hash = "phil02" }, + }, + }) + local menu = { + page = 1, perpage = 3, + item_table = { + { + _readest_group = { + name = "Selected/Very", + _group_by = "group_name", + }, + }, + { + _readest_group = { + name = "Selected/Philosophy", + _group_by = "group_name", + }, + }, + -- A book entry on the same page + { [libraryitem.CLOUD_ONLY_FLAG] = true, file = "readest-cloud://book01.epub" }, + }, + } + libraryitem.set_visible_hashes(menu) + local set = get() + -- Group children + assert.is_truthy(set["child01"]) + assert.is_truthy(set["child04"]) + assert.is_truthy(set["phil01"]) + assert.is_truthy(set["phil02"]) + -- Cloud-only book on the same page + assert.is_truthy(set["book01"]) + end) + + it("only walks the current page's window, not the full item_table", function() + local get = capture_set() + fake_widget._store = fake_store({ + ["group_name:OnPage1"] = { { hash = "child01" } }, + ["group_name:OnPage2"] = { { hash = "child99" } }, + }) + local menu = { + page = 2, perpage = 1, + item_table = { + { _readest_group = { name = "OnPage1", _group_by = "group_name" } }, + { _readest_group = { name = "OnPage2", _group_by = "group_name" } }, + }, + } + libraryitem.set_visible_hashes(menu) + local set = get() + assert.is_nil(set["child01"]) + assert.is_truthy(set["child99"]) + end) +end)