Merge pull request #1930 from koreader/houqp-master

fix(readerrolling): crash with legacy last_per config in page mode

.ci/script.sh

@@ -8,4 +8,4 @@ make all
 retry_cmd 6 make testfront
 set +o pipefail
 luajit $(which luacheck) --no-color -q frontend | tee ./luacheck.out
-test $(grep Total ./luacheck.out | awk '{print $2}') -le 63
+test $(grep Total ./luacheck.out | awk '{print $2}') -le 59

.luacheckrc

@@ -1,5 +1,7 @@
 unused_args = false
 std = "luajit"
+-- ignore implicit self
+self = false
 
 globals = {
     "G_reader_settings",

Makefile

@@ -340,9 +340,12 @@ po:
 	$(MAKE) -i -C l10n bootstrap pull
 
 static-check:
-	@if which luacheck > /dev/null; then luacheck -q frontend; else \
-		echo "[!] luacheck not found. "\
-		"you can install it with 'luarocks install luacheck'"; fi
+	@if which luacheck > /dev/null; then \
+			luacheck -q frontend; \
+		else \
+			echo "[!] luacheck not found. "\
+			"you can install it with 'luarocks install luacheck'"; \
+		fi
 
 doc:
 	make -C doc

frontend/apps/reader/modules/readerrolling.lua

@@ -180,12 +180,21 @@ function ReaderRolling:onReadSettings(config)
             self.ui.document:gotoXPointer(self.xpointer)
         end)
     -- we read last_percent just for backward compatibility
+    -- FIXME: remove this branch with migration script
     elseif last_per then
         table.insert(self.ui.postInitCallback, function()
             self:_gotoPercent(last_per)
-            -- we have to do a real pos change in self.ui.document._document
+            -- _gotoPercent calls _gotoPos, which only updates self.current_pos
+            -- and self.view.
+            -- we need to do a real pos change in self.ui.document._document
             -- to update status information in CREngine.
             self.ui.document:gotoPos(self.current_pos)
+            -- _gotoPercent already calls gotoPos, so no need to emit
+            -- PosUpdate event in scroll mode
+            if self.view.view_mode == "page" then
+                self.ui:handleEvent(
+                    Event:new("PageUpdate", self.ui.document:getCurrentPage()))
+            end
             self.xpointer = self.ui.document:getXPointer()
         end)
     else

frontend/dbg.lua

@@ -2,7 +2,7 @@ local dump = require("dump")
 local isAndroid, android = pcall(require, "android")
 
 local Dbg = {
-    is_on = false,
+    is_on = nil,
     ev_log = nil,
 }
 
@@ -25,17 +25,40 @@ local function LvDEBUG(lv, ...)
     end
 end
 
-function Dbg_mt.__call(dbg, ...)
-    if dbg.is_on then LvDEBUG(math.huge, ...) end
-end
-
 function Dbg:turnOn()
+    if self.is_on == true then return end
     self.is_on = true
 
+    Dbg_mt.__call = function(dbg, ...) LvDEBUG(math.huge, ...) end
+    Dbg.guard = function(_, module, method, pre_guard, post_guard)
+        local old_method = module[method]
+        module[method] = function(...)
+            if pre_guard then
+                pre_guard(...)
+            end
+            local values = {old_method(...)}
+            if post_guard then
+                post_guard(...)
+            end
+            return unpack(values)
+        end
+    end
+
     -- create or clear ev log file
     self.ev_log = io.open("ev.log", "w")
 end
 
+function Dbg:turnOff()
+    if self.is_on == false then return end
+    self.is_on = false
+    function Dbg_mt.__call() end
+    function Dbg.guard() end
+    if self.ev_log then
+        io.close(self.ev_log)
+        self.ev_log = nil
+    end
+end
+
 function Dbg:logEv(ev)
     local log = ev.type.."|"..ev.code.."|"
                 ..ev.value.."|"..ev.time.sec.."|"..ev.time.usec.."\n"
@@ -51,4 +74,5 @@ end
 
 setmetatable(Dbg, Dbg_mt)
 
+Dbg:turnOff()
 return Dbg

frontend/ui/uimanager.lua

@@ -4,9 +4,10 @@ local Input = require("device").input
 local Event = require("ui/event")
 local Geom = require("ui/geometry")
 local util = require("ffi/util")
-local DEBUG = require("dbg")
+local dbg = require("dbg")
 local _ = require("gettext")
 
+local noop = function() end
 local MILLION = 1000000
 
 -- there is only one instance of this
@@ -46,17 +47,13 @@ function UIManager:init()
         -- resume.
         self:_initAutoSuspend()
         self.event_handlers["Suspend"] = function(input_event)
-            if self:_autoSuspendEnabled() then
-                self:unschedule(self.auto_suspend_action)
-            end
+            self:_stopAutoSuspend()
             Device:onPowerEvent(input_event)
         end
         self.event_handlers["Resume"] = function(input_event)
             Device:onPowerEvent(input_event)
             self:sendEvent(Event:new("Resume"))
-            if self:_autoSuspendEnabled() then
-                self:_startAutoSuspend()
-            end
+            self:_startAutoSuspend()
         end
         self.event_handlers["Light"] = function()
             Device:getPowerDevice():toggleFrontlight()
@@ -124,7 +121,7 @@ end
 -- modal widget should be always on the top
 -- for refreshtype & refreshregion see description of setDirty()
 function UIManager:show(widget, refreshtype, refreshregion, x, y)
-    DEBUG("show widget", widget._name)
+    dbg("show widget", widget._name)
     self._running = true
     local window = {x = x or 0, y = y or 0, widget = widget}
     -- put this window on top of the toppest non-modal window
@@ -150,10 +147,10 @@ end
 -- for refreshtype & refreshregion see description of setDirty()
 function UIManager:close(widget, refreshtype, refreshregion)
     if not widget then
-        DEBUG("widget not exist to be closed")
+        dbg("widget not exist to be closed")
         return
     end
-    DEBUG("close widget", widget.id)
+    dbg("close widget", widget.id)
     -- TODO: Why do we the following?
     Input.disable_double_tap = DGESDETECT_DISABLE_DOUBLE_TAP
     local dirty = false
@@ -178,8 +175,6 @@ end
 
 -- schedule an execution task, task queue is in ascendant order
 function UIManager:schedule(time, action)
-    assert(time[1] >= 0 and time[2] >= 0, "Only positive time allowed")
-    assert(action ~= nil)
     local p, s, e = 1, 1, #self._task_queue
     if e ~= 0 then
         local us = time[1] * MILLION + time[2]
@@ -212,10 +207,14 @@ function UIManager:schedule(time, action)
     table.insert(self._task_queue, p, { time = time, action = action })
     self._task_queue_dirty = true
 end
+dbg:guard(UIManager, 'schedule',
+    function(self, time, action)
+        assert(time[1] >= 0 and time[2] >= 0, "Only positive time allowed")
+        assert(action ~= nil)
+    end)
 
 -- schedule task in a certain amount of seconds (fractions allowed) from now
 function UIManager:scheduleIn(seconds, action)
-    assert(seconds >= 0, "Only positive seconds allowed")
     local when = { util.gettime() }
     local s = math.floor(seconds)
     local usecs = (seconds - s) * MILLION
@@ -227,6 +226,10 @@ function UIManager:scheduleIn(seconds, action)
     end
     self:schedule(when, action)
 end
+dbg:guard(UIManager, 'scheduleIn',
+    function(self, seconds, action)
+        assert(seconds >= 0, "Only positive seconds allowed")
+    end)
 
 function UIManager:nextTick(action)
     return self:scheduleIn(0, action)
@@ -239,13 +242,14 @@ end
 -- UIManager:scheduleIn(10, self.anonymousFunction)
 -- UIManager:unschedule(self.anonymousFunction)
 function UIManager:unschedule(action)
-    assert(action ~= nil)
     for i = #self._task_queue, 1, -1 do
         if self._task_queue[i].action == action then
             table.remove(self._task_queue, i)
         end
     end
 end
+dbg:guard(UIManager, 'unschedule',
+    function(self, action) assert(action ~= nil) end)
 
 --[[
 register a widget to be repainted and enqueue a refresh
@@ -269,17 +273,6 @@ function UIManager:setDirty(widget, refreshtype, refreshregion)
             end
         else
             self._dirty[widget] = true
-            if DEBUG.is_on then
-                -- when debugging, we check if we get handed a valid widget,
-                -- which would be a dialog that was previously passed via show()
-                local found = false
-                for i = 1, #self._window_stack do
-                    if self._window_stack[i].widget == widget then found = true end
-                end
-                if not found then
-                    DEBUG("INFO: invalid widget for setDirty()", debug.traceback())
-                end
-            end
         end
     end
     -- handle refresh information
@@ -291,6 +284,20 @@ function UIManager:setDirty(widget, refreshtype, refreshregion)
         self:_refresh(refreshtype, refreshregion)
     end
 end
+dbg:guard(UIManager, 'setDirty',
+    nil,
+    function(self, widget, refreshtype, refreshregion)
+        if not widget then return end
+        -- when debugging, we check if we get handed a valid widget,
+        -- which would be a dialog that was previously passed via show()
+        local found = false
+        for i = 1, #self._window_stack do
+            if self._window_stack[i].widget == widget then found = true end
+        end
+        if not found then
+            dbg("INFO: invalid widget for setDirty()", debug.traceback())
+        end
+    end)
 
 function UIManager:insertZMQ(zeromq)
     table.insert(self._zeromqs, zeromq)
@@ -308,7 +315,7 @@ end
 -- set full refresh rate for e-ink screen
 -- and make the refresh rate persistant in global reader settings
 function UIManager:setRefreshRate(rate)
-    DEBUG("set screen full refresh rate", rate)
+    dbg("set screen full refresh rate", rate)
     self.FULL_REFRESH_COUNT = rate
     G_reader_settings:saveSetting("full_refresh_count", rate)
 end
@@ -320,7 +327,7 @@ end
 
 -- signal to quit
 function UIManager:quit()
-    DEBUG("quiting uimanager")
+    dbg("quiting uimanager")
     self._task_queue_dirty = false
     self._running = false
     self._run_forever = nil
@@ -441,7 +448,7 @@ function UIManager:_refresh(mode, region)
     if not region and mode == "partial" and not self.refresh_counted then
         self.refresh_count = (self.refresh_count + 1) % self.FULL_REFRESH_COUNT
         if self.refresh_count == self.FULL_REFRESH_COUNT - 1 then
-            DEBUG("promote refresh to full refresh")
+            dbg("promote refresh to full refresh")
             mode = "full"
         end
         self.refresh_counted = true
@@ -499,13 +506,13 @@ function UIManager:_repaint()
     -- If we don't, we add one now and print a warning if we
     -- are debugging
     if dirty and #self._refresh_stack == 0 then
-        DEBUG("WARNING: no refresh got enqueued. Will do a partial full screen refresh, which might be inefficient")
+        dbg("WARNING: no refresh got enqueued. Will do a partial full screen refresh, which might be inefficient")
         self:_refresh("partial")
     end
 
     -- execute refreshes:
     for _, refresh in ipairs(self._refresh_stack) do
-        DEBUG("triggering refresh", refresh)
+        dbg("triggering refresh", refresh)
         Screen[refresh_methods[refresh.mode]](Screen,
             refresh.region.x - 1, refresh.region.y - 1,
             refresh.region.w + 2, refresh.region.h + 2)
@@ -529,15 +536,15 @@ function UIManager:handleInput()
     -- for input events:
     repeat
         wait_until, now = self:_checkTasks()
-        --DEBUG("---------------------------------------------------")
-        --DEBUG("exec stack", self._task_queue)
-        --DEBUG("window stack", self._window_stack)
-        --DEBUG("dirty stack", self._dirty)
-        --DEBUG("---------------------------------------------------")
+        --dbg("---------------------------------------------------")
+        --dbg("exec stack", self._task_queue)
+        --dbg("window stack", self._window_stack)
+        --dbg("dirty stack", self._dirty)
+        --dbg("---------------------------------------------------")
 
         -- stop when we have no window to show
         if #self._window_stack == 0 and not self._run_forever then
-            DEBUG("no dialog left to show")
+            dbg("no dialog left to show")
             self:quit()
             return nil
         end
@@ -575,9 +582,7 @@ function UIManager:handleInput()
 
     -- delegate input_event to handler
     if input_event then
-        if self:_autoSuspendEnabled() then
-            self.last_action_sec = util.gettime()
-        end
+        self:_resetAutoSuspendTimer()
         local handler = self.event_handlers[input_event]
         if handler then
             handler(input_event)
@@ -587,7 +592,7 @@ function UIManager:handleInput()
     end
 
     if self.looper then
-        DEBUG("handle input in turbo I/O looper")
+        dbg("handle input in turbo I/O looper")
         self.looper:add_callback(function()
             -- FIXME: force close looper when there is unhandled error,
             -- otherwise the looper will hang. Any better solution?
@@ -637,6 +642,10 @@ end
 
 -- Kobo does not have an auto suspend function, so we implement it ourselves.
 function UIManager:_initAutoSuspend()
+    local function isAutoSuspendEnabled()
+        return Device:isKobo() and self.auto_suspend_sec > 0
+    end
+
     local sec = G_reader_settings:readSetting("auto_suspend_timeout_seconds")
     if sec then
         self.auto_suspend_sec = sec
@@ -644,7 +653,8 @@ function UIManager:_initAutoSuspend()
         -- default setting is 60 minutes
         self.auto_suspend_sec = 60 * 60
     end
-    if self:_autoSuspendEnabled() then
+
+    if isAutoSuspendEnabled() then
         self.auto_suspend_action = function()
             local now = util.gettime()
             -- Do not repeat auto suspend procedure after suspend.
@@ -656,20 +666,32 @@ function UIManager:_initAutoSuspend()
                     self.auto_suspend_action)
             end
         end
+
+        function UIManager:_startAutoSuspend()
+            self.last_action_sec = util.gettime()
+            self:nextTick(self.auto_suspend_action)
+        end
+        dbg:guard(UIManager, '_startAutoSuspend',
+            function()
+                assert(isAutoSuspendEnabled())
+            end)
+
+        function UIManager:_stopAutoSuspend()
+            self:unschedule(self.auto_suspend_action)
+        end
+
+        function UIManager:_resetAutoSuspendTimer()
+            self.last_action_sec = util.gettime()
+        end
+
         self:_startAutoSuspend()
+    else
+        self._startAutoSuspend = noop
+        self._stopAutoSuspend = noop
     end
 end
 
-function UIManager:_startAutoSuspend()
-    assert(self:_autoSuspendEnabled())
-    self.last_action_sec = util.gettime()
-    self:nextTick(self.auto_suspend_action)
-end
-
-function UIManager:_autoSuspendEnabled()
-    return Device:isKobo() and self.auto_suspend_sec > 0
-end
+UIManager._resetAutoSuspendTimer = noop
 
 UIManager:init()
 return UIManager
-

spec/unit/commonrequire.lua

@@ -21,8 +21,7 @@ local Input = require("device").input
 Input.dummy = true
 
 -- turn on debug
-local DEBUG = require("dbg")
---DEBUG:turnOn()
+--require("dbg"):turnOn()
 
 function assertAlmostEquals(expected, actual, margin)
     if type(actual) ~= 'number' or type(expected) ~= 'number'

spec/unit/dbg_spec.lua

@@ -0,0 +1,65 @@
+describe("Dbg module", function()
+    local dbg
+    setup(function()
+        package.path = "?.lua;common/?.lua;rocks/share/lua/5.1/?.lua;frontend/?.lua;" .. package.path
+        dbg = require("dbg")
+    end)
+
+    it("setup mt.__call and guard after tunrnOn is called", function()
+        local old_call = getmetatable(dbg).__call
+        local old_guard = dbg.guard
+        dbg:turnOn()
+        assert.is_not.same(old_call, getmetatable(dbg).__call)
+        assert.is_not.same(old_guard, dbg.guard)
+    end)
+
+    it("should call pre_gard callback", function()
+        local called = false
+        local foo = {}
+        function foo:bar() end
+        assert.is.falsy(called)
+
+        dbg:turnOff()
+        assert.is.falsy(called)
+
+        dbg:turnOn()
+        dbg:guard(foo, 'bar', function() called = true end)
+        foo:bar()
+        assert.is.truthy(called)
+    end)
+
+    it("should call post_gard callback", function()
+        local called = false
+        local foo = {}
+        function foo:bar() end
+        assert.is.falsy(called)
+
+        dbg:turnOff()
+        assert.is.falsy(called)
+
+        dbg:turnOn()
+        dbg:guard(foo, 'bar', nil, function() called = true end)
+        foo:bar()
+        assert.is.truthy(called)
+    end)
+
+    it("should return all values returned by the guarded function", function()
+        local called = false, re
+        local foo = {}
+        function foo:bar() return 1 end
+        assert.is.falsy(called)
+
+        dbg:turnOn()
+        dbg:guard(foo, 'bar', function() called = true end)
+        re = {foo:bar()}
+        assert.is.truthy(called)
+        assert.is.same(re, {1})
+
+        called = false
+        function foo:bar() return 1, 2, 3 end
+        dbg:guard(foo, 'bar', function() called = true end)
+        assert.is.falsy(called)
+        re = {foo:bar()}
+        assert.is.same(re, {1, 2, 3})
+    end)
+end)

spec/unit/font_spec.lua

@@ -1,10 +1,11 @@
-require("commonrequire")
-local DEBUG = require("dbg")
-local Font = require("ui/font")
-
 describe("Font module", function()
-    local f = nil
+    local Font
+    setup(function()
+        require("commonrequire")
+        Font = require("ui/font")
+    end)
     it("should get face", function()
+        local f
         f = Font:getFace('cfont', 18)
         assert.are_not.equals(f.ftface, nil)
         f = Font:getFace('tfont', 16)

spec/unit/readerbookmark_spec.lua

@@ -1,4 +1,4 @@
-describe("ReaderBookmark module #ok", function()
+describe("ReaderBookmark module", function()
     local DocumentRegistry, ReaderUI, UIManager, Screen, Geom, DEBUG, DocSettings
     local sample_epub, sample_pdf
 

spec/unit/uimanager_spec.lua

@@ -1,12 +1,14 @@
-require("commonrequire")
-local util = require("ffi/util")
-local UIManager = require("ui/uimanager")
-local DEBUG = require("dbg")
-DEBUG:turnOn()
-
 describe("UIManager spec", function()
+    local Device, UIManager, util
     local noop = function() end
 
+    setup(function()
+        require("commonrequire")
+        util = require("ffi/util")
+        UIManager = require("ui/uimanager")
+        Device = require("device")
+    end)
+
     it("should consume due tasks", function()
         local now = { util.gettime() }
         local future = { now[1] + 60000, now[2] }
@@ -161,4 +163,32 @@ describe("UIManager spec", function()
         UIManager:_checkTasks()
         assert.is_true(UIManager._task_queue_dirty)
     end)
+
+    it("should setup auto suspend on kobo", function()
+        local old_reset_timer = UIManager._resetAutoSuspendTimer
+        local noop = old_reset_timer
+        assert.falsy(UIManager._startAutoSuspend)
+        assert.falsy(UIManager._stopAutoSuspend)
+        assert.truthy(old_reset_timer)
+        G_reader_settings:saveSetting("auto_suspend_timeout_seconds", 3600)
+
+        UIManager:quit()
+        -- should skip on non-kobo devices
+        UIManager:_initAutoSuspend()
+        assert.is.same(noop, UIManager._startAutoSuspend)
+        assert.is.same(noop, UIManager._stopAutoSuspend)
+        assert.truthy(old_reset_timer)
+        assert.is.same(#UIManager._task_queue, 0)
+        -- now test kobo devices
+        local old_is_kobo = Device.isKobo
+        Device.isKobo = function() return true end
+        UIManager:_initAutoSuspend()
+        assert.truthy(UIManager._startAutoSuspend)
+        assert.truthy(UIManager._stopAutoSuspend)
+        assert.is_not.same(UIManager._resetAutoSuspendTimer, old_reset_timer)
+        assert.is.same(#UIManager._task_queue, 1)
+        assert.is.same(UIManager._task_queue[1].action,
+                       UIManager.auto_suspend_action)
+        Device.isKobo = old_is_kobo
+    end)
 end)