diff --git a/apps/readest.koplugin/main.lua b/apps/readest.koplugin/main.lua index fef40c51..eb58b315 100644 --- a/apps/readest.koplugin/main.lua +++ b/apps/readest.koplugin/main.lua @@ -771,7 +771,15 @@ function ReadestSync:onPageUpdate(page) end end -function ReadestSync:onAnnotationsModified() +function ReadestSync:onAnnotationsModified(items) + -- A removal fires AnnotationsModified with a negative index_modified and the + -- deleted item at items[1]. Capture a tombstone now, before the item is gone + -- for good — the push walk only sees live annotations, so without this the + -- deletion never reaches the server (issue #4119, push direction). + if self.settings.access_token and items and items.index_modified + and items.index_modified < 0 and items[1] then + SyncAnnotations:recordDeletion(self.ui.doc_settings, items[1]) + end if self.settings.auto_sync and self.settings.access_token then UIManager:nextTick(function() self:pushBookNotes(false) diff --git a/apps/readest.koplugin/readest_syncannotations.lua b/apps/readest.koplugin/readest_syncannotations.lua index 535c282b..551708f9 100644 --- a/apps/readest.koplugin/readest_syncannotations.lua +++ b/apps/readest.koplugin/readest_syncannotations.lua @@ -64,6 +64,64 @@ function SyncAnnotations:generateNoteId(book_hash, note_type, pos0, pos1) return sha2.md5(raw):sub(1, 7) end +-- Build the Readest note payload for a single KOReader annotation item, or nil +-- if the item isn't a syncable highlight/bookmark. Shared by the push walk +-- (getAnnotations) and the deletion path (recordDeletion) so a note's id is +-- derived identically however it was created. +function SyncAnnotations:buildNoteDescriptor(item, book_hash, meta_hash) + local note_text = item.note + if note_text == "" then note_text = nil end + + 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 item.drawer and pos0 then + -- Annotation (highlight/underline/strikeout): has drawer and pos0/pos1 + local style = "highlight" + if item.drawer == "underscore" then + style = "underline" + elseif item.drawer == "strikeout" then + style = "squiggly" + end + + local id = item.id or self:generateNoteId(book_hash, "annotation", tostring(pos0), pos1 and tostring(pos1)) + return { + bookHash = book_hash, + metaHash = meta_hash, + id = id, + type = "annotation", + xpointer0 = tostring(pos0), + xpointer1 = pos1 and tostring(pos1) or nil, + text = item.text or "", + note = note_text, + style = style, + color = KO_TO_READEST_COLOR[item.color or "yellow"], + page = item.pageno, + createdAt = self:parseDatetimeToMs(item.datetime), + updatedAt = self:parseDatetimeToMs(item.datetime_updated or item.datetime), + } + elseif not item.drawer and type(item.page) == "string" then + -- Bookmark: no drawer, position in page field (xpointer string) + local page_xp = item.page + local id = item.id or self:generateNoteId(book_hash, "bookmark", page_xp) + return { + bookHash = book_hash, + metaHash = meta_hash, + id = id, + type = "bookmark", + xpointer0 = page_xp, + text = item.text or "", + note = note_text, + page = item.pageno, + createdAt = self:parseDatetimeToMs(item.datetime), + updatedAt = self:parseDatetimeToMs(item.datetime_updated or item.datetime), + } + end + return nil +end + function SyncAnnotations:getAnnotations(ui, settings, book_hash, meta_hash, full_sync) local annotations = ui.annotation and ui.annotation.annotations if not annotations then return {} end @@ -73,62 +131,12 @@ function SyncAnnotations:getAnnotations(ui, settings, book_hash, meta_hash, full local notes = {} for _, item in ipairs(annotations) do local updated_at = self:parseDatetimeToMs(item.datetime_updated or item.datetime) - if updated_at <= last_sync then - goto skip - end - - 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 item.drawer and pos0 then - -- Annotation (highlight/underline/strikeout): has drawer and pos0/pos1 - local style = "highlight" - if item.drawer == "underscore" then - style = "underline" - elseif item.drawer == "strikeout" then - style = "squiggly" + if updated_at > last_sync then + local note = self:buildNoteDescriptor(item, book_hash, meta_hash) + if note then + notes[#notes + 1] = note end - - local id = item.id or self:generateNoteId(book_hash, "annotation", tostring(pos0), pos1 and tostring(pos1)) - local note_text = item.note - if note_text == "" then note_text = nil end - table.insert(notes, { - bookHash = book_hash, - metaHash = meta_hash, - id = id, - type = "annotation", - xpointer0 = tostring(pos0), - xpointer1 = pos1 and tostring(pos1) or nil, - text = item.text or "", - note = note_text, - style = style, - color = KO_TO_READEST_COLOR[item.color or "yellow"], - page = item.pageno, - createdAt = self:parseDatetimeToMs(item.datetime), - updatedAt = updated_at, - }) - elseif not item.drawer and type(item.page) == "string" then - -- Bookmark: no drawer, position in page field (xpointer string) - local page_xp = item.page - local id = item.id or self:generateNoteId(book_hash, "bookmark", page_xp) - local note_text = item.note - if note_text == "" then note_text = nil end - table.insert(notes, { - bookHash = book_hash, - metaHash = meta_hash, - id = id, - type = "bookmark", - xpointer0 = page_xp, - text = item.text or "", - note = note_text, - page = item.pageno, - createdAt = self:parseDatetimeToMs(item.datetime), - updatedAt = updated_at, - }) end - ::skip:: end return notes end @@ -202,13 +210,51 @@ function SyncAnnotations:removeDeletedAnnotations(annotation_mgr, notes, book_ha return #indexes end +-- Stash a tombstone for a note deleted locally. By the time auto-sync runs the +-- deleted item is already gone from ui.annotation.annotations, so the push walk +-- can't see it; without this the deletion never reaches the server and the +-- highlight resurrects on the next pull (issue #4119, push direction). The +-- tombstone is a normal note payload with deletedAt set, persisted in the +-- per-book sidecar and folded into the next push (see push()), then cleared +-- once the server accepts it. +function SyncAnnotations:recordDeletion(doc_settings, item) + if not doc_settings or not item then return end + local book_hash = doc_settings:readSetting("partial_md5_checksum") + if not book_hash then return end + + local doc_readest_sync = doc_settings:readSetting("readest_sync") or {} + local note = self:buildNoteDescriptor(item, book_hash, doc_readest_sync.meta_hash_v1) + if not note then return end + note.deletedAt = os.time() * 1000 + + local deleted = doc_readest_sync.deleted_notes or {} + for _, t in ipairs(deleted) do + if t.id == note.id then return end -- already recorded + end + deleted[#deleted + 1] = note + doc_readest_sync.deleted_notes = deleted + doc_settings:saveSetting("readest_sync", doc_readest_sync) +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 {} - meta_hash = meta_hash.meta_hash_v1 + local doc_readest_sync = ui.doc_settings:readSetting("readest_sync") or {} + local meta_hash = doc_readest_sync.meta_hash_v1 if not book_hash or not meta_hash then return end local annotations = self:getAnnotations(ui, settings, book_hash, meta_hash, full_sync) + + -- Fold in tombstones for notes deleted locally since the last push. These + -- are gone from ui.annotation.annotations, so getAnnotations can't see them; + -- re-stamp the current book/meta hash in case they were recorded before the + -- book was registered for sync. + local deleted_notes = doc_readest_sync.deleted_notes or {} + for _, t in ipairs(deleted_notes) do + t.bookHash = book_hash + t.metaHash = meta_hash + annotations[#annotations + 1] = t + end + if #annotations == 0 then if interactive then UIManager:show(InfoMessage:new{ @@ -253,9 +299,12 @@ function SyncAnnotations:push(ui, settings, client, interactive, full_sync) settings.last_notes_sync_at = os.time() * 1000 G_reader_settings:saveSetting("readest_sync", settings) if ui.doc_settings then - local doc_readest_sync = ui.doc_settings:readSetting("readest_sync") or {} - doc_readest_sync.last_synced_at_notes = os.time() - ui.doc_settings:saveSetting("readest_sync", doc_readest_sync) + local synced = ui.doc_settings:readSetting("readest_sync") or {} + synced.last_synced_at_notes = os.time() + -- The server has the tombstones now; drop them so they don't + -- ride along on every future push. + synced.deleted_notes = nil + ui.doc_settings:saveSetting("readest_sync", synced) end end end diff --git a/apps/readest.koplugin/spec/syncannotations_spec.lua b/apps/readest.koplugin/spec/syncannotations_spec.lua index c384ca61..b8867563 100644 --- a/apps/readest.koplugin/spec/syncannotations_spec.lua +++ b/apps/readest.koplugin/spec/syncannotations_spec.lua @@ -172,4 +172,148 @@ describe("readest_syncannotations", function() assert.are.equal(0, removed) end) end) + + -- A note deleted in KOReader is removed from ui.annotation.annotations, so + -- the push walk (getAnnotations) can never see it. recordDeletion stashes a + -- deletedAt-stamped tombstone in the per-book sidecar so the next push tells + -- the server it's gone (issue #4119, push direction). Without this, the + -- deletion stays local and a later pull resurrects the highlight. + local function makeDocSettings(initial) + local store = initial or {} + return { + readSetting = function(_, k) return store[k] end, + saveSetting = function(_, k, v) store[k] = v end, + } + end + + describe("recordDeletion", function() + it("records a tombstone for a deleted highlight", function() + local doc_settings = makeDocSettings({ + partial_md5_checksum = "book-hash-1", + readest_sync = { meta_hash_v1 = "meta-1" }, + }) + SyncAnnotations:recordDeletion(doc_settings, { + id = "n1", drawer = "lighten", pos0 = "/a/1", pos1 = "/a/2", + text = "hello", datetime = "2026-05-17 10:00:00", + }) + + local deleted = doc_settings:readSetting("readest_sync").deleted_notes + assert.are.equal(1, #deleted) + assert.are.equal("n1", deleted[1].id) + assert.are.equal("annotation", deleted[1].type) + assert.are.equal("/a/1", deleted[1].xpointer0) + assert.are.equal("/a/2", deleted[1].xpointer1) + assert.is_truthy(deleted[1].deletedAt) + end) + + it("derives the id for a native highlight (no stored id) from positions", function() + local doc_settings = makeDocSettings({ + partial_md5_checksum = "book-hash-1", + readest_sync = { meta_hash_v1 = "meta-1" }, + }) + SyncAnnotations:recordDeletion(doc_settings, { + drawer = "lighten", pos0 = "/x/1", pos1 = "/x/2", + }) + + local expected = SyncAnnotations:generateNoteId("book-hash-1", "annotation", "/x/1", "/x/2") + local deleted = doc_settings:readSetting("readest_sync").deleted_notes + assert.are.equal(expected, deleted[1].id) + end) + + it("records a tombstone for a deleted bookmark", function() + local doc_settings = makeDocSettings({ + partial_md5_checksum = "book-hash-1", + readest_sync = { meta_hash_v1 = "meta-1" }, + }) + SyncAnnotations:recordDeletion(doc_settings, { id = "bm1", page = "/doc/bm" }) + + local deleted = doc_settings:readSetting("readest_sync").deleted_notes + assert.are.equal(1, #deleted) + assert.are.equal("bm1", deleted[1].id) + assert.are.equal("bookmark", deleted[1].type) + assert.are.equal("/doc/bm", deleted[1].xpointer0) + end) + + it("dedupes by id so re-deleting the same note doesn't pile up", function() + local doc_settings = makeDocSettings({ + partial_md5_checksum = "book-hash-1", + readest_sync = { meta_hash_v1 = "meta-1" }, + }) + local item = { id = "n1", drawer = "lighten", pos0 = "/a/1" } + SyncAnnotations:recordDeletion(doc_settings, item) + SyncAnnotations:recordDeletion(doc_settings, item) + + local deleted = doc_settings:readSetting("readest_sync").deleted_notes + assert.are.equal(1, #deleted) + end) + + it("ignores items that aren't syncable notes", function() + local doc_settings = makeDocSettings({ + partial_md5_checksum = "book-hash-1", + readest_sync = { meta_hash_v1 = "meta-1" }, + }) + -- No drawer and a non-string page → neither highlight nor bookmark. + SyncAnnotations:recordDeletion(doc_settings, { page = 12 }) + + local readest_sync = doc_settings:readSetting("readest_sync") + assert.is_nil(readest_sync.deleted_notes) + end) + end) + + describe("push folds tombstones", function() + local function makeUi(doc_settings, annotations) + return { + doc_settings = doc_settings, + annotation = { annotations = annotations or {} }, + } + end + + it("includes recorded tombstones in the payload and clears them on success", function() + local doc_settings = makeDocSettings({ + partial_md5_checksum = "book-hash-1", + readest_sync = { + meta_hash_v1 = "meta-1", + deleted_notes = { + { id = "gone1", type = "annotation", xpointer0 = "/b/1", deletedAt = 111 }, + }, + }, + }) + local captured + local client = { + pushChanges = function(_, payload, cb) + captured = payload + cb(true, {}) + end, + } + SyncAnnotations:push(makeUi(doc_settings), {}, client, false, false) + + assert.is_truthy(captured) + assert.are.equal(1, #captured.notes) + assert.are.equal("gone1", captured.notes[1].id) + assert.are.equal(111, captured.notes[1].deletedAt) + assert.are.equal("book-hash-1", captured.notes[1].bookHash) + assert.are.equal("meta-1", captured.notes[1].metaHash) + assert.is_nil(doc_settings:readSetting("readest_sync").deleted_notes) + end) + + it("keeps tombstones when the push fails so a later push retries", function() + local doc_settings = makeDocSettings({ + partial_md5_checksum = "book-hash-1", + readest_sync = { + meta_hash_v1 = "meta-1", + deleted_notes = { + { id = "gone1", type = "annotation", xpointer0 = "/b/1", deletedAt = 111 }, + }, + }, + }) + local client = { + pushChanges = function(_, _payload, cb) cb(false, nil) end, + } + SyncAnnotations:push(makeUi(doc_settings), {}, client, false, false) + + local deleted = doc_settings:readSetting("readest_sync").deleted_notes + assert.are.equal(1, #deleted) + assert.are.equal("gone1", deleted[1].id) + end) + end) end)