From d8c54149e200dd522947df084db73d16049b26be Mon Sep 17 00:00:00 2001 From: Kageetai Date: Mon, 5 Oct 2026 13:24:50 +0200 Subject: [PATCH 1/2] Add Reader cover images as KOReader custom covers --- .github/workflows/ci.yml | 3 + README.md | 1 + readwisereader.koplugin/main.lua | 23 +- .../readwisereader/covers.lua | 89 +++++++ tests/covers_test.lua | 240 ++++++++++++++++++ tests/highlights_test.lua | 2 +- 6 files changed, 355 insertions(+), 3 deletions(-) create mode 100644 readwisereader.koplugin/readwisereader/covers.lua create mode 100644 tests/covers_test.lua diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4eea7b9..84bf492 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,3 +39,6 @@ jobs: - name: Highlight export regression tests run: luajit tests/highlights_test.lua + + - name: Reader cover regression tests + run: luajit tests/covers_test.lua diff --git a/README.md b/README.md index 62c6b48..2dbdb53 100644 --- a/README.md +++ b/README.md @@ -4,6 +4,7 @@ A plugin for KOReader integration with the highlight saving and read later servi ## Key features: - Articles in Readwise Reader saved to “Inbox”, “Later” or “Shortlist” are downloaded to KOReader as HTML files. - Images are downloaded where this is possible. +- Reader cover images are downloaded as KOReader custom covers when image downloads are enabled. Sync also adds missing covers to previously downloaded articles without rewriting their HTML. Existing custom covers, including manually chosen ones, are preserved. Missing, unsupported or unavailable images leave the cover unchanged. - Articles which have been read in KOReader and marked as “finished” will be moved to the Readwise Reader Archive at the next sync, and deleted from KOReader. - At sync, articles which have been archived in Readwise Reader will be deleted from KOReader. - Particular types of article, locations and document tags can be excluded from syncing in the settings menu. diff --git a/readwisereader.koplugin/main.lua b/readwisereader.koplugin/main.lua index a24124c..5c18229 100755 --- a/readwisereader.koplugin/main.lua +++ b/readwisereader.koplugin/main.lua @@ -25,6 +25,7 @@ local logger = require("logger") -- file, so plugin modules must be required here, never lazily inside a function. local ReadwiseAPI = require("readwisereader/api") local HighlightExporter = require("readwisereader/highlights") +local ReaderCovers = require("readwisereader/covers") local BD = require("ui/bidi") local DataStorage = require("datastorage") @@ -1543,13 +1544,19 @@ function ReadwiseReader:documentExists(doc_id) return self:findLocalDocumentByReadwiseId(doc_id) ~= nil end +function ReadwiseReader:syncDocumentCover(filepath, document) + return ReaderCovers:sync(filepath, document.image_url, self.download_images, self.max_image_size_mb) +end + function ReadwiseReader:downloadDocument(document) if self:shouldSkipDocument(document) then logger.dbg("ReadwiseReader:downloadDocument: skipping", document.id, "- has excluded tags or location") return "skipped" end - if self:documentExists(document.id) then + local existing_filepath = self:findLocalDocumentByReadwiseId(document.id) + if existing_filepath then + self:syncDocumentCover(existing_filepath, document) logger.dbg("ReadwiseReader:downloadDocument: skipping", document.id, "- already exists") return "skipped" end @@ -1607,6 +1614,8 @@ function ReadwiseReader:downloadDocument(document) logger.warn("ReadwiseReader:downloadDocument: metadata writing failed:", err) end + self:syncDocumentCover(filepath, document) + -- Update KOReader collections local coll_status, coll_err = pcall(function() self:updateDocumentCollections(filepath, document) end) if not coll_status then @@ -2161,16 +2170,22 @@ function ReadwiseReader:synchronize() local filtered_documents = {} local existing_count = 0 + local covers_downloaded = 0 for _, document in ipairs(documents) do if self:documentExists(document.id) then existing_count = existing_count + 1 + local filepath = self:findLocalDocumentByReadwiseId(document.id) + self:showProgress(string.format("Checking cover: %s", document.title or "Untitled")) + if filepath and self:syncDocumentCover(filepath, document) then + covers_downloaded = covers_downloaded + 1 + end else table.insert(filtered_documents, document) end end - if #filtered_documents == 0 and cleaned_count == 0 and archived_count == 0 and highlights_exported == 0 and reconciled_count == 0 then + if #filtered_documents == 0 and covers_downloaded == 0 and cleaned_count == 0 and archived_count == 0 and highlights_exported == 0 and reconciled_count == 0 then local msg = "No new articles found and no changes to process." if existing_count > 0 then msg = msg .. "\n" .. string.format("Skipped %d existing articles.", existing_count) @@ -2231,6 +2246,10 @@ function ReadwiseReader:synchronize() msg = msg .. "\n" .. string.format("Downloaded: %d", downloaded) end + if covers_downloaded > 0 then + msg = msg .. "\n" .. string.format("Covers downloaded: %d", covers_downloaded) + end + if existing_count > 0 then msg = msg .. "\n" .. string.format("Skipped (already exists): %d", existing_count) end diff --git a/readwisereader.koplugin/readwisereader/covers.lua b/readwisereader.koplugin/readwisereader/covers.lua new file mode 100644 index 0000000..e9c5961 --- /dev/null +++ b/readwisereader.koplugin/readwisereader/covers.lua @@ -0,0 +1,89 @@ +-- Reader covers are KOReader custom covers, separate from the article HTML. +local DataStorage = require("datastorage") +local DocSettings = require("docsettings") +local Event = require("ui/event") +local RenderImage = require("ui/renderimage") +local UIManager = require("ui/uimanager") +local http = require("socket.http") +local logger = require("logger") +local socketutil = require("socketutil") + +local Covers = {} + +-- URLs and Content-Type headers may not describe the actual image format. +local function imageSuffix(data) + if data:sub(1, 8) == "\137PNG\r\n\26\n" then return "png" end + if data:sub(1, 3) == "\255\216\255" then return "jpg" end + if data:sub(1, 6) == "GIF87a" or data:sub(1, 6) == "GIF89a" then return "gif" end + if data:sub(1, 4) == "RIFF" and data:sub(9, 12) == "WEBP" then return "webp" end +end + +-- Best effort: never prevent an article from syncing because its cover failed. +-- Existing custom covers are left alone, including manually chosen ones. +function Covers:sync(filepath, url, enabled, max_size_mb) + if not enabled or type(url) ~= "string" or not url:match("^https?://") then return false end + local temporary + local ok, saved = pcall(function() + if DocSettings:findCustomCoverFile(filepath) then return false end + local chunks, size = {}, 0 + local limit = max_size_mb * 1024 * 1024 + local request = { + url = url, + method = "GET", + headers = { + ["User-Agent"] = "Mozilla/5.0 (compatible; KOReader Readwise Plugin)", + ["Accept"] = "image/*", + }, + sink = function(chunk) + if chunk then + size = size + #chunk + if size > limit then return nil, "cover exceeds size limit" end + chunks[#chunks + 1] = chunk + end + return 1 + end, + } + socketutil:set_timeout(8, 45) + local transport_ok, result, code = pcall(http.request, request) + socketutil:reset_timeout() + if not transport_ok or not result or code ~= 200 or size == 0 or size > limit then return false end + local data = table.concat(chunks) + local suffix = imageSuffix(data) + if not suffix then return false end + -- Reject corrupt images before making them the document's custom cover. + local image = RenderImage:renderImageData(data, #data, false, 600, 800) + if not image then return false end + image:free() + + -- Use KOReader's writable settings directory: /tmp is unavailable on + -- some devices. Sync runs sequentially, so only one cover uses this file. + temporary = DataStorage:getSettingsDir() .. "/readwisereader-cover.tmp." .. suffix + local file = io.open(temporary, "wb") + if not file then return false end + local written = file:write(data) + local closed = file:close() + if not written or not closed then return false end + if not DocSettings:flushCustomCover(filepath, temporary) then return false end + -- KOReader's copy helper may miss a write failure on full storage. + -- Remove only the cover we just created, allowing a later sync to retry. + local installed = DocSettings:findCustomCoverFile(filepath) + local cover = installed and io.open(installed, "rb") + local installed_data + if cover then + installed_data = cover:read(#data + 1) + cover:close() + end + if installed_data ~= data then + if installed then os.remove(installed) end + return false + end + UIManager:broadcastEvent(Event:new("InvalidateMetadataCache", filepath)) + UIManager:broadcastEvent(Event:new("BookMetadataChanged")) + return true + end) + if temporary then os.remove(temporary) end + if not ok then logger.warn("ReadwiseReader: cover download failed:", saved) end + return ok and saved or false +end + +return Covers diff --git a/tests/covers_test.lua b/tests/covers_test.lua new file mode 100644 index 0000000..2497c9f --- /dev/null +++ b/tests/covers_test.lua @@ -0,0 +1,240 @@ +-- Run from the repo root: luajit tests/covers_test.lua +-- Network and KOReader are external boundaries; temporary cover files are real. +local function eq(actual, expected) + assert(actual == expected, string.format("expected %s, got %s", tostring(expected), tostring(actual))) +end +local png = "\137PNG\r\n\26\nfixture" +local jpeg = "\255\216\255fixture" +local gif = "GIF89afixture" +local webp = "RIFFxxxxWEBPfixture" +local calls, reply, installed, existing, timeout, freed, events, temporary, saved_path +local function noop() end +package.preload.datastorage = function() return { + getSettingsDir = function() return os.getenv("TMPDIR") or "/tmp" end, +} end +package.preload.logger = function() return { warn = noop, dbg = noop } end +package.preload.docsettings = function() return { + findCustomCoverFile = function() + if existing then return existing end + if saved_path then + local f = io.open(saved_path, "rb") + if f then f:close(); return saved_path end + end + end, + flushCustomCover = function(_, path, file) + local f = assert(io.open(file, "rb")) + installed = { path = path, bytes = f:read("*a"), suffix = file:match("%.([^.]*)$") } + f:close() + temporary = file + if reply.save_ok == false then return false end + saved_path = os.tmpname() + f = assert(io.open(saved_path, "wb")) + f:write(reply.partial_copy and installed.bytes:sub(1, 8) or installed.bytes) + f:close() + return true + end, +} end +package.preload["socket.http"] = function() return { request = function(request) + calls = calls + 1 + eq(request.url, "https://example.com/cover?size=large") + eq(request.headers.Authorization, nil) + if reply.throw then error("offline") end + for _, chunk in ipairs(reply.chunks or { reply.body or png }) do + local ok = request.sink(chunk) + if not ok then return nil, "sink rejected body" end + end + request.sink(nil) + return 1, reply.code or 200, { ["content-type"] = reply.mime or "application/octet-stream" } +end } end +package.preload.socketutil = function() return { + set_timeout = function() timeout = timeout + 1 end, + reset_timeout = function() timeout = timeout - 1 end, +} end +package.preload["ui/renderimage"] = function() return { + renderImageData = function(_, data, size, frames, width, height) + eq(size, #data) + eq(frames, false) + assert(width and height, "validation should bound rendered dimensions") + if data ~= png and data ~= jpeg and data ~= gif and data ~= webp then return nil end + return { free = function() freed = freed + 1 end } + end, +} end +package.preload["ui/event"] = function() return { + new = function(_, name, path) return { name = name, path = path } end, +} end +package.preload["ui/uimanager"] = function() return { + broadcastEvent = function(_, event) events[#events + 1] = event end, +} end +local Covers = dofile("readwisereader.koplugin/readwisereader/covers.lua") +local passed = 0 +local function test(name, run) + calls, timeout, freed = 0, 0, 0 + reply, events = {}, {} + installed, existing, temporary, saved_path = nil, nil, nil, nil + local ok, err = pcall(run) + if not ok then error(name .. ": " .. tostring(err)) end + eq(timeout, 0) + if temporary then eq(io.open(temporary, "rb"), nil) end + if saved_path then os.remove(saved_path) end + passed = passed + 1 + print("ok - " .. name) +end +local function sync(url, enabled, limit) + return Covers:sync("/reader/article.html", url or "https://example.com/cover?size=large", enabled ~= false, limit or 10) +end + +test("Reader image becomes a custom cover and invalidates cached metadata", function() + assert(sync()) + eq(installed.path, "/reader/article.html") + eq(installed.bytes, png) + eq(installed.suffix, "png") + eq(freed, 1) + eq(events[1].name, "InvalidateMetadataCache") + eq(events[1].path, "/reader/article.html") + eq(events[2].name, "BookMetadataChanged") +end) +test("image bytes determine the extension even with misleading headers", function() + reply = { body = jpeg, mime = "image/png" } + assert(sync()) + eq(installed.suffix, "jpg") +end) +test("GIF and WebP covers use extensions KOReader can open", function() + for _, fixture in ipairs({ { data = gif, suffix = "gif" }, { data = webp, suffix = "webp" } }) do + if saved_path then os.remove(saved_path) end + reply = { body = fixture.data } + assert(sync()) + eq(installed.suffix, fixture.suffix) + end +end) +test("manual and previously downloaded custom covers are preserved", function() + existing = "/reader/article.sdr/cover.jpg" + eq(sync(), false) + eq(calls, 0) + eq(installed, nil) +end) +test("disabled image downloads and missing or invalid URLs do not fetch", function() + eq(sync(nil, false), false) + for _, url in ipairs({ "", "file:///secret", "data:image/png;base64,abc", {}, false }) do + eq(Covers:sync("/reader/article.html", url, true, 10), false) + end + eq(Covers:sync("/reader/article.html", nil, true, 10), false) + eq(calls, 0) +end) +test("empty, HTTP error and corrupt image responses leave the cover unset", function() + for _, fixture in ipairs({ { body = "" }, { code = 404 }, { body = "error" }, { body = "\137PNG\r\n\26\ncorrupt" } }) do + reply = fixture + eq(sync(), false) + eq(installed, nil) + end + eq(#events, 0) +end) +test("oversized bodies are stopped while receiving rather than saved", function() + reply = { chunks = { png, string.rep("x", 1024 * 1024) } } + eq(sync(nil, true, 1), false) + eq(installed, nil) + eq(freed, 0) +end) +test("transport exceptions reset timeouts and allow a later retry", function() + reply = { throw = true } + eq(sync(), false) + eq(timeout, 0) + reply = {} + assert(sync()) +end) +test("failed installation cleans temporary files and does not invalidate caches", function() + reply = { save_ok = false } + eq(sync(), false) + eq(#events, 0) +end) +test("partial sidecar copies are removed so the next sync can retry", function() + reply = { partial_copy = true } + eq(sync(), false) + eq(io.open(saved_path, "rb"), nil) + eq(#events, 0) + reply = {} + assert(sync()) + eq(calls, 2) +end) +test("a downloaded cover persists and is not fetched again", function() + assert(sync()) + local f = assert(io.open(saved_path, "rb")) + eq(f:read("*a"), png) + f:close() + eq(sync(), false) + eq(calls, 1) +end) + +-- Load the actual plugin with lightweight KOReader UI boundaries. +local object = {} +function object:extend(value) return setmetatable(value or {}, { __index = self }) end +function object:new(value) return self:extend(value) end +local empty_modules = { + "ui/bidi", "dispatcher", "ffi/util", "apps/filemanager/filemanager", + "ui/widget/inputdialog", "luasettings", "ui/widget/confirmbox", "ui/widget/multiconfirmbox", + "readcollection", "clip", "ui/network/manager", "readhistory", "device", + "apps/filemanager/filemanagerutil", "libs/libkoreader-lfs", "ltn12", "mime", "socket", + "json", "rapidjson", +} +for _, name in ipairs(empty_modules) do package.preload[name] = function() return {} end end +package.preload["ui/widget/container/widgetcontainer"] = function() return object end +package.preload["ui/widget/infomessage"] = function() return object end +package.preload.gettext = function() return function(s) return s end end +package.preload.util = function() return { getSafeFilename = function(title) return title end } end +local ui = require("ui/uimanager") +local messages = {} +ui.show = function(_, info) messages[#messages + 1] = info.text end +ui.close = noop +package.path = "readwisereader.koplugin/?.lua;" .. package.path +local Reader = dofile("readwisereader.koplugin/main.lua") +local function readerFor(document, filepath) + return Reader:new{ + download_images = true, max_image_size_mb = 10, + findLocalDocumentByReadwiseId = function() return filepath end, + shouldSkipDocument = function() return false end, + validateSettings = function() return true end, + api = { resetRateLimit = noop }, + cleanupArchivedDocuments = function() return 0 end, + reconcileLocalDocuments = function() return 0 end, + getDocumentList = function() return { document } end, + updateAvailableTags = noop, showProgress = noop, hideProgress = noop, + saveSettings = noop, initCollectionTracking = noop, saveCollections = noop, + storeAuthorMetadata = noop, storeSourceUrlMetadata = noop, + setDocumentMetadata = noop, updateDocumentCollections = noop, + } +end +local document = { id = "doc-1", title = "Article", image_url = "https://example.com/cover?size=large", html_content = "Article body" } +test("sync fills an existing article's missing cover without rewriting its HTML", function() + local file = os.tmpname() + local f = assert(io.open(file, "wb")); f:write("existing annotations depend on this HTML"); f:close() + local reader = readerFor(document, file) + reader:synchronize() + eq(installed and installed.path, file) + f = assert(io.open(file, "rb")); eq(f:read("*a"), "existing annotations depend on this HTML"); f:close() + os.remove(file) + assert(messages[#messages]:find("Covers downloaded: 1", 1, true), messages[#messages]) +end) +test("new article downloads save HTML and a Reader cover", function() + local dir = os.tmpname() + os.remove(dir) + -- Use a unique filename prefix rather than creating a directory. + local reader = readerFor(document) + reader.directory = dir + reader.processHtmlContent = function(_, content) return content end + eq(reader:downloadDocument(document), "downloaded") + local path = dir .. "[rw-id_doc-1] Article.html" + eq(installed and installed.path, path) + local f = assert(io.open(path, "rb")); eq(f:read("*a"), "Article body"); f:close() + os.remove(path) +end) +test("cover errors do not fail an article download", function() + local dir = os.tmpname() + os.remove(dir) + local reader = readerFor(document) + reader.directory = dir + reader.processHtmlContent = function(_, content) return content end + reply = { throw = true } + eq(reader:downloadDocument(document), "downloaded") + eq(installed, nil) + os.remove(dir .. "[rw-id_doc-1] Article.html") +end) +print(string.format("%d cover tests passed", passed)) diff --git a/tests/highlights_test.lua b/tests/highlights_test.lua index d3aca0d..dfc5aea 100644 --- a/tests/highlights_test.lua +++ b/tests/highlights_test.lua @@ -104,7 +104,7 @@ local unused = { "ui/bidi", "docsettings", "ui/event", "ffi/util", "apps/filemanager/filemanager", "ui/widget/inputdialog", "ui/widget/confirmbox", "ui/widget/multiconfirmbox", "readcollection", "ui/network/manager", "readhistory", "apps/filemanager/filemanagerutil", - "libs/libkoreader-lfs", "mime", + "libs/libkoreader-lfs", "mime", "ui/renderimage", } for _, name in ipairs(unused) do modules[name] = {} end for name, module in pairs(modules) do From 2fd7996378c2c9db826c22280bbdfb760977cb20 Mon Sep 17 00:00:00 2001 From: Zhenbo Li Date: Mon, 12 Oct 2026 19:01:00 -0400 Subject: [PATCH 2/2] Make Reader covers opt-in, limited and attempted once per article - Covers are off by default: "Download article covers" in Settings/Image Download, independent of article image downloads. - New articles get their cover at download. Articles already on the device wait in covers_pending and get theirs within 30 seconds per sync; an article open in the reader is skipped until a later sync. - One attempt per article. Only temporary failures (no response, 408, 429, 5xx, local write failure) are kept, in covers_failed, for "Retry failed covers". Successes and permanent failures store nothing, so a cover the user removed stays removed. Both lists are pruned to the articles on the device. - After three network errors in a row, a sync stops fetching covers. - Covers over 1 MB are logged and over 2 MB stopped while downloading. Images under 200 px on a side or over 4 million pixels are refused from their header, before decoding. - Requests accept only formats KOReader renders, use KOReader's timeout sink so the 15 s total applies, and every failure is logged with its reason. - One directory scan per sync instead of two per existing article, and only InvalidateMetadataCache is broadcast per cover. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VTrfmhAJ2zK6Z875JKAhpm --- README.md | 2 +- readwisereader.koplugin/main.lua | 194 +++++++- .../readwisereader/covers.lua | 220 +++++++-- tests/covers_test.lua | 450 ++++++++++++++---- tests/highlights_test.lua | 1 + 5 files changed, 694 insertions(+), 173 deletions(-) diff --git a/README.md b/README.md index ae35c24..59ae0e0 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A plugin for KOReader integration with the highlight saving and read later servi ## Key features: - Articles in Readwise Reader saved to “Inbox”, “Later” or “Shortlist” are downloaded to KOReader as HTML files. - Images are downloaded where this is possible. -- Reader cover images are downloaded as KOReader custom covers when image downloads are enabled. Sync also adds missing covers to previously downloaded articles without rewriting their HTML. Existing custom covers, including manually chosen ones, are preserved. Missing, unsupported or unavailable images leave the cover unchanged. +- Optionally, each article's Reader image becomes its KOReader cover (off by default - enable "Download article covers" in Settings/Image Download). New articles get their cover when downloaded; articles already on the device get theirs over the next few syncs, for about 30 seconds per sync. The article HTML is not rewritten. Existing custom covers, including manually chosen ones, are kept, and a cover you remove is not downloaded again. Images over 2MB, under 200 pixels on a side or over 4 million pixels are skipped. Failed downloads are not retried automatically: use "Retry failed covers" in the same menu. - Articles which have been read in KOReader and marked as “finished” will be moved to the Readwise Reader Archive at the next sync, and deleted from KOReader. - At sync, articles which have been archived in Readwise Reader will be deleted from KOReader. - Particular types of article, locations and document tags can be excluded from syncing in the settings menu. diff --git a/readwisereader.koplugin/main.lua b/readwisereader.koplugin/main.lua index 64eb6aa..a451020 100755 --- a/readwisereader.koplugin/main.lua +++ b/readwisereader.koplugin/main.lua @@ -7,6 +7,7 @@ -- MAIN FEATURES: -- - Downloads articles from Readwise Reader "later" and "shortlist" locations -- - Converts articles to HTML format with embedded images +-- - Optionally uses each article's Reader image as its KOReader cover -- - Generates KOReader metadata sidecars (.sdr) for enhanced library integration -- - Automatically manages KOReader collections based on article location -- - Offers filtering by article tags, location, type and site name (via series) @@ -120,6 +121,15 @@ function ReadwiseReader:init() self.download_images = settings.download_images == nil and true or settings.download_images self.max_image_size_mb = settings.max_image_size_mb or 10 + -- Reader images as KOReader custom covers, off by default. + self.download_covers = settings.download_covers or false + -- Ids of articles whose cover was never tried, and of those that failed for a + -- reason that may pass. Successes and permanent failures are not recorded, so + -- a cover the user removed stays removed. covers_pending is nil until the first + -- sync with covers on fills it with the articles already on the device. + self.covers_pending = settings.covers_pending + self.covers_failed = settings.covers_failed or {} + -- Initialize max articles download limit self.max_articles_to_download = settings.max_articles_to_download or 0 -- 0 = unlimited @@ -706,6 +716,35 @@ function ReadwiseReader:addToMainMenu(menu_items) self:showImageSizeDialog() end, }, + { + text = "Download article covers", + help_text = "Use each article's Reader image as its cover. Articles already on this device get theirs over the next few syncs.", + checked_func = function() + return self.download_covers + end, + callback = function() + self.download_covers = not self.download_covers + -- Start over either way: turning covers on gives every + -- article without one an attempt, and off stores nothing. + self.covers_pending = nil + self.covers_failed = {} + self:saveSettings() + end, + }, + { + text_func = function() + return string.format("Retry failed covers (%d)", util.tableSize(self.covers_failed)) + end, + enabled_func = function() + return self.download_covers and next(self.covers_failed) ~= nil + end, + callback = function() + local count = self:retryFailedCovers() + UIManager:show(InfoMessage:new{ + text = string.format("%d cover(s) will be retried at the next sync.", count), + }) + end, + }, } }, { @@ -1553,19 +1592,122 @@ function ReadwiseReader:documentExists(doc_id) return self:findLocalDocumentByReadwiseId(doc_id) ~= nil end -function ReadwiseReader:syncDocumentCover(filepath, document) - return ReaderCovers:sync(filepath, document.image_url, self.download_images, self.max_image_size_mb) +function ReadwiseReader:localDocumentPaths() + local paths = {} + self:forEachLocalDocument(function(doc_id, filepath) + paths[doc_id] = filepath + end) + return paths +end + +-- =============================================================================== +-- READER COVERS +-- =============================================================================== +-- New articles try their cover once, at download. Articles that were already on +-- the device when covers were turned on wait in covers_pending and get theirs +-- within a time budget per sync. Nothing is retried automatically. + +-- Seconds per sync for covers of earlier articles. One download may run over it. +local COVER_BUDGET_SECONDS = 30 +-- Network errors in a row after which this sync stops downloading covers. +local COVER_OFFLINE_AFTER = 3 + +-- Per-sync cover state, or nil when covers are off. +function ReadwiseReader:startCoverSync(documents, local_paths) + if not self.download_covers then + return nil + end + if not self.covers_pending then + -- First sync with covers on: every article already here gets one attempt. + self.covers_pending = {} + for _, document in ipairs(documents) do + local filepath = local_paths[document.id] + if filepath and ReaderCovers.isFetchable(document.image_url) + and not DocSettings:findCustomCoverFile(filepath) then + self.covers_pending[document.id] = true + end + end + end + -- Forget articles that are no longer on the device. + for _, ids in ipairs({ self.covers_pending, self.covers_failed }) do + for doc_id in pairs(ids) do + if not local_paths[doc_id] then + ids[doc_id] = nil + end + end + end + return { added = 0, failed = 0, network_errors = 0 } +end + +-- One attempt. Only failures that may pass are remembered, for the manual retry. +function ReadwiseReader:downloadCover(filepath, document, cover_run) + if cover_run.network_errors >= COVER_OFFLINE_AFTER then + -- Not attempted, so it waits for a later sync. + self.covers_pending[document.id] = true + return + end + self.covers_pending[document.id] = nil + local status = ReaderCovers:sync(filepath, document.image_url) + if status == "offline" then + cover_run.network_errors = cover_run.network_errors + 1 + elseif status ~= "exists" then + cover_run.network_errors = 0 + end + if status == "installed" then + cover_run.added = cover_run.added + 1 + elseif status == "temporary" or status == "offline" then + self.covers_failed[document.id] = true + cover_run.failed = cover_run.failed + 1 + end +end + +-- Earlier articles, in document list order. An open article is skipped and keeps +-- its place, so the reader's cached settings never miss a new cover. +function ReadwiseReader:fillPendingCovers(documents, local_paths, cover_run) + local started = os.time() + for _, document in ipairs(documents) do + if cover_run.network_errors >= COVER_OFFLINE_AFTER + or os.time() - started >= COVER_BUDGET_SECONDS then + break + end + local filepath = local_paths[document.id] + if self.covers_pending[document.id] and filepath and not self:isOpenDocument(filepath) then + if ReaderCovers.isFetchable(document.image_url) then + self:showProgress(string.format("Adding cover: %s", document.title or "Untitled")) + self:downloadCover(filepath, document, cover_run) + else + self.covers_pending[document.id] = nil + end + end + end +end + +function ReadwiseReader:isOpenDocument(filepath) + return self.ui and self.ui.document and self.ui.document.file == filepath or false end -function ReadwiseReader:downloadDocument(document) +function ReadwiseReader:retryFailedCovers() + local count = 0 + for doc_id in pairs(self.covers_failed) do + -- Without a pending list, the next sync builds one that includes these. + if self.covers_pending then + self.covers_pending[doc_id] = true + end + count = count + 1 + end + self.covers_failed = {} + self:saveSettings() + return count +end + +-- cover_run is the sync's cover state, nil when covers are off. +function ReadwiseReader:downloadDocument(document, cover_run) if self:shouldSkipDocument(document) then logger.dbg("ReadwiseReader:downloadDocument: skipping", document.id, "- has excluded tags or location") return "skipped" end - - local existing_filepath = self:findLocalDocumentByReadwiseId(document.id) - if existing_filepath then - self:syncDocumentCover(existing_filepath, document) + + if self:documentExists(document.id) then logger.dbg("ReadwiseReader:downloadDocument: skipping", document.id, "- already exists") return "skipped" end @@ -1623,7 +1765,9 @@ function ReadwiseReader:downloadDocument(document) logger.warn("ReadwiseReader:downloadDocument: metadata writing failed:", err) end - self:syncDocumentCover(filepath, document) + if cover_run and ReaderCovers.isFetchable(document.image_url) then + self:downloadCover(filepath, document, cover_run) + end -- Update KOReader collections local coll_status, coll_err = pcall(function() self:updateDocumentCollections(filepath, document) end) @@ -2177,24 +2321,24 @@ function ReadwiseReader:synchronize() local reconciled_count = keep_local and 0 or self:reconcileLocalDocuments(documents) self:hideProgress() + -- One directory scan, after reconciliation has removed files. + local local_paths = self:localDocumentPaths() + local cover_run = self:startCoverSync(documents, local_paths) + local filtered_documents = {} local existing_count = 0 - local covers_downloaded = 0 + local covers_pending = false for _, document in ipairs(documents) do - if self:documentExists(document.id) then + if local_paths[document.id] then existing_count = existing_count + 1 - local filepath = self:findLocalDocumentByReadwiseId(document.id) - self:showProgress(string.format("Checking cover: %s", document.title or "Untitled")) - if filepath and self:syncDocumentCover(filepath, document) then - covers_downloaded = covers_downloaded + 1 - end + covers_pending = covers_pending or (cover_run and self.covers_pending[document.id]) or false else table.insert(filtered_documents, document) end end - if #filtered_documents == 0 and covers_downloaded == 0 and cleaned_count == 0 and archived_count == 0 and highlights_exported == 0 and reconciled_count == 0 then + if #filtered_documents == 0 and not covers_pending and cleaned_count == 0 and archived_count == 0 and highlights_exported == 0 and reconciled_count == 0 then local msg = "No new articles found and no changes to process." if existing_count > 0 then msg = msg .. "\n" .. string.format("Skipped %d existing articles.", existing_count) @@ -2219,7 +2363,7 @@ function ReadwiseReader:synchronize() for i, document in ipairs(filtered_documents) do self:showProgress(string.format("Downloading %d of %d: %s", i, #filtered_documents, document.title)) - local result = self:downloadDocument(document) + local result = self:downloadDocument(document, cover_run) if result == "downloaded" then downloaded = downloaded + 1 @@ -2238,6 +2382,10 @@ function ReadwiseReader:synchronize() -- Final save for any remaining collection changes self:saveCollections() + if cover_run then + self:fillPendingCovers(documents, local_paths, cover_run) + end + self:hideProgress() if not keep_local then @@ -2255,8 +2403,12 @@ function ReadwiseReader:synchronize() msg = msg .. "\n" .. string.format("Downloaded: %d", downloaded) end - if covers_downloaded > 0 then - msg = msg .. "\n" .. string.format("Covers downloaded: %d", covers_downloaded) + if cover_run and cover_run.added > 0 then + msg = msg .. "\n" .. string.format("Covers added: %d", cover_run.added) + end + + if cover_run and cover_run.failed > 0 then + msg = msg .. "\n" .. string.format("Covers failed: %d (retry in Image Download settings)", cover_run.failed) end if existing_count > 0 then @@ -2321,6 +2473,10 @@ function ReadwiseReader:saveSettings() reader_highlights = self.reader_highlights, download_images = self.download_images, max_image_size_mb = self.max_image_size_mb, + download_covers = self.download_covers, + -- An empty pending list is kept: it means earlier articles are done. + covers_pending = self.covers_pending, + covers_failed = next(self.covers_failed or {}) and self.covers_failed or nil, max_articles_to_download = self.max_articles_to_download, sync_only_koreader_tag = self.sync_only_koreader_tag, } diff --git a/readwisereader.koplugin/readwisereader/covers.lua b/readwisereader.koplugin/readwisereader/covers.lua index e9c5961..fd075f6 100644 --- a/readwisereader.koplugin/readwisereader/covers.lua +++ b/readwisereader.koplugin/readwisereader/covers.lua @@ -1,14 +1,40 @@ -- Reader covers are KOReader custom covers, separate from the article HTML. +-- Covers:sync makes one attempt and reports how it went; the caller decides +-- what to remember, so this module keeps no state between calls. local DataStorage = require("datastorage") local DocSettings = require("docsettings") local Event = require("ui/event") local RenderImage = require("ui/renderimage") local UIManager = require("ui/uimanager") local http = require("socket.http") +local lfs = require("libs/libkoreader-lfs") local logger = require("logger") local socketutil = require("socketutil") -local Covers = {} +local Covers = { + -- Covers above WARN_BYTES are installed but logged; above MAX_BYTES the + -- download is stopped. Covers are shown at 600x800, so neither is needed. + WARN_BYTES = 1024 * 1024, + MAX_BYTES = 2 * 1024 * 1024, + -- A smaller image makes a worse cover than KOReader's generated one, and a + -- larger one is decoded at full size before scaling, which can exhaust memory. + MIN_SIDE = 200, + MAX_PIXELS = 4 * 1000 * 1000, + -- Seconds: for each network wait, and for the whole download. + BLOCK_TIMEOUT = 5, + TOTAL_TIMEOUT = 15, +} + +-- Covers:sync results: +-- "installed" the cover was downloaded and set +-- "exists" the article already has a custom cover, possibly chosen by hand +-- "permanent" retrying cannot help: HTTP error, unsupported or rejected image +-- "temporary" the server or this device may recover: 408, 429, 5xx, local write failure +-- "offline" no HTTP response at all: network error or timeout + +function Covers.isFetchable(url) + return type(url) == "string" and url:match("^https?://") ~= nil +end -- URLs and Content-Type headers may not describe the actual image format. local function imageSuffix(data) @@ -18,72 +44,166 @@ local function imageSuffix(data) if data:sub(1, 4) == "RIFF" and data:sub(9, 12) == "WEBP" then return "webp" end end +-- Unsigned integer of `count` bytes at `pos`, or nil past the end of data. +local function uint(data, pos, count, little_endian) + if #data < pos + count - 1 then return nil end + local value = 0 + for k = 0, count - 1 do + value = value * 256 + data:byte(little_endian and pos + count - 1 - k or pos + k) + end + return value +end + +local function jpegSize(data) + local pos = 3 -- after the start-of-image marker + while pos + 3 <= #data do + if data:byte(pos) ~= 0xFF then return nil end + local marker = data:byte(pos + 1) + if marker == 0xFF then + pos = pos + 1 -- fill byte + elseif marker == 0x01 or (marker >= 0xD0 and marker <= 0xD8) then + pos = pos + 2 -- markers without a length + elseif marker == 0xD9 or marker == 0xDA then + return nil -- end of image or image data before any frame header + elseif marker >= 0xC0 and marker <= 0xCF and marker ~= 0xC4 and marker ~= 0xC8 and marker ~= 0xCC then + -- start of frame: length, precision, height, width + return uint(data, pos + 7, 2), uint(data, pos + 5, 2) + else + local length = uint(data, pos + 2, 2) + if not length then return nil end + pos = pos + 2 + length + end + end +end + +-- Width and height from the image header, without decoding the image. +local function imageSize(data, suffix) + if suffix == "png" then + if data:sub(13, 16) ~= "IHDR" then return nil end + return uint(data, 17, 4), uint(data, 21, 4) + elseif suffix == "gif" then + return uint(data, 7, 2, true), uint(data, 9, 2, true) + elseif suffix == "jpg" then + return jpegSize(data) + end + local chunk = data:sub(13, 16) + if chunk == "VP8X" then + local width, height = uint(data, 25, 3, true), uint(data, 28, 3, true) + return width and width + 1, height and height + 1 + elseif chunk == "VP8 " then + if data:sub(24, 26) ~= "\157\1\42" then return nil end + local width, height = uint(data, 27, 2, true), uint(data, 29, 2, true) + return width and width % 16384, height and height % 16384 + elseif chunk == "VP8L" then + local bits = data:byte(21) == 0x2F and uint(data, 22, 4, true) + if not bits then return nil end + return bits % 16384 + 1, math.floor(bits / 16384) % 16384 + 1 + end +end + +-- Returns the image bytes, or nil, a result and a reason. +local function download(url) + local size, too_large = 0, false + socketutil:set_timeout(Covers.BLOCK_TIMEOUT, Covers.TOTAL_TIMEOUT) + -- LuaSocket restarts its own timeout for every chunk; this sink enforces the + -- total timeout as data arrives. + local sink, chunks = socketutil.table_sink({}) + local ok, result, code = pcall(http.request, { + url = url, + method = "GET", + headers = { + ["User-Agent"] = "Mozilla/5.0 (compatible; KOReader Readwise Plugin)", + -- Formats KOReader renders. image/* lets some CDNs choose AVIF. + ["Accept"] = "image/webp,image/png,image/jpeg,image/gif", + }, + sink = function(chunk, err) + if chunk then + size = size + #chunk + if size > Covers.MAX_BYTES then + too_large = true + return nil, "cover exceeds size limit" + end + end + return sink(chunk, err) + end, + }) + socketutil:reset_timeout() + + if too_large then return nil, "permanent", "larger than 2 MB" end + if not ok or not result then return nil, "offline", tostring(ok and code or result) end + if code == 408 or code == 429 or (type(code) == "number" and code >= 500) then + return nil, "temporary", "HTTP " .. code + end + if code ~= 200 then return nil, "permanent", "HTTP " .. tostring(code) end + if size == 0 then return nil, "permanent", "empty response" end + if size > Covers.WARN_BYTES then + logger.warn("ReadwiseReader: large cover,", size, "bytes:", url) + end + return table.concat(chunks) +end + +-- Returns the file suffix of a usable image, or nil and a reason. +local function check(data) + local suffix = imageSuffix(data) + if not suffix then return nil, "not a PNG, JPEG, GIF or WebP image" end + local width, height = imageSize(data, suffix) + if not width or not height then return nil, "unreadable image size" end + if math.min(width, height) < Covers.MIN_SIDE then + return nil, string.format("image too small (%dx%d)", width, height) + end + if width * height > Covers.MAX_PIXELS then + return nil, string.format("image too large (%dx%d)", width, height) + end + -- Reject corrupt images before making them the document's custom cover. + local image = RenderImage:renderImageData(data, #data, false, 600, 800) + if not image then return nil, "image could not be decoded" end + image:free() + return suffix +end + -- Best effort: never prevent an article from syncing because its cover failed. -- Existing custom covers are left alone, including manually chosen ones. -function Covers:sync(filepath, url, enabled, max_size_mb) - if not enabled or type(url) ~= "string" or not url:match("^https?://") then return false end +function Covers:sync(filepath, url) + if not Covers.isFetchable(url) then return "permanent", "no image URL" end local temporary - local ok, saved = pcall(function() - if DocSettings:findCustomCoverFile(filepath) then return false end - local chunks, size = {}, 0 - local limit = max_size_mb * 1024 * 1024 - local request = { - url = url, - method = "GET", - headers = { - ["User-Agent"] = "Mozilla/5.0 (compatible; KOReader Readwise Plugin)", - ["Accept"] = "image/*", - }, - sink = function(chunk) - if chunk then - size = size + #chunk - if size > limit then return nil, "cover exceeds size limit" end - chunks[#chunks + 1] = chunk - end - return 1 - end, - } - socketutil:set_timeout(8, 45) - local transport_ok, result, code = pcall(http.request, request) - socketutil:reset_timeout() - if not transport_ok or not result or code ~= 200 or size == 0 or size > limit then return false end - local data = table.concat(chunks) - local suffix = imageSuffix(data) - if not suffix then return false end - -- Reject corrupt images before making them the document's custom cover. - local image = RenderImage:renderImageData(data, #data, false, 600, 800) - if not image then return false end - image:free() + local ok, result, reason = pcall(function() + if DocSettings:findCustomCoverFile(filepath) then return "exists" end + local data, status, why = download(url) + if not data then return status, why end + local suffix + suffix, why = check(data) + if not suffix then return "permanent", why end -- Use KOReader's writable settings directory: /tmp is unavailable on -- some devices. Sync runs sequentially, so only one cover uses this file. temporary = DataStorage:getSettingsDir() .. "/readwisereader-cover.tmp." .. suffix local file = io.open(temporary, "wb") - if not file then return false end + if not file then return "temporary", "cannot create temporary file" end local written = file:write(data) local closed = file:close() - if not written or not closed then return false end - if not DocSettings:flushCustomCover(filepath, temporary) then return false end + if not written or not closed then return "temporary", "cannot write temporary file" end + if not DocSettings:flushCustomCover(filepath, temporary) then + return "temporary", "cannot save cover" + end -- KOReader's copy helper may miss a write failure on full storage. - -- Remove only the cover we just created, allowing a later sync to retry. + -- Remove only the cover we just created, so a retry starts clean. local installed = DocSettings:findCustomCoverFile(filepath) - local cover = installed and io.open(installed, "rb") - local installed_data - if cover then - installed_data = cover:read(#data + 1) - cover:close() - end - if installed_data ~= data then + if not installed or lfs.attributes(installed, "size") ~= #data then if installed then os.remove(installed) end - return false + return "temporary", "incomplete cover copy" end + -- The file browser is refreshed once at the end of the sync. UIManager:broadcastEvent(Event:new("InvalidateMetadataCache", filepath)) - UIManager:broadcastEvent(Event:new("BookMetadataChanged")) - return true + return "installed" end) if temporary then os.remove(temporary) end - if not ok then logger.warn("ReadwiseReader: cover download failed:", saved) end - return ok and saved or false + if not ok then + result, reason = "temporary", tostring(result) + end + if result ~= "installed" and result ~= "exists" then + logger.warn("ReadwiseReader: cover not set (" .. result .. "):", reason, url) + end + return result, reason end return Covers diff --git a/tests/covers_test.lua b/tests/covers_test.lua index 2497c9f..f10c8a4 100644 --- a/tests/covers_test.lua +++ b/tests/covers_test.lua @@ -3,23 +3,57 @@ local function eq(actual, expected) assert(actual == expected, string.format("expected %s, got %s", tostring(expected), tostring(actual))) end -local png = "\137PNG\r\n\26\nfixture" -local jpeg = "\255\216\255fixture" -local gif = "GIF89afixture" -local webp = "RIFFxxxxWEBPfixture" -local calls, reply, installed, existing, timeout, freed, events, temporary, saved_path +local function be(n, bytes) + local out = {} + for i = bytes - 1, 0, -1 do out[#out + 1] = string.char(math.floor(n / 256 ^ i) % 256) end + return table.concat(out) +end +local function le(n, bytes) + local out = {} + for i = 0, bytes - 1 do out[#out + 1] = string.char(math.floor(n / 256 ^ i) % 256) end + return table.concat(out) +end +-- Image headers with real dimensions; the rest of each fixture is filler. +local function png(w, h) return "\137PNG\r\n\26\n" .. be(13, 4) .. "IHDR" .. be(w, 4) .. be(h, 4) .. "fixture" end +local function gif(w, h) return "GIF89a" .. le(w, 2) .. le(h, 2) .. "fixture" end +local function jpeg(w, h) + -- an APP0 segment to skip, then a baseline frame header + return "\255\216\255\224" .. be(16, 2) .. string.rep("\0", 14) + .. "\255\192" .. be(17, 2) .. "\8" .. be(h, 2) .. be(w, 2) .. "fixture" +end +local function webp(chunk, w, h) + local body + if chunk == "VP8X" then + body = "\0\0\0\0" .. le(w - 1, 3) .. le(h - 1, 3) + elseif chunk == "VP8 " then + body = "\0\0\0\157\1\42" .. le(w, 2) .. le(h, 2) + else + body = "\47" .. le((w - 1) + (h - 1) * 16384, 4) + end + return "RIFF" .. le(12 + #body, 4) .. "WEBP" .. chunk .. le(#body, 4) .. body .. "fixture" +end +local cover = png(600, 800) + +local calls, urls, request, reply, installed, existing, covers, timeout, timeouts, sinks +local rendered, freed, events, temporary, warnings local function noop() end package.preload.datastorage = function() return { getSettingsDir = function() return os.getenv("TMPDIR") or "/tmp" end, } end -package.preload.logger = function() return { warn = noop, dbg = noop } end +package.preload.logger = function() return { + warn = function(...) + local parts = {} + for i = 1, select("#", ...) do parts[i] = tostring(select(i, ...)) end + warnings[#warnings + 1] = table.concat(parts, " ") + end, + dbg = noop, err = noop, info = noop, +} end package.preload.docsettings = function() return { - findCustomCoverFile = function() + findCustomCoverFile = function(_, path) if existing then return existing end - if saved_path then - local f = io.open(saved_path, "rb") - if f then f:close(); return saved_path end - end + local saved = covers[path] + local f = saved and io.open(saved, "rb") + if f then f:close(); return saved end end, flushCustomCover = function(_, path, file) local f = assert(io.open(file, "rb")) @@ -27,35 +61,57 @@ package.preload.docsettings = function() return { f:close() temporary = file if reply.save_ok == false then return false end - saved_path = os.tmpname() - f = assert(io.open(saved_path, "wb")) + local saved = os.tmpname() + f = assert(io.open(saved, "wb")) f:write(reply.partial_copy and installed.bytes:sub(1, 8) or installed.bytes) f:close() + covers[path] = saved return true end, } end -package.preload["socket.http"] = function() return { request = function(request) +package.preload["libs/libkoreader-lfs"] = function() return { + attributes = function(path, name) + local f = io.open(path, "rb") + if not f then return nil end + local size = f:seek("end") + f:close() + return name == "size" and size or { size = size } + end, +} end +package.preload["socket.http"] = function() return { request = function(req) calls = calls + 1 - eq(request.url, "https://example.com/cover?size=large") - eq(request.headers.Authorization, nil) + request = req + urls[#urls + 1] = req.url if reply.throw then error("offline") end - for _, chunk in ipairs(reply.chunks or { reply.body or png }) do - local ok = request.sink(chunk) - if not ok then return nil, "sink rejected body" end + if reply.network_error then return nil, reply.network_error end + for _, chunk in ipairs(reply.chunks or { reply.body or cover }) do + local ok, err = req.sink(chunk) + if not ok then return nil, err end end - request.sink(nil) + req.sink(nil) return 1, reply.code or 200, { ["content-type"] = reply.mime or "application/octet-stream" } end } end package.preload.socketutil = function() return { - set_timeout = function() timeout = timeout + 1 end, + set_timeout = function(_, block, total) + timeout = timeout + 1 + timeouts = { block, total } + end, reset_timeout = function() timeout = timeout - 1 end, + table_sink = function(t) + sinks = sinks + 1 + return function(chunk) + if chunk then t[#t + 1] = chunk end + return 1 + end, t + end, } end package.preload["ui/renderimage"] = function() return { renderImageData = function(_, data, size, frames, width, height) + rendered = rendered + 1 eq(size, #data) eq(frames, false) assert(width and height, "validation should bound rendered dimensions") - if data ~= png and data ~= jpeg and data ~= gif and data ~= webp then return nil end + if data:find("corrupt", 1, true) then return nil end return { free = function() freed = freed + 1 end } end, } end @@ -68,100 +124,137 @@ package.preload["ui/uimanager"] = function() return { local Covers = dofile("readwisereader.koplugin/readwisereader/covers.lua") local passed = 0 local function test(name, run) - calls, timeout, freed = 0, 0, 0 - reply, events = {}, {} - installed, existing, temporary, saved_path = nil, nil, nil, nil + calls, timeout, sinks, rendered, freed = 0, 0, 0, 0, 0 + reply, events, warnings, urls, covers = {}, {}, {}, {}, {} + installed, existing, temporary, request, timeouts = nil, nil, nil, nil, nil local ok, err = pcall(run) + for _, saved in pairs(covers) do os.remove(saved) end if not ok then error(name .. ": " .. tostring(err)) end eq(timeout, 0) if temporary then eq(io.open(temporary, "rb"), nil) end - if saved_path then os.remove(saved_path) end passed = passed + 1 print("ok - " .. name) end -local function sync(url, enabled, limit) - return Covers:sync("/reader/article.html", url or "https://example.com/cover?size=large", enabled ~= false, limit or 10) +local URL = "https://example.com/cover?size=large" +local function sync(url) + return Covers:sync("/reader/article.html", url or URL) +end +local function removeCover(path) + os.remove(covers[path]) + covers[path] = nil end -test("Reader image becomes a custom cover and invalidates cached metadata", function() - assert(sync()) +test("a Reader image becomes a custom cover and invalidates only that file's metadata", function() + eq(sync(), "installed") eq(installed.path, "/reader/article.html") - eq(installed.bytes, png) + eq(installed.bytes, cover) eq(installed.suffix, "png") eq(freed, 1) + eq(#events, 1) eq(events[1].name, "InvalidateMetadataCache") eq(events[1].path, "/reader/article.html") - eq(events[2].name, "BookMetadataChanged") -end) -test("image bytes determine the extension even with misleading headers", function() - reply = { body = jpeg, mime = "image/png" } - assert(sync()) - eq(installed.suffix, "jpg") -end) -test("GIF and WebP covers use extensions KOReader can open", function() - for _, fixture in ipairs({ { data = gif, suffix = "gif" }, { data = webp, suffix = "webp" } }) do - if saved_path then os.remove(saved_path) end - reply = { body = fixture.data } - assert(sync()) + eq(#warnings, 0) +end) +test("image bytes, not headers, determine the format, and each format's size is read", function() + local fixtures = { + { data = jpeg(600, 800), suffix = "jpg" }, + { data = gif(600, 800), suffix = "gif" }, + { data = webp("VP8X", 600, 800), suffix = "webp" }, + { data = webp("VP8 ", 600, 800), suffix = "webp" }, + { data = webp("VP8L", 600, 800), suffix = "webp" }, + } + for _, fixture in ipairs(fixtures) do + if covers["/reader/article.html"] then removeCover("/reader/article.html") end + reply = { body = fixture.data, mime = "image/png" } + eq(sync(), "installed") eq(installed.suffix, fixture.suffix) end end) -test("manual and previously downloaded custom covers are preserved", function() +test("requests ask for formats KOReader renders and use KOReader's timeout sink", function() + sync() + eq(request.url, URL) + eq(request.headers.Accept, "image/webp,image/png,image/jpeg,image/gif") + eq(request.headers.Authorization, nil) + eq(timeouts[1], 5) + eq(timeouts[2], 15) + eq(sinks, 1) +end) +test("existing custom covers, including manual ones, are kept without a request", function() existing = "/reader/article.sdr/cover.jpg" - eq(sync(), false) + eq(sync(), "exists") eq(calls, 0) eq(installed, nil) end) -test("disabled image downloads and missing or invalid URLs do not fetch", function() - eq(sync(nil, false), false) +test("missing or non-HTTP URLs are permanent and make no request", function() for _, url in ipairs({ "", "file:///secret", "data:image/png;base64,abc", {}, false }) do - eq(Covers:sync("/reader/article.html", url, true, 10), false) + eq(Covers:sync("/reader/article.html", url), "permanent") end - eq(Covers:sync("/reader/article.html", nil, true, 10), false) + eq(Covers:sync("/reader/article.html", nil), "permanent") eq(calls, 0) end) -test("empty, HTTP error and corrupt image responses leave the cover unset", function() - for _, fixture in ipairs({ { body = "" }, { code = 404 }, { body = "error" }, { body = "\137PNG\r\n\26\ncorrupt" } }) do +test("HTTP errors are permanent, except ones the server may recover from", function() + for _, code in ipairs({ 301, 403, 404, 410 }) do + reply = { code = code } + eq(sync(), "permanent") + end + for _, code in ipairs({ 408, 429, 500, 503 }) do + reply = { code = code } + eq(sync(), "temporary") + end + eq(installed, nil) +end) +test("no response is offline: exceptions, network errors and the total timeout", function() + for _, fixture in ipairs({ { throw = true }, { network_error = "timeout" }, { network_error = "sink timeout" } }) do reply = fixture - eq(sync(), false) - eq(installed, nil) + eq(sync(), "offline") end - eq(#events, 0) + eq(installed, nil) end) -test("oversized bodies are stopped while receiving rather than saved", function() - reply = { chunks = { png, string.rep("x", 1024 * 1024) } } - eq(sync(nil, true, 1), false) +test("empty, non-image, unreadable and corrupt responses are permanent", function() + local bodies = { "", "error", "\137PNG\r\n\26\n", "\255\216\255\217", png(600, 800) .. "corrupt" } + for _, body in ipairs(bodies) do + reply = { body = body } + eq(sync(), "permanent") + end eq(installed, nil) - eq(freed, 0) + eq(#events, 0) end) -test("transport exceptions reset timeouts and allow a later retry", function() - reply = { throw = true } - eq(sync(), false) - eq(timeout, 0) - reply = {} - assert(sync()) +test("images too small or with too many pixels are refused before decoding", function() + for _, body in ipairs({ png(199, 800), jpeg(800, 150), gif(32, 32), png(2001, 2000), webp("VP8X", 4000, 3000) }) do + reply = { body = body } + eq(sync(), "permanent") + end + eq(rendered, 0) + for _, body in ipairs({ png(200, 200), png(2000, 2000) }) do + if covers["/reader/article.html"] then removeCover("/reader/article.html") end + reply = { body = body } + eq(sync(), "installed") + end end) -test("failed installation cleans temporary files and does not invalidate caches", function() - reply = { save_ok = false } - eq(sync(), false) - eq(#events, 0) +test("covers over 2 MB are stopped while receiving; over 1 MB they are installed and logged", function() + reply = { chunks = { cover, string.rep("x", 2 * 1024 * 1024) } } + eq(sync(), "permanent") + eq(installed, nil) + eq(rendered, 0) + reply = { body = cover .. string.rep("x", 1536 * 1024) } + eq(sync(), "installed") + assert(warnings[#warnings]:find("large cover", 1, true), warnings[#warnings]) end) -test("partial sidecar copies are removed so the next sync can retry", function() +test("failed or incomplete installs are temporary, cleaned up, and invalidate nothing", function() + reply = { save_ok = false } + eq(sync(), "temporary") reply = { partial_copy = true } - eq(sync(), false) - eq(io.open(saved_path, "rb"), nil) + eq(sync(), "temporary") + eq(io.open(covers["/reader/article.html"], "rb"), nil) eq(#events, 0) reply = {} - assert(sync()) - eq(calls, 2) + eq(sync(), "installed") end) -test("a downloaded cover persists and is not fetched again", function() - assert(sync()) - local f = assert(io.open(saved_path, "rb")) - eq(f:read("*a"), png) - f:close() - eq(sync(), false) - eq(calls, 1) +test("every failure is logged with its reason and URL", function() + reply = { code = 404 } + sync() + eq(#warnings, 1) + assert(warnings[1]:find("HTTP 404", 1, true) and warnings[1]:find(URL, 1, true), warnings[1]) end) -- Load the actual plugin with lightweight KOReader UI boundaries. @@ -172,7 +265,7 @@ local empty_modules = { "ui/bidi", "dispatcher", "ffi/util", "apps/filemanager/filemanager", "ui/widget/inputdialog", "luasettings", "ui/widget/confirmbox", "ui/widget/multiconfirmbox", "readcollection", "clip", "ui/network/manager", "readhistory", "device", - "apps/filemanager/filemanagerutil", "libs/libkoreader-lfs", "ltn12", "mime", "socket", + "apps/filemanager/filemanagerutil", "ltn12", "mime", "socket", "json", "rapidjson", } for _, name in ipairs(empty_modules) do package.preload[name] = function() return {} end end @@ -186,55 +279,206 @@ ui.show = function(_, info) messages[#messages + 1] = info.text end ui.close = noop package.path = "readwisereader.koplugin/?.lua;" .. package.path local Reader = dofile("readwisereader.koplugin/main.lua") -local function readerFor(document, filepath) - return Reader:new{ - download_images = true, max_image_size_mb = 10, - findLocalDocumentByReadwiseId = function() return filepath end, + +local function doc(n) + return { id = "doc-" .. n, title = "Article " .. n, image_url = "https://example.com/cover-" .. n, + html_content = "Article body" } +end +local function pathOf(n) return "/reader/doc-" .. n .. ".html" end +-- local_paths: Readwise id -> file, the articles already on the device. +local function readerFor(documents, local_paths, fields) + local reader = Reader:new{ + download_covers = true, covers_failed = {}, + localDocumentPaths = function() return local_paths end, + documentExists = function(_, id) return local_paths[id] ~= nil end, shouldSkipDocument = function() return false end, validateSettings = function() return true end, api = { resetRateLimit = noop }, cleanupArchivedDocuments = function() return 0 end, reconcileLocalDocuments = function() return 0 end, - getDocumentList = function() return { document } end, + getDocumentList = function() return documents end, updateAvailableTags = noop, showProgress = noop, hideProgress = noop, saveSettings = noop, initCollectionTracking = noop, saveCollections = noop, storeAuthorMetadata = noop, storeSourceUrlMetadata = noop, setDocumentMetadata = noop, updateDocumentCollections = noop, + processHtmlContent = function(_, content) return content end, } + for key, value in pairs(fields or {}) do reader[key] = value end + return reader end -local document = { id = "doc-1", title = "Article", image_url = "https://example.com/cover?size=large", html_content = "Article body" } -test("sync fills an existing article's missing cover without rewriting its HTML", function() +local function lastMessage() return messages[#messages] end +local function count(t) local n = 0; for _ in pairs(t) do n = n + 1 end; return n end + +test("with covers off, sync downloads and stores nothing", function() + local reader = readerFor({ doc(1) }, { ["doc-1"] = pathOf(1) }, { download_covers = false }) + reader:synchronize() + eq(calls, 0) + eq(reader.covers_pending, nil) +end) +test("the first sync with covers on adds covers to earlier articles without rewriting them", function() local file = os.tmpname() local f = assert(io.open(file, "wb")); f:write("existing annotations depend on this HTML"); f:close() - local reader = readerFor(document, file) + local reader = readerFor({ doc(1) }, { ["doc-1"] = file }) reader:synchronize() eq(installed and installed.path, file) f = assert(io.open(file, "rb")); eq(f:read("*a"), "existing annotations depend on this HTML"); f:close() os.remove(file) - assert(messages[#messages]:find("Covers downloaded: 1", 1, true), messages[#messages]) + assert(lastMessage():find("Covers added: 1", 1, true), lastMessage()) + eq(next(reader.covers_pending), nil) end) -test("new article downloads save HTML and a Reader cover", function() +test("earlier articles are queued once, so a removed cover is not downloaded again", function() + local reader = readerFor({ doc(1) }, { ["doc-1"] = pathOf(1) }) + reader:synchronize() + eq(calls, 1) + removeCover(pathOf(1)) + reader:synchronize() + eq(calls, 1) + assert(lastMessage():find("No new articles found", 1, true), lastMessage()) +end) +test("articles that already have a cover or have no image are not queued", function() + covers[pathOf(1)] = os.tmpname() + local no_image = doc(2) + no_image.image_url = nil + local reader = readerFor({ doc(1), no_image }, { ["doc-1"] = pathOf(1), ["doc-2"] = pathOf(2) }) + reader:synchronize() + eq(calls, 0) + eq(next(reader.covers_pending), nil) +end) +test("an open article is skipped and keeps its place for a later sync", function() + local reader = readerFor({ doc(1) }, { ["doc-1"] = pathOf(1) }, { ui = { document = { file = pathOf(1) } } }) + reader:synchronize() + eq(calls, 0) + eq(reader.covers_pending["doc-1"], true) + reader.ui = {} + reader:synchronize() + eq(calls, 1) +end) +test("the time budget stops covers of earlier articles; the rest wait", function() + local real_time, now = os.time, 0 + os.time = function() now = now + 20; return now end -- luacheck: ignore + local reader = readerFor({ doc(1), doc(2), doc(3) }, + { ["doc-1"] = pathOf(1), ["doc-2"] = pathOf(2), ["doc-3"] = pathOf(3) }) + local ok, err = pcall(reader.synchronize, reader) + os.time = real_time -- luacheck: ignore + assert(ok, err) + eq(calls, 1) + eq(reader.covers_pending["doc-2"], true) + eq(reader.covers_pending["doc-3"], true) +end) +test("temporary failures wait for the manual retry instead of being retried", function() + local reader = readerFor({ doc(1) }, { ["doc-1"] = pathOf(1) }) + reply = { code = 503 } + reader:synchronize() + eq(reader.covers_failed["doc-1"], true) + eq(next(reader.covers_pending), nil) + assert(lastMessage():find("Covers failed: 1", 1, true), lastMessage()) + reader:synchronize() + eq(calls, 1) + eq(reader:retryFailedCovers(), 1) + eq(next(reader.covers_failed), nil) + reply = {} + reader:synchronize() + eq(calls, 2) + eq(installed and installed.path, pathOf(1)) +end) +test("permanent failures are not recorded or retried", function() + local reader = readerFor({ doc(1) }, { ["doc-1"] = pathOf(1) }) + reply = { code = 404 } + reader:synchronize() + eq(next(reader.covers_failed), nil) + eq(next(reader.covers_pending), nil) + reader:synchronize() + eq(calls, 1) +end) +test("three network errors in a row stop cover downloads for the sync", function() + local documents, local_paths = {}, {} + for n = 1, 5 do + documents[n] = doc(n) + local_paths["doc-" .. n] = pathOf(n) + end + local reader = readerFor(documents, local_paths) + reply = { throw = true } + reader:synchronize() + eq(calls, 3) + eq(count(reader.covers_failed), 3) + eq(reader.covers_pending["doc-4"], true) + eq(reader.covers_pending["doc-5"], true) +end) +test("a pending article missing from this sync's list waits without blocking 'no changes'", function() + local reader = readerFor({ doc(1) }, { ["doc-1"] = pathOf(1), ["doc-9"] = pathOf(9) }, { + covers_pending = { ["doc-9"] = true }, + }) + reader:synchronize() + eq(calls, 0) + eq(reader.covers_pending["doc-9"], true) + assert(lastMessage():find("No new articles found", 1, true), lastMessage()) +end) +test("articles no longer on the device are dropped from both lists", function() + local reader = readerFor({ doc(1) }, { ["doc-1"] = pathOf(1) }, { + covers_pending = { gone = true, ["doc-1"] = true }, + covers_failed = { ["also-gone"] = true }, + }) + reader:startCoverSync({ doc(1) }, { ["doc-1"] = pathOf(1) }) + eq(reader.covers_pending.gone, nil) + eq(reader.covers_pending["doc-1"], true) + eq(next(reader.covers_failed), nil) +end) +local function downloadNew(fields, run) local dir = os.tmpname() os.remove(dir) -- Use a unique filename prefix rather than creating a directory. - local reader = readerFor(document) + local reader = readerFor({}, {}, fields) reader.directory = dir - reader.processHtmlContent = function(_, content) return content end - eq(reader:downloadDocument(document), "downloaded") - local path = dir .. "[rw-id_doc-1] Article.html" - eq(installed and installed.path, path) + reader.covers_pending = {} + local result = reader:downloadDocument(doc(1), run) + local path = dir .. "[rw-id_doc-1] Article 1.html" local f = assert(io.open(path, "rb")); eq(f:read("*a"), "Article body"); f:close() os.remove(path) + return result, path, reader +end +test("new article downloads save HTML and get their cover once", function() + local run = { added = 0, failed = 0, network_errors = 0 } + local result, path, reader = downloadNew(nil, run) + eq(result, "downloaded") + eq(installed and installed.path, path) + eq(run.added, 1) + eq(next(reader.covers_failed), nil) end) test("cover errors do not fail an article download", function() - local dir = os.tmpname() - os.remove(dir) - local reader = readerFor(document) - reader.directory = dir - reader.processHtmlContent = function(_, content) return content end reply = { throw = true } - eq(reader:downloadDocument(document), "downloaded") + local result, _, reader = downloadNew(nil, { added = 0, failed = 0, network_errors = 0 }) + eq(result, "downloaded") eq(installed, nil) - os.remove(dir .. "[rw-id_doc-1] Article.html") + eq(reader.covers_failed["doc-1"], true) +end) +test("after three network errors, new articles' covers wait in the pending list", function() + local result, _, reader = downloadNew(nil, { added = 0, failed = 0, network_errors = 3 }) + eq(result, "downloaded") + eq(calls, 0) + eq(reader.covers_pending["doc-1"], true) +end) +test("with covers off, new article downloads make no cover request", function() + eq(downloadNew({ download_covers = false }, nil), "downloaded") + eq(calls, 0) +end) +test("downloadDocument skips articles already on the device without fetching a cover", function() + local reader = readerFor({}, { ["doc-1"] = pathOf(1) }) + eq(reader:downloadDocument(doc(1), { added = 0, failed = 0, network_errors = 0 }), "skipped") + eq(calls, 0) +end) +test("settings keep an empty pending list but no empty failed list", function() + local saved + local reader = readerFor({}, {}, { + covers_pending = {}, covers_failed = {}, + readwise_settings = { saveSetting = function(_, _, value) saved = value end, flush = noop }, + }) + reader.saveSettings = nil -- use the real one + reader:saveSettings() + eq(type(saved.covers_pending), "table") + eq(saved.covers_failed, nil) + eq(saved.download_covers, true) + reader.covers_failed = { ["doc-1"] = true } + reader:saveSettings() + eq(saved.covers_failed["doc-1"], true) end) print(string.format("%d cover tests passed", passed)) diff --git a/tests/highlights_test.lua b/tests/highlights_test.lua index e9859c4..5e7ca28 100644 --- a/tests/highlights_test.lua +++ b/tests/highlights_test.lua @@ -535,6 +535,7 @@ test("failed export keeps local files and sync cursor but still downloads", func reader.reconcileLocalDocuments = forbidden reader.getDocumentList = function() return server_documents end reader.updateAvailableTags = noop + reader.localDocumentPaths = function() return {} end reader.documentExists = function() return false end reader.initCollectionTracking = noop reader.saveCollections = noop