fix(koplugin): render group cover previews in Library (#4064)

Group cells in the Readest Library view rendered as FakeCover even when
their child books had perfectly good cloud covers — the queue that
fetches <hash>.png covers was only primed for cloud-only book entries
on the visible page, so children of group entries were never requested.
A later partial composite (3/4 covers) was also written to a
content-fingerprinted disk cache and kept serving forever, since the
fingerprint stayed the same after the 4th cover landed.

Two fixes wired together:
- libraryitem.set_visible_hashes now expands visible group entries to
  include their first-N children's hashes, so trigger_download's
  visibility filter no longer rejects them.
- group_covers.child_cover_bb queues a cloud-cover download when the
  fallback path misses for a cloud-present book. Capped at 4 per group
  by the existing cells_for(shape) limit.

Disk caching of composites is dropped entirely; mosaics are recomposed
in memory each paint. New spec/library/group_covers_spec.lua locks the
contract for URI round-trip, child_cover_bb's missing-cover branches,
and libraryitem's group-children expansion.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Huang Xin
2026-05-05 23:01:35 +08:00
committed by GitHub
parent d66fedcab7
commit 15c0a7a2f2
5 changed files with 416 additions and 104 deletions
+3 -3
View File
@@ -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()
+12 -25
View File
@@ -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
+40 -74
View File
@@ -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
-- <settings>/readest_group_covers/<key>.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
-- <settings>/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
+65 -2
View File
@@ -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
-- ---------------------------------------------------------------------------
@@ -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)