From 29708884c730feee155281b709ce3d3efed7d757 Mon Sep 17 00:00:00 2001 From: poire-z Date: Wed, 18 Oct 2017 17:19:06 +0200 Subject: [PATCH] Enable Edit (rename bookmark) when tap on highlight (#3369) Also fix a few crash possibilities when unhighlighting. Also fix bug with binary search that could not be able to remove bookmark when there are multiple bookmarks/highlights on the same page. --- .../apps/reader/modules/readerbookmark.lua | 42 ++++++++- .../apps/reader/modules/readerhighlight.lua | 92 ++++++++++++++----- 2 files changed, 105 insertions(+), 29 deletions(-) diff --git a/frontend/apps/reader/modules/readerbookmark.lua b/frontend/apps/reader/modules/readerbookmark.lua index fe293bc09..b4f5bc18e 100644 --- a/frontend/apps/reader/modules/readerbookmark.lua +++ b/frontend/apps/reader/modules/readerbookmark.lua @@ -252,7 +252,7 @@ function ReaderBookmark:onShowBookmark() end, ok_text = _("Remove"), ok_callback = function() - bookmark:removeHightligit(item) + bookmark:removeHighlight(item) bm_menu:switchItemTable(nil, bookmark.bookmarks, -1) UIManager:close(self.textviewer) end, @@ -368,7 +368,7 @@ function ReaderBookmark:isBookmarkAdded(item) return false end -function ReaderBookmark:removeHightligit(item) +function ReaderBookmark:removeHighlight(item) if item.pos0 then self.ui:handleEvent(Event:new("Unhighlight", item)) else @@ -391,9 +391,41 @@ function ReaderBookmark:removeBookmark(item) _start = _middle + 1 end end + -- If we haven't found item, it may be because there are multiple + -- bookmarks on the same page, and the above binary search decided to + -- not search on one side of one it found on page, where item could be. + -- Fallback to do a full scan. + logger.dbg("removeBookmark: binary search didn't find bookmark, doing full scan") + for i=1, #self.bookmarks do + local v = self.bookmarks[i] + if item.datetime == v.datetime and item.page == v.page then + return table.remove(self.bookmarks, i) + end + end + logger.warn("removeBookmark: full scan search didn't find bookmark") end -function ReaderBookmark:renameBookmark(item) +function ReaderBookmark:renameBookmark(item, from_highlight) + if from_highlight then + -- Called by ReaderHighlight:editHighlight, we need to find the bookmark + for i=1, #self.bookmarks do + if item.datetime == self.bookmarks[i].datetime and item.page == self.bookmarks[i].page then + item = self.bookmarks[i] + if item.text == nil or item.text == "" then + -- Make up bookmark text as done in onShowBookmark + local page = item.page + if not self.ui.document.info.has_pages then + page = self.ui.document:getPageFromXPointer(page) + end + item.text = T(_("Page %1 %2 @ %3"), page, item.notes, item.datetime) + end + break + end + end + if item.text == nil then -- bookmark not found + return + end + end self.input = InputDialog:new{ title = _("Rename bookmark"), input = item.text, @@ -417,7 +449,9 @@ function ReaderBookmark:renameBookmark(item) item.pos1 == self.bookmarks[i].pos1 and item.page == self.bookmarks[i].page then self.bookmarks[i].text = value UIManager:close(self.input) - self.refresh() + if not from_highlight then + self.refresh() + end break end end diff --git a/frontend/apps/reader/modules/readerhighlight.lua b/frontend/apps/reader/modules/readerhighlight.lua index f7aae9a7f..47d5201fa 100644 --- a/frontend/apps/reader/modules/readerhighlight.lua +++ b/frontend/apps/reader/modules/readerhighlight.lua @@ -223,9 +223,8 @@ function ReaderHighlight:onShowHighlightDialog(page, index) }, { text = _("Edit"), - enabled = false, callback = function() - self:editHighlight() + self:editHighlight(page, index) UIManager:close(self.edit_highlight_dialog) end, }, @@ -454,31 +453,65 @@ function ReaderHighlight:onHighlight() self:saveHighlight() end -function ReaderHighlight:onUnhighlight(item) +function ReaderHighlight:onUnhighlight(bookmark_item) local page local sel_text local sel_pos0 + local datetime local idx - if item then - local bookmark_text = item.text - local words = {} - for word in bookmark_text:gmatch("%S+") do table.insert(words, word) end - page = tonumber(words[2]) - sel_text = item.notes - sel_pos0 = item.pos0 - else + if bookmark_item then -- called from Bookmarks menu onHold + page = bookmark_item.page + sel_text = bookmark_item.notes + sel_pos0 = bookmark_item.pos0 + datetime = bookmark_item.datetime + else -- called from DictQuickLookup Unhighlight button page = self.hold_pos.page sel_text = self.selected_text.text sel_pos0 = self.selected_text.pos0 end - for index = 1, #self.view.highlight.saved[page] do - if self.view.highlight.saved[page][index].text == sel_text and - self.view.highlight.saved[page][index].pos0 == sel_pos0 then - idx = index - break + if self.ui.document.info.has_pages then -- We can safely use page + for index = 1, #self.view.highlight.saved[page] do + local highlight = self.view.highlight.saved[page][index] + -- pos0 are tables and can't be compared directly, except when from + -- DictQuickLookup where these are the same object. + -- If bookmark_item provided, just check datetime + if highlight.text == sel_text and ( + (datetime == nil and highlight.pos0 == sel_pos0) or + (datetime ~= nil and highlight.datetime == datetime)) then + idx = index + break + end + end + else -- page is a xpointer + -- The original page could be found in bookmark_item.text, but + -- no more if it has been renamed: we need to loop through all + -- highlights on all page slots + for p, highlights in pairs(self.view.highlight.saved) do + for index = 1, #highlights do + local highlight = highlights[index] + -- pos0 are strings and can be compared directly + if highlight.text == sel_text and ( + (datetime == nil and highlight.pos0 == sel_pos0) or + (datetime ~= nil and highlight.datetime == datetime)) then + page = p -- this is the original page slot + idx = index + break + end + end + if idx then + break + end end end - self:deleteHighlight(page, idx) + if bookmark_item and not idx then + logger.warn("unhighlight: bookmark_item not found among highlights", bookmark_item) + -- Remove it from bookmarks anyway, so we're not stuck with an + -- unremovable bookmark + self.ui.bookmark:removeBookmark(bookmark_item) + return + end + logger.dbg("found highlight to delete on page", page, idx) + self:deleteHighlight(page, idx, bookmark_item) return true end @@ -598,18 +631,27 @@ function ReaderHighlight:moreAction() logger.info("more action") end -function ReaderHighlight:deleteHighlight(page, i) +function ReaderHighlight:deleteHighlight(page, i, bookmark_item) self.ui:handleEvent(Event:new("DelHighlight")) - logger.dbg("delete highlight") + logger.dbg("delete highlight", page, i) local removed = table.remove(self.view.highlight.saved[page], i) - self.ui.bookmark:removeBookmark({ - page = self.ui.document.info.has_pages and page or removed.pos0, - datetime = removed.datetime, - }) + if bookmark_item then + self.ui.bookmark:removeBookmark(bookmark_item) + else + self.ui.bookmark:removeBookmark({ + page = self.ui.document.info.has_pages and page or removed.pos0, + datetime = removed.datetime, + }) + end end -function ReaderHighlight:editHighlight() - logger.info("edit highlight") +function ReaderHighlight:editHighlight(page, i) + logger.info("edit highlight", page, i) + local item = self.view.highlight.saved[page][i] + self.ui.bookmark:renameBookmark({ + page = self.ui.document.info.has_pages and page or item.pos0, + datetime = item.datetime, + }, true) end function ReaderHighlight:onReadSettings(config)