diff --git a/frontend/apps/reader/modules/readerbookmark.lua b/frontend/apps/reader/modules/readerbookmark.lua index edfb03ae4..cbd7743ae 100644 --- a/frontend/apps/reader/modules/readerbookmark.lua +++ b/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 diff --git a/frontend/apps/reader/modules/readerhighlight.lua b/frontend/apps/reader/modules/readerhighlight.lua index 2d0dca710..21d0f71e8 100644 --- a/frontend/apps/reader/modules/readerhighlight.lua +++ b/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() diff --git a/frontend/document/credocument.lua b/frontend/document/credocument.lua index 3e4a7f0eb..6810e8178 100644 --- a/frontend/document/credocument.lua +++ b/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 diff --git a/spec/unit/readerbookmark_spec.lua b/spec/unit/readerbookmark_spec.lua new file mode 100644 index 000000000..10764fac7 --- /dev/null +++ b/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)