diff --git a/apps/readest.koplugin/readest_syncannotations.lua b/apps/readest.koplugin/readest_syncannotations.lua index db08ac7b..535c282b 100644 --- a/apps/readest.koplugin/readest_syncannotations.lua +++ b/apps/readest.koplugin/readest_syncannotations.lua @@ -133,6 +133,75 @@ function SyncAnnotations:getAnnotations(ui, settings, book_hash, meta_hash, full return notes end +-- Drop local annotations the server has tombstoned (deleted_at set), so a +-- highlight deleted on Readest also disappears in KOReader. Without this, +-- pull only skips deleted notes — the local copy lingers and any later +-- push resurrects it on the server, making it reappear (issue #4119). +function SyncAnnotations:removeDeletedAnnotations(annotation_mgr, notes, book_hash) + local annotations = annotation_mgr and annotation_mgr.annotations + if not annotations or #annotations == 0 then return 0 end + + -- Map every local annotation to its index by id and by position so a + -- tombstone can be matched however it was originally created: pulled + -- notes carry a stored id, native KOReader highlights don't (their id + -- is derived from book hash + positions, matching what push uploads). + local index_by_id = {} + local index_by_anno = {} + local index_by_bookmark = {} + for i, item in ipairs(annotations) do + if item.id then + index_by_id[item.id] = i + end + if item.drawer then + local pos0 = item.pos0 + local pos1 = item.pos1 + if type(pos0) == "table" then pos0 = nil end + if type(pos1) == "table" then pos1 = nil end + if pos0 then + index_by_anno[tostring(pos0) .. "|" .. tostring(pos1 or "")] = i + if not item.id then + local id = self:generateNoteId(book_hash, "annotation", tostring(pos0), pos1 and tostring(pos1)) + index_by_id[id] = i + end + end + elseif type(item.page) == "string" then + index_by_bookmark[item.page] = i + if not item.id then + local id = self:generateNoteId(book_hash, "bookmark", item.page) + index_by_id[id] = i + end + end + end + + local to_remove = {} + for _, note in ipairs(notes) do + if note.deleted_at then + local idx + if note.id and index_by_id[note.id] then + idx = index_by_id[note.id] + elseif note.type == "bookmark" and note.xpointer0 then + idx = index_by_bookmark[note.xpointer0] + elseif note.xpointer0 then + idx = index_by_anno[note.xpointer0 .. "|" .. (note.xpointer1 or "")] + end + if idx then + to_remove[idx] = true + end + end + end + + -- Remove highest index first so earlier indexes stay valid. + local indexes = {} + for idx in pairs(to_remove) do + indexes[#indexes + 1] = idx + end + table.sort(indexes, function(a, b) return a > b end) + for _, idx in ipairs(indexes) do + table.remove(annotations, idx) + end + return #indexes +end + function SyncAnnotations:push(ui, settings, client, interactive, full_sync) local book_hash = ui.doc_settings:readSetting("partial_md5_checksum") local meta_hash = ui.doc_settings:readSetting("readest_sync") or {} @@ -252,6 +321,10 @@ function SyncAnnotations:pull(ui, settings, client, book_hash, meta_hash, dialog local annotation_mgr = ui.annotation if not annotation_mgr then return end + -- Honor remote deletions before adding: drop local annotations + -- the server has tombstoned so they don't reappear (issue #4119). + local removed = self:removeDeletedAnnotations(annotation_mgr, data, book_hash) + -- Build dedup sets: by ID, by pos0|pos1 for annotations, by page xpointer for bookmarks local existing_ids = {} local existing_annotations = {} @@ -378,7 +451,7 @@ function SyncAnnotations:pull(ui, settings, client, book_hash, meta_hash, dialog }) end - if added > 0 then + if added > 0 or removed > 0 then UIManager:setDirty(dialog, "ui") end end diff --git a/apps/readest.koplugin/spec/syncannotations_spec.lua b/apps/readest.koplugin/spec/syncannotations_spec.lua new file mode 100644 index 00000000..c384ca61 --- /dev/null +++ b/apps/readest.koplugin/spec/syncannotations_spec.lua @@ -0,0 +1,175 @@ +-- syncannotations_spec.lua +-- Tests for readest_syncannotations.lua. The push/pull entrypoints are +-- glued to KOReader's UI (UIManager, InfoMessage, the document/annotation +-- objects) and aren't unit-testable in isolation. These specs lock down the +-- pure reconciliation logic: removeDeletedAnnotations, which drops local +-- annotations the server has tombstoned so deletions sync across platforms +-- (issue #4119 — deleted highlights reappearing in KOReader). + +require("spec_helper") + +-- The module pulls a handful of KOReader globals at require-time. Stub the +-- minimum surface here so the require succeeds under busted. +package.preload["ui/event"] = function() + return { new = function(_, name, data) return { name = name, data = data } end } +end +package.preload["ui/widget/infomessage"] = function() + return { new = function() return {} end } +end +package.preload["ui/network/manager"] = function() return {} end +package.preload["ui/uimanager"] = function() + return { + show = function() end, + setDirty = function() end, + nextTick = function(_, fn) fn() end, + } +end +package.preload["ffi/util"] = function() + return { template = function(s) return s end } +end +package.preload["readest_i18n"] = function() + return function(s) return s end +end +-- Deterministic stand-in for KOReader's FFI md5. Real cryptographic +-- strength is irrelevant to the matching logic; we only need a stable, +-- well-distributed hash so generated note IDs collide iff their inputs do. +package.preload["ffi/sha2"] = function() + return { + md5 = function(s) + -- djb2: addition/multiplication only, so it compiles under + -- LuaJIT (5.1), which rejects the `~` bitwise operator. + local h = 5381 + for i = 1, #s do + h = (h * 33 + s:byte(i)) % 4294967296 + end + return string.format("%08x", h) + end, + } +end + +describe("readest_syncannotations", function() + local SyncAnnotations + + before_each(function() + package.loaded["readest_syncannotations"] = nil + SyncAnnotations = require("readest_syncannotations") + end) + + describe("removeDeletedAnnotations", function() + local BOOK_HASH = "book-hash-1" + + local function mgr(annotations) + return { annotations = annotations } + end + + it("removes a local annotation tombstoned by matching id", function() + local annotation_mgr = mgr({ + { id = "keep1", drawer = "lighten", pos0 = "/a/1", pos1 = "/a/2" }, + { id = "gone1", drawer = "lighten", pos0 = "/b/1", pos1 = "/b/2" }, + }) + local removed = SyncAnnotations:removeDeletedAnnotations(annotation_mgr, { + { id = "gone1", type = "annotation", xpointer0 = "/b/1", deleted_at = "2026-05-17T00:00:00" }, + }, BOOK_HASH) + + assert.are.equal(1, removed) + assert.are.equal(1, #annotation_mgr.annotations) + assert.are.equal("keep1", annotation_mgr.annotations[1].id) + end) + + it("removes a native annotation (no stored id) via the generated id", function() + local annotation_mgr = mgr({ + { drawer = "lighten", pos0 = "/x/1", pos1 = "/x/2" }, + }) + -- The note id is what KOReader produced when it first pushed + -- this highlight: a hash of the book + positions. + local note_id = SyncAnnotations:generateNoteId(BOOK_HASH, "annotation", "/x/1", "/x/2") + local removed = SyncAnnotations:removeDeletedAnnotations(annotation_mgr, { + { id = note_id, type = "annotation", xpointer0 = "/x/1", xpointer1 = "/x/2", + deleted_at = "2026-05-17T00:00:00" }, + }, BOOK_HASH) + + assert.are.equal(1, removed) + assert.are.equal(0, #annotation_mgr.annotations) + end) + + it("removes an annotation by position when the id is absent on both sides", function() + local annotation_mgr = mgr({ + { drawer = "underscore", pos0 = "/p/1", pos1 = "/p/9" }, + }) + local removed = SyncAnnotations:removeDeletedAnnotations(annotation_mgr, { + { type = "annotation", xpointer0 = "/p/1", xpointer1 = "/p/9", + deleted_at = "2026-05-17T00:00:00" }, + }, BOOK_HASH) + + assert.are.equal(1, removed) + assert.are.equal(0, #annotation_mgr.annotations) + end) + + it("removes a bookmark by page xpointer", function() + local annotation_mgr = mgr({ + { id = "bm1", page = "/doc/bm" }, + { id = "hl1", drawer = "lighten", pos0 = "/h/1" }, + }) + local removed = SyncAnnotations:removeDeletedAnnotations(annotation_mgr, { + { id = "bm1", type = "bookmark", xpointer0 = "/doc/bm", + deleted_at = "2026-05-17T00:00:00" }, + }, BOOK_HASH) + + assert.are.equal(1, removed) + assert.are.equal(1, #annotation_mgr.annotations) + assert.are.equal("hl1", annotation_mgr.annotations[1].id) + end) + + it("ignores notes without a deleted_at tombstone", function() + local annotation_mgr = mgr({ + { id = "a1", drawer = "lighten", pos0 = "/a/1" }, + }) + local removed = SyncAnnotations:removeDeletedAnnotations(annotation_mgr, { + { id = "a1", type = "annotation", xpointer0 = "/a/1" }, + }, BOOK_HASH) + + assert.are.equal(0, removed) + assert.are.equal(1, #annotation_mgr.annotations) + end) + + it("leaves annotations untouched when a tombstone has no local match", function() + local annotation_mgr = mgr({ + { id = "a1", drawer = "lighten", pos0 = "/a/1" }, + }) + local removed = SyncAnnotations:removeDeletedAnnotations(annotation_mgr, { + { id = "not-here", type = "annotation", xpointer0 = "/z/9", + deleted_at = "2026-05-17T00:00:00" }, + }, BOOK_HASH) + + assert.are.equal(0, removed) + assert.are.equal(1, #annotation_mgr.annotations) + end) + + it("removes multiple tombstoned annotations without index shift errors", function() + local annotation_mgr = mgr({ + { id = "a1", drawer = "lighten", pos0 = "/a/1" }, + { id = "a2", drawer = "lighten", pos0 = "/a/2" }, + { id = "a3", drawer = "lighten", pos0 = "/a/3" }, + { id = "a4", drawer = "lighten", pos0 = "/a/4" }, + }) + local removed = SyncAnnotations:removeDeletedAnnotations(annotation_mgr, { + { id = "a1", type = "annotation", xpointer0 = "/a/1", deleted_at = "2026-05-17T00:00:00" }, + { id = "a3", type = "annotation", xpointer0 = "/a/3", deleted_at = "2026-05-17T00:00:00" }, + }, BOOK_HASH) + + assert.are.equal(2, removed) + assert.are.equal(2, #annotation_mgr.annotations) + assert.are.equal("a2", annotation_mgr.annotations[1].id) + assert.are.equal("a4", annotation_mgr.annotations[2].id) + end) + + it("returns 0 when the annotation manager has no annotations", function() + local annotation_mgr = mgr({}) + local removed = SyncAnnotations:removeDeletedAnnotations(annotation_mgr, { + { id = "a1", type = "annotation", xpointer0 = "/a/1", deleted_at = "2026-05-17T00:00:00" }, + }, BOOK_HASH) + + assert.are.equal(0, removed) + end) + end) +end)