From dffc42d234b7f2117d1846baaba23e575a166860 Mon Sep 17 00:00:00 2001 From: Qingping Hou Date: Mon, 28 Mar 2016 21:23:33 -0700 Subject: [PATCH 1/3] fix(readerrolling): crash with legacy last_per config in page mode --- frontend/apps/reader/modules/readerrolling.lua | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/frontend/apps/reader/modules/readerrolling.lua b/frontend/apps/reader/modules/readerrolling.lua index 9ab3fc0c2..b4a08c0c1 100644 --- a/frontend/apps/reader/modules/readerrolling.lua +++ b/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 From e3137134f8b6923cc80e3ca92ac814463dfa0de2 Mon Sep 17 00:00:00 2001 From: Qingping Hou Date: Mon, 28 Mar 2016 23:37:15 -0700 Subject: [PATCH 2/3] dbg(add): guard method to toggle assert at based at runtimemode --- frontend/dbg.lua | 34 ++++++++++++-- frontend/ui/uimanager.lua | 77 ++++++++++++++++++------------- spec/unit/commonrequire.lua | 3 +- spec/unit/dbg_spec.lua | 65 ++++++++++++++++++++++++++ spec/unit/font_spec.lua | 11 +++-- spec/unit/readerbookmark_spec.lua | 2 +- 6 files changed, 147 insertions(+), 45 deletions(-) create mode 100644 spec/unit/dbg_spec.lua diff --git a/frontend/dbg.lua b/frontend/dbg.lua index fea21d785..af437cc7b 100644 --- a/frontend/dbg.lua +++ b/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 diff --git a/frontend/ui/uimanager.lua b/frontend/ui/uimanager.lua index 8281df05f..4e8a23f94 100644 --- a/frontend/ui/uimanager.lua +++ b/frontend/ui/uimanager.lua @@ -4,7 +4,7 @@ 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 MILLION = 1000000 @@ -124,7 +124,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 +150,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 +178,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 +210,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 +229,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 +245,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 +276,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 +287,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 +318,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 +330,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 +451,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 +509,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 +539,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 @@ -587,7 +597,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? @@ -661,10 +671,13 @@ function UIManager:_initAutoSuspend() end function UIManager:_startAutoSuspend() - assert(self:_autoSuspendEnabled()) self.last_action_sec = util.gettime() self:nextTick(self.auto_suspend_action) end +dbg:guard(UIManager, '_startAutoSuspend', + function(self) + assert(self:_autoSuspendEnabled()) + end) function UIManager:_autoSuspendEnabled() return Device:isKobo() and self.auto_suspend_sec > 0 diff --git a/spec/unit/commonrequire.lua b/spec/unit/commonrequire.lua index 244305401..fea1bb05e 100644 --- a/spec/unit/commonrequire.lua +++ b/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' diff --git a/spec/unit/dbg_spec.lua b/spec/unit/dbg_spec.lua new file mode 100644 index 000000000..8b3d59be5 --- /dev/null +++ b/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) diff --git a/spec/unit/font_spec.lua b/spec/unit/font_spec.lua index 4d6c155bd..5dab20e85 100644 --- a/spec/unit/font_spec.lua +++ b/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) diff --git a/spec/unit/readerbookmark_spec.lua b/spec/unit/readerbookmark_spec.lua index 61a8d859d..f568f6b96 100644 --- a/spec/unit/readerbookmark_spec.lua +++ b/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 From 144fd170f1feca3bbcd04e3b0748542360251bdd Mon Sep 17 00:00:00 2001 From: Qingping Hou Date: Tue, 29 Mar 2016 00:56:29 -0700 Subject: [PATCH 3/3] uimanager(refactor): replace autosuspend if check with noop --- .ci/script.sh | 2 +- .luacheckrc | 2 ++ Makefile | 9 ++++-- frontend/ui/uimanager.lua | 55 +++++++++++++++++++++--------------- spec/unit/uimanager_spec.lua | 42 +++++++++++++++++++++++---- 5 files changed, 77 insertions(+), 33 deletions(-) diff --git a/.ci/script.sh b/.ci/script.sh index a5233bad1..196cec7cd 100755 --- a/.ci/script.sh +++ b/.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 diff --git a/.luacheckrc b/.luacheckrc index ab850f9bb..729129542 100644 --- a/.luacheckrc +++ b/.luacheckrc @@ -1,5 +1,7 @@ unused_args = false std = "luajit" +-- ignore implicit self +self = false globals = { "G_reader_settings", diff --git a/Makefile b/Makefile index 3525f14b4..ef61be234 100644 --- a/Makefile +++ b/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 diff --git a/frontend/ui/uimanager.lua b/frontend/ui/uimanager.lua index 4e8a23f94..af850fec9 100644 --- a/frontend/ui/uimanager.lua +++ b/frontend/ui/uimanager.lua @@ -7,6 +7,7 @@ local util = require("ffi/util") 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() @@ -585,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) @@ -647,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 @@ -654,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. @@ -666,23 +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() - self.last_action_sec = util.gettime() - self:nextTick(self.auto_suspend_action) -end -dbg:guard(UIManager, '_startAutoSuspend', - function(self) - assert(self:_autoSuspendEnabled()) - end) - -function UIManager:_autoSuspendEnabled() - return Device:isKobo() and self.auto_suspend_sec > 0 -end +UIManager._resetAutoSuspendTimer = noop UIManager:init() return UIManager - diff --git a/spec/unit/uimanager_spec.lua b/spec/unit/uimanager_spec.lua index b36c9c7da..e74ed26ba 100644 --- a/spec/unit/uimanager_spec.lua +++ b/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)