refactoring readerrolling

and fix lossing progress when switching screen mode

base

@@ -1 +1 @@
-Subproject commit e8385ad081470ba8aab214a187887b7870c1ada2
+Subproject commit ce27f6ed762ec1046c94a540cc0d555eaeda9efd

frontend/apps/reader/modules/readerrolling.lua

@@ -1,4 +1,5 @@
 local InputContainer = require("ui/widget/container/inputcontainer")
+local ReaderPanning = require("apps/reader/modules/readerpanning")
 local Screen = require("ui/screen")
 local Device = require("ui/device")
 local Geom = require("ui/geometry")
@@ -8,8 +9,27 @@ local GestureRange = require("ui/gesturerange")
 local UIManager = require("ui/uimanager")
 local DEBUG = require("dbg")
 local _ = require("gettext")
-local ReaderPanning = require("apps/reader/modules/readerpanning")
 
+--[[
+    Rolling is just like paging in page-based documents except that
+    sometimes (in scroll mode) there is no concept of page number to indicate
+    current progress.
+    There are three kind of progress measurements for credocuments.
+    1. page number (in page mode)
+    2. progress percentage (in scroll mode)
+    3. xpointer (in document dom structure)
+    We found that the first two measurements are not suitable for keeping a
+    record of the real progress. For example, when switching screen orientation
+    from portrait to landscape, or switching view mode from page to scroll, the
+    internal xpointer should not be used as the view dimen/mode is changed and
+    crengine's pagination mechanism will find a closest xpointer for the new view.
+    So if we change the screen orientation or view mode back, we cannot find the
+    original place since the internal xpointer is changed, which is counter-
+    intuitive as users didn't goto any other page.
+    The solution is that we keep a record of the internal xpointer and only change
+    it in explicit page turning. And use that xpointer for non-page-turning
+    rendering.
+--]]
 local ReaderRolling = InputContainer:new{
     old_doc_height = nil,
     old_page = nil,
@@ -17,6 +37,7 @@ local ReaderRolling = InputContainer:new{
     -- only used for page view mode
     current_page= nil,
     doc_height = nil,
+    xpointer = nil,
     panning_steps = ReaderPanning.panning_steps,
     show_overlap_enable = true,
     overlap = 20,
@@ -152,36 +173,36 @@ function ReaderRolling:onReadSettings(config)
         self.show_overlap_enable = soe
     end
     local last_xp = config:readSetting("last_xpointer")
+    local last_per = config:readSetting("last_percent")
     if last_xp then
         table.insert(self.ui.postInitCallback, function()
-            self:gotoXPointer(last_xp)
+            self.xpointer = last_xp
+            self:gotoXPointer(self.xpointer)
             -- we have to do a real jump in self.ui.document._document to
             -- update status information in CREngine.
-            self.ui.document:gotoXPointer(last_xp)
+            self.ui.document:gotoXPointer(self.xpointer)
         end)
-    end
     -- we read last_percent just for backward compatibility
-    if not last_xp then
-        local last_per = config:readSetting("last_percent")
-        if 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
-                -- to update status information in CREngine.
-                self.ui.document:gotoPos(self.current_pos)
-            end)
+    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
+            -- to update status information in CREngine.
+            self.ui.document:gotoPos(self.current_pos)
+            self.xpointer = self.ui.document:getXPointer()
+        end)
+    else
+        if self.view.view_mode == "page" then
+            self.ui:handleEvent(Event:new("PageUpdate", 1))
         end
+        self.xpointer = self.ui.document:getXPointer()
     end
-    if self.view.view_mode == "page" then
-        self.ui:handleEvent(Event:new("PageUpdate", self.ui.document:getCurrentPage()))
-    end
-    self.ui:handleEvent(Event:new("UpdatePos"))
 end
 
 function ReaderRolling:onSaveSettings()
     -- remove last_percent config since its deprecated
     self.ui.doc_settings:saveSetting("last_percent", nil)
-    self.ui.doc_settings:saveSetting("last_xpointer", self.ui.document:getXPointer())
+    self.ui.doc_settings:saveSetting("last_xpointer", self.xpointer)
     self.ui.doc_settings:saveSetting("percent_finished", self:getLastPercent())
 end
 
@@ -247,7 +268,8 @@ function ReaderRolling:onResume()
 end
 
 function ReaderRolling:onDoubleTapForward()
-    local pageno = self.current_page + self.ui.document:getVisiblePageCount()
+    local visible_page_count = self.ui.document:getVisiblePageCount()
+    local pageno = self.current_page + (visible_page_count > 1 and 1 or 0)
     self:onGotoPage(self.ui.toc:getNextChapter(pageno, 0))
     return true
 end
@@ -268,6 +290,14 @@ function ReaderRolling:onGotoPercent(percent)
     return true
 end
 
+function ReaderRolling:onGotoPage(number)
+    if number then
+        self:gotoPage(number)
+    end
+    self.xpointer = self.ui.document:getXPointer()
+    return true
+end
+
 function ReaderRolling:onGotoViewRel(diff)
     DEBUG("goto relative screen:", diff, ", in mode: ", self.view.view_mode)
     if self.view.view_mode == "scroll" then
@@ -284,6 +314,7 @@ function ReaderRolling:onGotoViewRel(diff)
         local page_count = self.ui.document:getVisiblePageCount()
         self:gotoPage(self.current_page + diff*page_count)
     end
+    self.xpointer = self.ui.document:getXPointer()
     return true
 end
 
@@ -292,6 +323,7 @@ function ReaderRolling:onPanning(args, key)
     local _, dy = unpack(args)
     DEBUG("key =", key)
     self:gotoPos(self.current_pos + dy * self.panning_steps.normal)
+    self.xpointer = self.ui.document:getXPointer()
     return true
 end
 
@@ -303,6 +335,7 @@ end
 --[[
     remember to signal this event when the document has been zoomed,
     font has been changed, or line height has been changed.
+    Note that xpointer should not be changed.
 --]]
 function ReaderRolling:onUpdatePos()
     UIManager:scheduleIn(0.1, function () self:updatePos() end)
@@ -316,7 +349,7 @@ function ReaderRolling:updatePos()
     local new_height = self.ui.document.info.doc_height
     local new_page = self.ui.document.info.number_of_pages
     if self.old_doc_height ~= new_height or self.old_page ~= new_page then
-        self:gotoXPointer(self.ui.document:getXPointer())
+        self:gotoXPointer(self.xpointer)
         self.old_doc_height = new_height
         self.old_page = new_page
         self.ui:handleEvent(Event:new("UpdateToc"))
@@ -324,6 +357,7 @@ function ReaderRolling:updatePos()
     UIManager.repaint_all = true
 end
 
+-- FIXME: there should no other way to update xpointer
 function ReaderRolling:onUpdateXPointer()
     local xp = self.ui.document:getXPointer()
     if self.view.view_mode == "page" then
@@ -334,16 +368,20 @@ function ReaderRolling:onUpdateXPointer()
     return true
 end
 
+--[[
+    switching screen mode should not change current page number
+--]]
 function ReaderRolling:onChangeViewMode()
     self.ui.document:_readMetadata()
     self.old_doc_height = self.ui.document.info.doc_height
     self.old_page = self.ui.document.info.number_of_pages
     self.ui:handleEvent(Event:new("UpdateToc"))
-    self:gotoXPointer(self.ui.document:getXPointer())
-    if self.view.view_mode == "scroll" then
-        self.current_pos = self.ui.document:getCurrentPos()
+    if self.xpointer then
+        self:gotoXPointer(self.xpointer)
     else
-        self.current_page = self.ui.document:getCurrentPage()
+        table.insert(self.ui.postInitCallback, function()
+            self:gotoXPointer(self.xpointer)
+        end)
     end
     return true
 end
@@ -357,15 +395,17 @@ function ReaderRolling:onRedrawCurrentView()
     return true
 end
 
-function ReaderRolling:onSetDimensions()
+function ReaderRolling:onSetDimensions(dimen)
     -- update listening according to new screen dimen
     if Device:isTouchDevice() then
         self:initGesListener()
     end
+    self.ui.document:setViewDimen(Screen:getSize())
 end
 
 function ReaderRolling:onChangeScreenMode(mode)
     self.ui:handleEvent(Event:new("SetScreenMode", mode))
+    self.ui.document:setViewDimen(Screen:getSize())
     self:onChangeViewMode()
     self:onUpdatePos()
 end
@@ -392,6 +432,10 @@ function ReaderRolling:gotoPos(new_pos)
     self.ui:handleEvent(Event:new("PosUpdate", new_pos))
 end
 
+function ReaderRolling:gotoPercent(new_percent)
+    self:gotoPos(new_percent * self.doc_height / 10000)
+end
+
 function ReaderRolling:gotoPage(new_page)
     self.ui.document:gotoPage(new_page)
     self.ui:handleEvent(Event:new("PageUpdate", self.ui.document:getCurrentPage()))
@@ -405,17 +449,6 @@ function ReaderRolling:gotoXPointer(xpointer)
     end
 end
 
-function ReaderRolling:gotoPercent(new_percent)
-    self:gotoPos(new_percent * self.doc_height / 10000)
-end
-
-function ReaderRolling:onGotoPage(number)
-    if number then
-        self:gotoPage(number)
-    end
-    return true
-end
-
 --[[
 currently we don't need to get page links on each page/pos update
 since we can check link on the fly when tapping on the screen

frontend/document/credocument.lua

@@ -330,6 +330,11 @@ function CreDocument:setViewMode(new_mode)
     end
 end
 
+function CreDocument:setViewDimen(dimen)
+    DEBUG("CreDocument: set view dimen", dimen)
+    self._document:setViewDimen(dimen.w, dimen.h)
+end
+
 function CreDocument:setHeaderFont(new_font)
     if new_font then
         DEBUG("CreDocument: set header font", new_font)

spec/unit/readerrolling_spec.lua

@@ -0,0 +1,109 @@
+require("commonrequire")
+local DocumentRegistry = require("document/documentregistry")
+local ReaderUI = require("apps/reader/readerui")
+local Event = require("ui/event")
+local DEBUG = require("dbg")
+
+describe("Readerrolling module", function()
+    local sample_epub = "spec/front/unit/data/juliet.epub"
+    local readerui = ReaderUI:new{
+        document = DocumentRegistry:openDocument(sample_epub),
+    }
+    local rolling = readerui.rolling
+    describe("test in portrait screen mode", function()
+        it("should goto portrait screen mode", function()
+            readerui:handleEvent(Event:new("ChangeScreenMode", "portrait"))
+        end)
+        it("should goto certain page", function()
+            for i = 1, 10, 5 do
+                rolling:gotoPage(i)
+                assert.are.same(i, rolling.current_page)
+            end
+        end)
+        it("should goto relative page", function()
+            for i = 20, 40, 5 do
+                rolling:gotoPage(i)
+                rolling:onGotoViewRel(1)
+                assert.are.same(i + 1, rolling.current_page)
+                rolling:onGotoViewRel(-1)
+                assert.are.same(i, rolling.current_page)
+            end
+        end)
+        it("should goto next chapter", function()
+            local toc = readerui.toc
+            for i = 30, 50, 5 do
+                rolling:gotoPage(i)
+                rolling:onDoubleTapForward()
+                assert.are.same(toc:getNextChapter(i, 0), rolling.current_page)
+            end
+        end)
+        it("should goto previous chapter", function()
+            local toc = readerui.toc
+            for i = 60, 80, 5 do
+                rolling:gotoPage(i)
+                rolling:onDoubleTapBackward()
+                assert.are.same(toc:getPreviousChapter(i, 0), rolling.current_page)
+            end
+        end)
+    end)
+    describe("test in landscape screen mode", function()
+        it("should go to landscape screen mode", function()
+            readerui:handleEvent(Event:new("ChangeScreenMode", "landscape"))
+        end)
+        it("should goto certain page", function()
+            for i = 1, 10, 5 do
+                rolling:gotoPage(i)
+                assert.are.same(i, rolling.current_page)
+            end
+        end)
+        it("should goto relative page", function()
+            for i = 20, 40, 5 do
+                rolling:gotoPage(i)
+                rolling:onGotoViewRel(1)
+                assert.are.same(i + 1, rolling.current_page)
+                rolling:onGotoViewRel(-1)
+                assert.are.same(i, rolling.current_page)
+            end
+        end)
+        it("should goto next chapter", function()
+            local toc = readerui.toc
+            for i = 30, 50, 5 do
+                rolling:gotoPage(i)
+                rolling:onDoubleTapForward()
+                assert.are.same(toc:getNextChapter(i, 0), rolling.current_page)
+            end
+        end)
+        it("should goto previous chapter", function()
+            local toc = readerui.toc
+            for i = 60, 80, 5 do
+                rolling:gotoPage(i)
+                rolling:onDoubleTapBackward()
+                assert.are.same(toc:getPreviousChapter(i, 0), rolling.current_page)
+            end
+        end)
+    end)
+    describe("switching screen mode should not change current page number", function()
+        it("for portrait-landscape-portrait switching", function()
+            for i = 80, 100, 10 do
+                readerui:handleEvent(Event:new("ChangeScreenMode", "portrait"))
+                rolling:onGotoPage(i)
+                assert.are.same(i, rolling.current_page)
+                readerui:handleEvent(Event:new("ChangeScreenMode", "landscape"))
+                assert.are_not.same(i, rolling.current_page)
+                readerui:handleEvent(Event:new("ChangeScreenMode", "portrait"))
+                assert.are.same(i, rolling.current_page)
+            end
+        end)
+        it("for landscape-portrait-landscape switching", function()
+            for i = 110, 130, 10 do
+                readerui:handleEvent(Event:new("ChangeScreenMode", "landscape"))
+                rolling:onGotoPage(i)
+                assert.are.same(i, rolling.current_page)
+                readerui:handleEvent(Event:new("ChangeScreenMode", "portrait"))
+                assert.are_not.same(i, rolling.current_page)
+                readerui:handleEvent(Event:new("ChangeScreenMode", "landscape"))
+                assert.are.same(i, rolling.current_page)
+            end
+        end)
+    end)
+end)