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.

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

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
+    if bookmark_item then -- called from Bookmarks menu onHold
-        local bookmark_text = item.text
+        page = bookmark_item.page
-        local words = {}
+        sel_text = bookmark_item.notes
-        for word in bookmark_text:gmatch("%S+") do table.insert(words, word) end
+        sel_pos0 = bookmark_item.pos0
-        page = tonumber(words[2])
+        datetime = bookmark_item.datetime
-        sel_text = item.notes
+    else -- called from DictQuickLookup Unhighlight button
-        sel_pos0 = item.pos0
-    else
         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.ui.document.info.has_pages then -- We can safely use page
-        if self.view.highlight.saved[page][index].text == sel_text and
+        for index = 1, #self.view.highlight.saved[page] do
-            self.view.highlight.saved[page][index].pos0 == sel_pos0 then
+            local highlight = self.view.highlight.saved[page][index]
-            idx = index
+            -- pos0 are tables and can't be compared directly, except when from
-            break
+            -- 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({
+    if bookmark_item then
-        page = self.ui.document.info.has_pages and page or removed.pos0,
+        self.ui.bookmark:removeBookmark(bookmark_item)
-        datetime = removed.datetime,
+    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()
+function ReaderHighlight:editHighlight(page, i)
-    logger.info("edit highlight")
+    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)