refactoring readerbookmark to show both bookmarks and highlights

in the bookmark menu
and use binary search of the sorted bookmarks table whenever is
possible.

frontend/apps/reader/modules/readerbookmark.lua

@@ -42,12 +42,80 @@ function ReaderBookmark:init()
     self.ui.menu:registerToMainMenu(self)
 end
 
+function ReaderBookmark:addToMainMenu(tab_item_table)
+    -- insert table to main reader menu
+    table.insert(tab_item_table.navi, {
+        text = self.bm_menu_title,
+        callback = function()
+            self:onShowBookmark()
+        end,
+    })
+end
+
+function ReaderBookmark:isBookmarkInTimeOrder(a, b)
+    return a.datetime > b.datetime
+end
+
+function ReaderBookmark:isBookmarkInPageOrder(a, b)
+    if self.ui.document.info.has_pages then
+        return a.page > b.page
+    else
+        return self.ui.document:getPageFromXPointer(a.page) >
+                self.ui.document:getPageFromXPointer(b.page)
+    end
+end
+
+function ReaderBookmark:isBookmarkInReversePageOrder(a, b)
+    if self.ui.document.info.has_pages then
+        return a.page < b.page
+    else
+        return self.ui.document:getPageFromXPointer(a.page) <
+                self.ui.document:getPageFromXPointer(b.page)
+    end
+end
+
+function ReaderBookmark:fixBookmarkSort(config)
+    -- for backward compatibility, since previously bookmarks for credocuments
+    -- are not well sorted. We need to do a whole sorting for at least once.
+    if not config:readSetting("bookmarks_sorted") then
+        table.sort(self.bookmarks, function(a, b)
+            return self:isBookmarkInPageOrder(a, b)
+        end)
+    end
+end
+
+function ReaderBookmark:importSavedHighlight(config)
+    local textmarks = config:readSetting("highlight") or {}
+    -- import saved highlight once, because from now on highlight are added to
+    -- bookmarks when they are created.
+    if not config:readSetting("highlights_imported") then
+        for page, marks in pairs(textmarks) do
+            for _, mark in ipairs(marks) do
+                self:addBookmark({
+                    page = self.ui.document.info.has_pages and page or mark.pos0,
+                    datetime = mark.datetime,
+                    notes = mark.text,
+                    highlighted = true,
+                })
+            end
+        end
+    end
+end
+
 function ReaderBookmark:onReadSettings(config)
     self.bookmarks = config:readSetting("bookmarks") or {}
+    -- need to do this after initialization because checking xpointer
+    -- may cause segfaults before credocuments are inited.
+    self.ui:registerPostInitCallback(function()
+        self:fixBookmarkSort(config)
+        self:importSavedHighlight(config)
+    end)
 end
 
 function ReaderBookmark:onSaveSettings()
     self.ui.doc_settings:saveSetting("bookmarks", self.bookmarks)
+    self.ui.doc_settings:saveSetting("bookmarks_sorted", true)
+    self.ui.doc_settings:saveSetting("highlights_imported", true)
 end
 
 function ReaderBookmark:onToggleBookmark()
@@ -64,7 +132,7 @@ function ReaderBookmark:onToggleBookmark()
 end
 
 function ReaderBookmark:setDogearVisibility(pn_or_xp)
-    if self:isBookmarked(pn_or_xp) then
+    if self:getDogearBookmarkIndex(pn_or_xp) then
         self.ui:handleEvent(Event:new("SetDogearVisibility", true))
     else
         self.ui:handleEvent(Event:new("SetDogearVisibility", false))
@@ -75,15 +143,17 @@ function ReaderBookmark:onPageUpdate(pageno)
     if self.ui.document.info.has_pages then
         self:setDogearVisibility(pageno)
     else
-        -- FIXME: this is a dirty hack to prevent crash in isXPointerInCurrentPage
-        if pageno ~= 1 then
-            self:setDogearVisibility("dummy")
-        end
+        self:setDogearVisibility(self.ui.document:getXPointer())
     end
 end
 
 function ReaderBookmark:onPosUpdate(pos)
-    self:setDogearVisibility("dummy")
+    self:setDogearVisibility(self.ui.document:getXPointer())
+end
+
+function ReaderBookmark:gotoBookmark(pn_or_xp)
+    local event = self.ui.document.info.has_pages and "GotoPage" or "GotoXPointer"
+    self.ui:handleEvent(Event:new(event, pn_or_xp))
 end
 
 function ReaderBookmark:onShowBookmark()
@@ -91,8 +161,8 @@ function ReaderBookmark:onShowBookmark()
     for k, v in ipairs(self.bookmarks) do
         local page = v.page
         -- for CREngine, bookmark page is xpointer
-        if type(page) == "string" then
-            page = self.ui.document:getPageFromXPointer(v.page)
+        if not self.ui.document.info.has_pages then
+            page = self.ui.document:getPageFromXPointer(page)
         end
         v.text = _("Page") .. " " .. page .. " " .. v.notes .. " @ " .. v.datetime
     end
@@ -101,6 +171,7 @@ function ReaderBookmark:onShowBookmark()
         title = _("Bookmarks"),
         item_table = self.bookmarks,
         is_borderless = true,
+        is_popout = false,
         width = Screen:getWidth(),
         height = Screen:getHeight(),
         cface = Font:getFace("cfont", 20),
@@ -117,126 +188,129 @@ function ReaderBookmark:onShowBookmark()
         }
     }
 
-    local menu_container = CenterContainer:new{
+    self.bookmark_menu = CenterContainer:new{
         dimen = Screen:getSize(),
         bm_menu,
     }
 
     -- buid up menu widget method as closure
-    local doc = self.ui.document
-    local view = self.view
-    local sendEv = function(ev)
-        self.ui:handleEvent(ev)
-    end
+    local bookmark = self
     function bm_menu:onMenuChoice(item)
-        if doc.info.has_pages then
-            sendEv(Event:new("PageUpdate", item.page))
-        elseif view.view_mode == "page" then
-            sendEv(Event:new("PageUpdate", doc:getPageFromXPointer(item.page)))
-        else
-            sendEv(Event:new("PosUpdate", doc:getPosFromXPointer(item.page)))
-        end
+        bookmark:gotoBookmark(item.page)
     end
 
     bm_menu.close_callback = function()
-        UIManager:close(menu_container)
+        UIManager:close(self.bookmark_menu)
     end
 
-    bm_menu.show_parent = menu_container
+    bm_menu.show_parent = self.bookmark_menu
 
-    UIManager:show(menu_container)
+    UIManager:show(self.bookmark_menu)
     return true
 end
 
-function ReaderBookmark:addToMainMenu(tab_item_table)
-    -- insert table to main reader menu
-    table.insert(tab_item_table.navi, {
-        text = self.bm_menu_title,
-        callback = function()
-            self:onShowBookmark()
-        end,
-    })
+function ReaderBookmark:isBookmarkMatch(item, pn_or_xp)
+    if self.ui.document.info.has_pages then
+        return item.page == pn_or_xp
+    else
+        return self.ui.document:isXPointerInCurrentPage(item.page)
+    end
 end
 
-function ReaderBookmark:isBookmarked(pn_or_xp)
-    for k,v in ipairs(self.bookmarks) do
-        if (type(pn_or_xp) == "number" and v.page == pn_or_xp) or
-            (type(pn_or_xp) == "string" and self.ui.document:isXPointerInCurrentPage(v.page)) then
-            return true
+function ReaderBookmark:getDogearBookmarkIndex(pn_or_xp)
+    local _start, _middle, _end = 1, 1, #self.bookmarks
+    while _start <= _end do
+        _middle = math.floor((_start + _end)/2)
+        local v = self.bookmarks[_middle]
+        if self:isBookmarkMatch(v, pn_or_xp) then
+            if v.highlighted then
+                return
+            else
+                return _middle
+            end
+        elseif self:isBookmarkInPageOrder({page = pn_or_xp}, v) then
+            _end = _middle - 1
+        else
+            _start = _middle + 1
         end
     end
-    return false
 end
 
-function ReaderBookmark:addBookmark(pn_or_xp)
-    -- build notes from TOC
-    local notes = self.ui.toc:getTocTitleByPage(pn_or_xp)
-    if notes ~= "" then
-        notes = "in "..notes
+-- binary insert of sorted bookmarks
+function ReaderBookmark:addBookmark(item)
+    local _start, _middle, _end, direction = 1, 1, #self.bookmarks, 0
+    while _start <= _end do
+        local v = self.bookmarks[_middle]
+        _middle = math.floor((_start + _end)/2)
+        if self:isBookmarkInPageOrder(item, self.bookmarks[_middle]) then
+            _end, direction = _middle - 1, 0
+        else
+            _start, direction = _middle + 1, 1
+        end
     end
-    mark_item = {
-        page = pn_or_xp,
-        datetime = os.date("%Y-%m-%d %H:%M:%S"),
-        notes = notes,
-    }
-    table.insert(self.bookmarks, mark_item)
-    table.sort(self.bookmarks, function(a,b)
-        return self:isBookmarkInSequence(a, b)
-    end)
-    return true
+    table.insert(self.bookmarks, _middle + direction, item)
 end
 
-function ReaderBookmark:isBookmarkInSequence(a, b)
-    return a.page < b.page
+-- binary search to remove bookmark
+function ReaderBookmark:removeBookmark(item)
+    local _start, _middle, _end = 1, 1, #self.bookmarks
+    while _start <= _end do
+        _middle = math.floor((_start + _end)/2)
+        local v = self.bookmarks[_middle]
+        if item.datetime == v.datetime and item.page == v.page then
+            return table.remove(self.bookmarks, _middle)
+        elseif self:isBookmarkInPageOrder(item, v) then
+            _end = _middle - 1
+        else
+            _start = _middle + 1
+        end
+    end
 end
 
 function ReaderBookmark:toggleBookmark(pn_or_xp)
-    for k,v in ipairs(self.bookmarks) do
-        if (type(pn_or_xp) == "number" and v.page == pn_or_xp) or
-            (type(pn_or_xp) == "string" and self.ui.document:isXPointerInCurrentPage(v.page)) then
-            table.remove(self.bookmarks, k)
-            return
+    local index = self:getDogearBookmarkIndex(pn_or_xp)
+    if index then
+        table.remove(self.bookmarks, index)
+    else
+        -- build notes from TOC
+        local notes = self.ui.toc:getTocTitleByPage(pn_or_xp)
+        if notes ~= "" then
+            notes = "in "..notes
         end
+        self:addBookmark({
+            page = pn_or_xp,
+            datetime = os.date("%Y-%m-%d %H:%M:%S"),
+            notes = notes,
+        })
     end
-    self:addBookmark(pn_or_xp)
 end
 
 function ReaderBookmark:getPreviousBookmarkedPage(pn_or_xp)
-    for i = #self.bookmarks, 1, -1 do
-        if pn_or_xp > self.bookmarks[i].page then
+    DEBUG("go to next bookmark from", pn_or_xp)
+    for i = 1, #self.bookmarks do
+        if self:isBookmarkInPageOrder({page = pn_or_xp}, self.bookmarks[i]) then
             return self.bookmarks[i].page
         end
     end
 end
 
 function ReaderBookmark:getNextBookmarkedPage(pn_or_xp)
-    for i = 1, #self.bookmarks do
-        if pn_or_xp < self.bookmarks[i].page then
+    DEBUG("go to next bookmark from", pn_or_xp)
+    for i = #self.bookmarks, 1, -1 do
+        if self:isBookmarkInReversePageOrder({page = pn_or_xp}, self.bookmarks[i]) then
             return self.bookmarks[i].page
         end
     end
 end
 
 function ReaderBookmark:onGotoPreviousBookmark(pn_or_xp)
-    self:GotoBookmark(self:getPreviousBookmarkedPage(pn_or_xp))
+    self:gotoBookmark(self:getPreviousBookmarkedPage(pn_or_xp))
     return true
 end
 
 function ReaderBookmark:onGotoNextBookmark(pn_or_xp)
-    self:GotoBookmark(self:getNextBookmarkedPage(pn_or_xp))
+    self:gotoBookmark(self:getNextBookmarkedPage(pn_or_xp))
     return true
 end
 
-function ReaderBookmark:GotoBookmark(pn_or_xp)
-    if type(pn_or_xp) == "string" then
-        if self.view.view_mode == "page" then
-            self.ui:handleEvent(Event:new("PageUpdate", self.ui.document:getPageFromXPointer(pn_or_xp)))
-        else
-            self.ui:handleEvent(Event:new("PosUpdate", self.ui.document:getPosFromXPointer(pn_or_xp)))
-        end
-    elseif type(pn_or_xp) == "number" then
-        self.ui:handleEvent(Event:new("PageUpdate", pn_or_xp))
-    end
-end
-
 return ReaderBookmark

frontend/apps/reader/modules/readerhighlight.lua

@@ -394,14 +394,22 @@ function ReaderHighlight:saveHighlight()
         if not self.view.highlight.saved[page] then
             self.view.highlight.saved[page] = {}
         end
-        local hl_item = {}
-        hl_item["text"] = self.selected_text.text
-        hl_item["pos0"] = self.selected_text.pos0
-        hl_item["pos1"] = self.selected_text.pos1
-        hl_item["pboxes"] = self.selected_text.pboxes
-        hl_item["datetime"] = os.date("%Y-%m-%d %H:%M:%S")
-        hl_item["drawer"] = self.view.highlight.saved_drawer
+        local datetime = os.date("%Y-%m-%d %H:%M:%S")
+        local hl_item = {
+            datetime = datetime,
+            text = self.selected_text.text,
+            pos0 = self.selected_text.pos0,
+            pos1 = self.selected_text.pos1,
+            pboxes = self.selected_text.pboxes,
+            drawer = self.view.highlight.saved_drawer,
+        }
         table.insert(self.view.highlight.saved[page], hl_item)
+        self.ui.bookmark:addBookmark({
+            page = self.ui.document.info.has_pages and page or self.selected_text.pos0,
+            datetime = datetime,
+            notes = self.selected_text.text,
+            highlighted = true,
+        })
         --[[
         -- disable exporting hightlights to My Clippings
         -- since it's not portable and there is a better Evernote plugin
@@ -469,7 +477,11 @@ end
 
 function ReaderHighlight:deleteHighlight(page, i)
     DEBUG("delete highlight")
-    table.remove(self.view.highlight.saved[page], i)
+    local removed = table.remove(self.view.highlight.saved[page], i)
+    self.ui.bookmark:removeBookmark({
+        page = self.ui.document.info.has_pages and removed.page or removed.pos0,
+        datetime = removed.datetime,
+    })
 end
 
 function ReaderHighlight:editHighlight()

frontend/document/credocument.lua

@@ -419,7 +419,7 @@ function CreDocument:setBatteryState(state)
 end
 
 function CreDocument:isXPointerInCurrentPage(xp)
-    DEBUG("CreDocument: check in page", xp)
+    DEBUG("CreDocument: check xpointer in current page", xp)
     return self._document:isXPointerInCurrentPage(xp)
 end
 

spec/unit/readerbookmark_spec.lua

@@ -0,0 +1,160 @@
+require("commonrequire")
+local DocumentRegistry = require("document/documentregistry")
+local ReaderUI = require("apps/reader/readerui")
+local UIManager = require("ui/uimanager")
+local Screen = require("device").screen
+local Geom = require("ui/geometry")
+local DEBUG = require("dbg")
+
+local sample_epub = "spec/front/unit/data/juliet.epub"
+local sample_pdf = "spec/front/unit/data/sample.pdf"
+
+describe("Readerbookmark module", function()
+    local function highlight_text(readerui, pos0, pos1)
+        readerui.highlight:onHold(nil, { pos = pos0 })
+        readerui.highlight:onHoldPan(nil, { pos = pos1 })
+        readerui.highlight:onHoldRelease()
+        assert.truthy(readerui.highlight.highlight_dialog)
+        readerui.highlight:onHighlight()
+        UIManager:scheduleIn(1, function()
+            UIManager:close(readerui.highlight.highlight_dialog)
+            UIManager:close(readerui)
+        end)
+        UIManager:run()
+    end
+    local function toggler_dogear(readerui)
+        readerui.bookmark:onToggleBookmark()
+        UIManager:scheduleIn(1, function()
+            UIManager:close(readerui)
+        end)
+        UIManager:run()
+    end
+    local function show_bookmark_menu(readerui)
+        UIManager:scheduleIn(1, function()
+            UIManager:close(readerui.bookmark.bookmark_menu)
+            UIManager:close(readerui)
+        end)
+        UIManager:run()
+    end
+    describe("bookmark for EPUB document", function()
+        local page = 10
+        local readerui
+        setup(function()
+            readerui = ReaderUI:new{
+                document = DocumentRegistry:openDocument(sample_epub),
+            }
+        end)
+        before_each(function()
+            UIManager:quit()
+            UIManager:show(readerui)
+            readerui.rolling:gotoPage(10)
+        end)
+        it("should show dogear after togglering non-bookmarked page", function()
+            toggler_dogear(readerui)
+            Screen:shot("screenshots/reader_bookmark_dogear_epub.png")
+            assert.truthy(readerui.view.dogear_visible)
+        end)
+        it("should not show dogear after togglering bookmarked page", function()
+            toggler_dogear(readerui)
+            Screen:shot("screenshots/reader_bookmark_nodogear_epub.png")
+            assert.truthy(not readerui.view.dogear_visible)
+        end)
+        it("should sort bookmarks with descending page numbers", function()
+            local pages = {1, 20, 5, 30, 10, 40, 15, 25, 35, 45}
+            for _, page in ipairs(pages) do
+                readerui.rolling:gotoPage(page)
+                toggler_dogear(readerui)
+            end
+            readerui.bookmark:onShowBookmark()
+            show_bookmark_menu(readerui)
+            Screen:shot("screenshots/reader_bookmark_10marks_epub.png")
+            assert.are.same(10, #readerui.bookmark.bookmarks)
+        end)
+        it("should keep descending page numbers after removing bookmarks", function()
+            local pages = {1, 30, 10, 40, 20}
+            for _, page in ipairs(pages) do
+                readerui.rolling:gotoPage(page)
+                toggler_dogear(readerui)
+            end
+            readerui.bookmark:onShowBookmark()
+            show_bookmark_menu(readerui)
+            Screen:shot("screenshots/reader_bookmark_5marks_epub.png")
+            assert.are.same(5, #readerui.bookmark.bookmarks)
+        end)
+        it("should add bookmark by highlighting", function()
+            highlight_text(readerui, Geom:new{ x = 260, y = 60 }, Geom:new{ x = 260, y = 90 })
+            readerui.bookmark:onShowBookmark()
+            show_bookmark_menu(readerui)
+            Screen:shot("screenshots/reader_bookmark_6marks_epub.png")
+            assert.are.same(6, #readerui.bookmark.bookmarks)
+        end)
+        it("should get previous bookmark for certain page", function()
+            local xpointer = readerui.document:getXPointer()
+            local bm_xpointer = readerui.bookmark:getPreviousBookmarkedPage(xpointer)
+            assert.are.same(5, readerui.document:getPageFromXPointer(bm_xpointer))
+        end)
+        it("should get next bookmark for certain page", function()
+            local xpointer = readerui.document:getXPointer()
+            local bm_xpointer = readerui.bookmark:getNextBookmarkedPage(xpointer)
+            assert.are.same(15, readerui.document:getPageFromXPointer(bm_xpointer))
+        end)
+    end)
+    describe("bookmark for PDF document", function()
+        local readerui
+        setup(function()
+            readerui = ReaderUI:new{
+                document = DocumentRegistry:openDocument(sample_pdf),
+            }
+        end)
+        before_each(function()
+            UIManager:quit()
+            UIManager:show(readerui)
+            readerui.paging:gotoPage(10)
+        end)
+        it("should show dogear after togglering non-bookmarked page", function()
+            toggler_dogear(readerui)
+            Screen:shot("screenshots/reader_bookmark_dogear_pdf.png")
+            assert.truthy(readerui.view.dogear_visible)
+        end)
+        it("should not show dogear after togglering bookmarked page", function()
+            toggler_dogear(readerui)
+            Screen:shot("screenshots/reader_bookmark_nodogear_pdf.png")
+            assert.truthy(not readerui.view.dogear_visible)
+        end)
+        it("should sort bookmarks with descending page numbers", function()
+            local pages = {1, 20, 5, 30, 10, 40, 15, 25, 35, 45}
+            for _, page in ipairs(pages) do
+                readerui.paging:gotoPage(page)
+                toggler_dogear(readerui)
+            end
+            readerui.bookmark:onShowBookmark()
+            show_bookmark_menu(readerui)
+            Screen:shot("screenshots/reader_bookmark_10marks_pdf.png")
+            assert.are.same(10, #readerui.bookmark.bookmarks)
+        end)
+        it("should keep descending page numbers after removing bookmarks", function()
+            local pages = {1, 30, 10, 40, 20}
+            for _, page in ipairs(pages) do
+                readerui.paging:gotoPage(page)
+                toggler_dogear(readerui)
+            end
+            readerui.bookmark:onShowBookmark()
+            show_bookmark_menu(readerui)
+            Screen:shot("screenshots/reader_bookmark_5marks_pdf.png")
+            assert.are.same(5, #readerui.bookmark.bookmarks)
+        end)
+        it("should add bookmark by highlighting", function()
+            highlight_text(readerui, Geom:new{ x = 260, y = 70 }, Geom:new{ x = 260, y = 150 })
+            readerui.bookmark:onShowBookmark()
+            show_bookmark_menu(readerui)
+            Screen:shot("screenshots/reader_bookmark_6marks_pdf.png")
+            assert.are.same(6, #readerui.bookmark.bookmarks)
+        end)
+        it("should get previous bookmark for certain page", function()
+            assert.are.same(5, readerui.bookmark:getPreviousBookmarkedPage(10))
+        end)
+        it("should get next bookmark for certain page", function()
+            assert.are.same(15, readerui.bookmark:getNextBookmarkedPage(10))
+        end)
+    end)
+end)