fix(koreader): offer a further place before a fresh copy pushes its own

A device that had not weighed its open book against BookOrbit since
opening dated its place by its last page turn. One turn from a fresh
download's cover made another device's further place look older, so
Sync now pushed the cover over it without asking, and the phone then
reopened the book at its title page.

Until the open book's place has been compared with BookOrbit's (a pull
or push went through, or Jump or Stay was answered), a further BookOrbit
place now also counts as newer, so Sync now and pulls offer Jump instead
of overwriting it. Once compared, page-turn times decide as before, and a
reset stamped after the last page turn still prompts.

Ref #1806

koreader-plugin/bookorbit.koplugin/bookorbit_book_sheet.lua

@@ -337,10 +337,12 @@ local function onAction(page, action)
     elseif kind == "jump" then
         local remote = page.book_data.remote
         page:close()
+        plugin:noteProgressCompared(page.book_data.digest)
         plugin:syncToProgress(remote.progress, remote.percentage)
         plugin:recordSyncSuccess("progress_pull", _("Progress pulled"))
     elseif kind == "stay" then
         page:close()
+        plugin:noteProgressCompared(page.book_data.digest)
     end
     return true
 end

koreader-plugin/bookorbit.koplugin/bookorbit_progress_sync.lua

@@ -133,12 +133,32 @@ function ProgressSync:remoteProgressIsNewer(body, local_percentage)
     if body.timestamp ~= nil then
         if local_timestamp > 0 then
             return body.timestamp > local_timestamp
+                or self:furtherBeforeCompared(body.percentage, local_percentage)
         end
         return body.percentage > local_percentage
     end
     return body.percentage > local_percentage
 end
 
+-- A page turn dates this device's place, but until that place has been weighed
+-- against BookOrbit's since the book opened it can be a fresh download's cover
+-- or a copy left days behind. One turn from there must not make another
+-- device's further place look older and get overwritten (#1806).
+function ProgressSync:furtherBeforeCompared(remote_percentage, local_percentage)
+    return remote_percentage > local_percentage and not self:progressComparedSinceOpen()
+end
+
+-- Whether the open book's place has been weighed against BookOrbit's since it
+-- opened: a pull or push went through, or the reader answered Jump or Stay.
+function ProgressSync:progressComparedSinceOpen()
+    local compared = self.progress_compared_digest
+    return compared ~= nil and compared == self:getDocumentDigest()
+end
+
+function ProgressSync:noteProgressCompared(digest)
+    self.progress_compared_digest = digest or self:getDocumentDigest()
+end
+
 function ProgressSync:applyRemoteProgress(body, on_done)
     self:syncToProgress(body.progress, body.percentage)
     if on_done then
@@ -171,12 +191,14 @@ function ProgressSync:reconcileProgressBeforeBookSync(digest, on_done)
     end
 
     if not body.percentage then
+        self:noteProgressCompared(digest)
         on_done(false)
         return true
     end
 
     if body.device == Device.model and body.device_id == self.device_id
             and (self.last_page_turn_timestamp or 0) > 0 then
+        self:noteProgressCompared(digest)
         on_done(false)
         return true
     end
@@ -186,22 +208,28 @@ function ProgressSync:reconcileProgressBeforeBookSync(digest, on_done)
     local local_percentage = self:getLastPercent()
 
     if local_percentage == body.percentage or body.progress == local_progress then
+        self:noteProgressCompared(digest)
         on_done(false)
         return true
     end
 
     local remote_newer = self:remoteProgressIsNewer(body, local_percentage)
     local strategy = remote_newer and self.settings.sync_forward or self.settings.sync_backward
-    if strategy == SYNC_STRATEGY.SILENT then
+    if strategy == SYNC_STRATEGY.PROMPT then
-        self:applyRemoteProgress(body, on_done)
+        -- Weighed once answered: a Stay, then reading on, makes this place the newer one.
-        return true
-    elseif strategy == SYNC_STRATEGY.PROMPT then
         openPrompt(self, body, remote_newer, true, function()
+            self:noteProgressCompared(digest)
             self:applyRemoteProgress(body, on_done)
         end, function()
+            self:noteProgressCompared(digest)
             on_done(remote_newer)
         end)
         return true
+    end
+    self:noteProgressCompared(digest)
+    if strategy == SYNC_STRATEGY.SILENT then
+        self:applyRemoteProgress(body, on_done)
+        return true
     elseif remote_newer then
         Toast.show(_("Kept your place. BookOrbit is further ahead."), { icon = "info", timeout = 4 })
         on_done(true)
@@ -231,7 +259,9 @@ function ProgressSync:pushProgressAfterBookSync(digest)
     if not self.ui or not self.ui.document or self:getDocumentDigest() ~= digest then return end
     local body, err = self:newClient():updateProgress(digest, self:getLastPercent(), self:getLastProgress(), os.time())
     self.push_timestamp = UIManager:getElapsedTimeSinceBoot()
-    if not body then
+    if body then
+        self:noteProgressCompared(digest)
+    else
         logger.dbg("BookOrbit: progress push after book sync failed:", err)
     end
 end
@@ -306,6 +336,7 @@ function ProgressSync:updateProgress(ensure_networking, interactive, on_suspend)
         NetworkMgr:disableWifi()
     end
 
+    if body then self:noteProgressCompared(digest) end
     if body and self.book_sheet then self:fetchRemoteProgress(digest, nil, true) end
 
     self.push_timestamp = now
@@ -367,6 +398,7 @@ function ProgressSync:getProgress(ensure_networking, interactive)
     end
 
     if not body.percentage then
+        self:noteProgressCompared(digest)
         if interactive then
             Toast.show(_("BookOrbit has no position for this book yet"), { icon = "info" })
         end
@@ -377,6 +409,7 @@ function ProgressSync:getProgress(ensure_networking, interactive)
     -- Explicit pulls and opens before any page turn must still recover that position.
     if not interactive and body.device == Device.model and body.device_id == self.device_id
             and (self.last_page_turn_timestamp or 0) > 0 then
+        self:noteProgressCompared(digest)
         return
     end
 
@@ -385,6 +418,7 @@ function ProgressSync:getProgress(ensure_networking, interactive)
     local percentage = self:getLastPercent()
 
     if percentage == body.percentage or body.progress == progress then
+        self:noteProgressCompared(digest)
         if interactive then
             Toast.show(_("Already in sync"))
         end
@@ -392,6 +426,7 @@ function ProgressSync:getProgress(ensure_networking, interactive)
     end
 
     if interactive then
+        self:noteProgressCompared(digest)
         self:syncToProgress(body.progress, body.percentage)
         if self.recordSyncSuccess then
             self:recordSyncSuccess("progress_pull", _("Progress pulled"))
@@ -403,25 +438,32 @@ function ProgressSync:getProgress(ensure_networking, interactive)
     local self_older
     if body.timestamp ~= nil then
         self_older = (body.timestamp > self.last_page_turn_timestamp)
+            or self:furtherBeforeCompared(body.percentage, percentage)
     else
         self_older = (body.percentage > percentage)
     end
 
     local strategy = self_older and self.settings.sync_forward or self.settings.sync_backward
+    if strategy == SYNC_STRATEGY.PROMPT then
+        openPrompt(self, body, self_older, false, function()
+            self:noteProgressCompared(digest)
+            self:syncToProgress(body.progress, body.percentage)
+            if self.recordSyncSuccess then
+                self:recordSyncSuccess("progress_pull", _("Progress pulled"))
+            end
+            showMoved(body)
+        end, function()
+            self:noteProgressCompared(digest)
+        end)
+        return
+    end
+    self:noteProgressCompared(digest)
     if strategy == SYNC_STRATEGY.SILENT then
         self:syncToProgress(body.progress, body.percentage)
         if self.recordSyncSuccess then
             self:recordSyncSuccess("progress_pull", _("Progress pulled"))
         end
         showMoved(body)
-    elseif strategy == SYNC_STRATEGY.PROMPT then
-        openPrompt(self, body, self_older, false, function()
-            self:syncToProgress(body.progress, body.percentage)
-            if self.recordSyncSuccess then
-                self:recordSyncSuccess("progress_pull", _("Progress pulled"))
-            end
-            showMoved(body)
-        end)
     end
 end
 

koreader-plugin/bookorbit.koplugin/main.lua

@@ -85,6 +85,8 @@ local BookOrbit = WidgetContainer:extend{
     pull_timestamp = nil,
     last_page = nil,
     last_page_turn_timestamp = nil,
+    -- Digest of the open book once its place was weighed against BookOrbit's.
+    progress_compared_digest = nil,
 
     settings = nil,
 }

koreader-plugin/spec/bookorbit_fresh_device_progress_test.lua

@@ -0,0 +1,146 @@
+-- A device that opens a book it has not synced since opening (a fresh download
+-- at its cover, or a copy left days behind) used to date its place by its last
+-- page turn. One turn made another device's further place look older, so Sync
+-- now pushed the cover over it without asking (#1806). Until the open book's
+-- place has been weighed against BookOrbit's, a further BookOrbit place counts
+-- as newer; once weighed, page-turn times decide as before.
+
+package.loaded["gettext"] = function(text) return text end
+package.loaded["ffi/util"] = {
+    template = function(text, ...)
+        local values = { ... }
+        return (text:gsub("%%(%d+)", function(index) return tostring(values[tonumber(index)]) end))
+    end,
+}
+local prompts = {}
+package.loaded["bookorbit_book_sheet"] = {
+    openPrompt = function(_plugin, prompt) table.insert(prompts, prompt) end,
+}
+package.loaded["bookorbit_toast"] = { show = function() end, error = function() end }
+package.loaded["device"] = { model = "Kindle" }
+package.loaded["ui/event"] = { new = function(_, name, value) return { name = name, value = value } end }
+package.loaded["ui/widget/infomessage"] = { new = function(_, opts) return opts end }
+package.loaded["optmath"] = { roundPercent = function(value) return value end, round = function(value) return value end }
+package.loaded["ui/network/manager"] = { willRerunWhenConnected = function() return false end }
+package.loaded["ui/uimanager"] = {
+    show = function() end,
+    nextTick = function(_, fn) fn() end,
+    scheduleIn = function(_, _, fn) fn() end,
+    unschedule = function() end,
+    getElapsedTimeSinceBoot = function() return 10000 end,
+}
+package.loaded["logger"] = { dbg = function() end, warn = function() end }
+package.loaded["ui/time"] = { s = function(value) return value end }
+package.loaded["util"] = { partialMD5 = function(file) return "digest:" .. tostring(file) end }
+package.loaded["bookorbit_state_manager"] = { repairFileIdentity = function() end }
+
+package.path = "koreader-plugin/bookorbit.koplugin/?.lua;" .. package.path
+
+local ProgressSync = require("bookorbit_progress_sync")
+
+local function assertEqual(actual, expected, label)
+    if actual ~= expected then
+        error(string.format("%s: expected %s, got %s", label, tostring(expected), tostring(actual)))
+    end
+end
+
+local DIGEST = "digest:/tmp/book.epub"
+-- The phone read to 46% before this device turned its first page.
+local PHONE = { percentage = 0.46, progress = "/body/DocFragment[20]/body/p[56]", device = "iPhone", device_id = "phone", timestamp = 20000 }
+local PAGE_TURN = 30000
+
+-- A fresh download, opened at its cover and turned one page.
+local function makePlugin(remote, local_percent)
+    prompts = {}
+    local plugin = { SYNC_STRATEGY = { PROMPT = 1, SILENT = 2, DISABLE = 3 } }
+    ProgressSync.install(plugin)
+    plugin.pull_timestamp, plugin.push_timestamp = 0, 0
+    plugin.device_id = "kindle"
+    -- Defaults as shipped: forward prompts, backward is disabled, auto sync off.
+    plugin.settings = { sync_forward = 1, sync_backward = 3, auto_sync = false }
+    plugin.last_page_turn_timestamp = PAGE_TURN
+    plugin.pushes, plugin.moves = {}, {}
+    plugin.ui = { document = { file = "/tmp/book.epub", info = { has_pages = false } } }
+    plugin.ui.handleEvent = function(_, event) table.insert(plugin.moves, event) end
+    plugin.local_percent = local_percent or 0.008
+    plugin.getLastProgress = function() return "/body/DocFragment[2]/body/h1/img" end
+    plugin.getLastPercent = function() return plugin.local_percent end
+    plugin.isLoggedIn = function() return true end
+    plugin.requestUpdateCheck = function() end
+    plugin.recordSyncSuccess = function() end
+    plugin.recordSyncError = function() end
+    plugin.newClient = function()
+        return {
+            getProgress = function() return remote end,
+            updateProgress = function(_, digest, percent)
+                table.insert(plugin.pushes, { digest = digest, percent = percent })
+                return {}
+            end,
+        }
+    end
+    return plugin
+end
+
+local function syncNow(plugin)
+    plugin:syncBookThenProgress(DIGEST, function(_skip_progress, after_sync) after_sync() end)
+end
+
+-- Sync now from the cover asks before anything moves, and pushes nothing.
+local plugin = makePlugin(PHONE)
+syncNow(plugin)
+assertEqual(#prompts, 1, "a further place on BookOrbit is offered, not overwritten")
+assertEqual(prompts[1].newer, true, "and it is offered as the newer place")
+assertEqual(#plugin.pushes, 0, "nothing is pushed while the question is open")
+
+-- Taking it moves the reader there before the position goes up.
+prompts[1].on_jump()
+assertEqual(plugin.moves[1].name, "GotoXPointer", "Jump moves the reader to BookOrbit's place")
+assertEqual(#plugin.pushes, 1, "then the position is pushed")
+
+-- Staying pushes nothing; it settles the question, so reading on from here
+-- makes this place the newer one, as it did before.
+plugin = makePlugin(PHONE)
+syncNow(plugin)
+prompts[1].on_stay()
+assertEqual(#plugin.pushes, 0, "declining the further place leaves the server alone")
+plugin.last_page_turn_timestamp = PAGE_TURN + 60
+plugin.local_percent = 0.012
+syncNow(plugin)
+assertEqual(#prompts, 1, "after Stay and another page, Sync now does not ask again")
+assertEqual(#plugin.pushes, 1, "and this device's place goes up")
+
+-- Once weighed in this opening, page-turn times decide as before: a later
+-- turn beats an older BookOrbit place without a question.
+plugin = makePlugin(PHONE)
+plugin:noteProgressCompared(DIGEST)
+syncNow(plugin)
+assertEqual(#prompts, 0, "a weighed place is not asked about again")
+assertEqual(#plugin.pushes, 1, "the newer page turn is pushed")
+
+-- Weighing is per book: another book's sync does not count for this one.
+plugin = makePlugin(PHONE)
+plugin:noteProgressCompared("digest:/tmp/other.epub")
+syncNow(plugin)
+assertEqual(#prompts, 1, "another book's comparison does not date this place")
+
+-- A place behind this device is still kept and pushed without a question.
+plugin = makePlugin({ percentage = 0.1, progress = "/body/DocFragment[5]/body", device = "iPhone", device_id = "phone", timestamp = 20000 }, 0.3)
+syncNow(plugin)
+assertEqual(#prompts, 0, "BookOrbit behind this device asks nothing")
+assertEqual(#plugin.pushes, 1, "and this device's further place is pushed")
+
+-- A pull after a page turn offers the further place instead of dropping it.
+plugin = makePlugin(PHONE)
+plugin:getProgress(false, false)
+assertEqual(#prompts, 1, "a pull from the cover offers the further place")
+assertEqual(prompts[1].newer, true, "as the newer place")
+prompts[1].on_stay()
+assertEqual(plugin:progressComparedSinceOpen(), true, "staying settles the question for this opening")
+
+-- The This book sheet's chip agrees.
+plugin = makePlugin(PHONE)
+assertEqual(plugin:remoteProgressIsNewer(PHONE, 0.008), true, "the sheet shows BookOrbit ahead before weighing")
+plugin:noteProgressCompared(DIGEST)
+assertEqual(plugin:remoteProgressIsNewer(PHONE, 0.008), false, "and page-turn times once weighed")
+
+print("bookorbit_fresh_device_progress_test: ok")