readerhighlight: remove selected_word and use selected_text everywhere

There were two ways of specifing selected text for a highlight depending
on whether it was a "single word" or text selected using hold-and-pan.
In addition to being a bit more complicated than is necessary, with the
addition of the language support plugin system (where the "single word"
selected might be expanded), it makes more sense to simply use the same
logic and table structure for both cases.

The dictionary lookup special case (hold-without-pan triggering a
dictionary lookup by default) still works as before.

In addition, this patch fixes a minor inefficiency during dictionary
quick lookup -- before this patch, the highlight would be re-selected
because the quick lookup window is run concurrently and tries to fetch
ReaderHighlight.selected_text but this is set to nil immediately after
triggering the lookup. This is unnecessary because :clear() will be
called anyway when the quick pop-up closes, and so clearing this can be
left until then.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

frontend/apps/reader/modules/readerdictionary.lua

@@ -398,8 +398,8 @@ function ReaderDictionary:addToMainMenu(menu_items)
     end
 end
 
-function ReaderDictionary:onLookupWord(word, is_sane, box, highlight, link)
+function ReaderDictionary:onLookupWord(word, is_sane, boxes, highlight, link)
-    logger.dbg("dict lookup word:", word, box)
+    logger.dbg("dict lookup word:", word, boxes)
     -- escape quotes and other funny characters in word
     word = self:cleanSelection(word, is_sane)
     logger.dbg("dict stripped word:", word)
@@ -408,7 +408,7 @@ function ReaderDictionary:onLookupWord(word, is_sane, box, highlight, link)
 
     -- Wrapped through Trapper, as we may be using Trapper:dismissablePopen() in it
     Trapper:wrap(function()
-        self:stardictLookup(word, self.enabled_dict_names, not self.disable_fuzzy_search, box, link)
+        self:stardictLookup(word, self.enabled_dict_names, not self.disable_fuzzy_search, boxes, link)
     end)
     return true
 end
@@ -447,7 +447,7 @@ function ReaderDictionary:onHtmlDictionaryLinkTapped(dictionary, link)
 
     -- Wrapped through Trapper, as we may be using Trapper:dismissablePopen() in it
     Trapper:wrap(function()
-        self:stardictLookup(word, {dictionary}, false, link_box, nil)
+        self:stardictLookup(word, {dictionary}, false, {link_box}, nil)
     end)
 end
 
@@ -815,7 +815,7 @@ function ReaderDictionary:startSdcv(word, dict_names, fuzzy_search)
     return final_results
 end
 
-function ReaderDictionary:stardictLookup(word, dict_names, fuzzy_search, box, link)
+function ReaderDictionary:stardictLookup(word, dict_names, fuzzy_search, boxes, link)
     if word == "" then
         return
     end
@@ -859,7 +859,7 @@ function ReaderDictionary:stardictLookup(word, dict_names, fuzzy_search, box, li
                 lookup_cancelled = false,
             }
         }
-        self:showDict(word, nope, box, link)
+        self:showDict(word, nope, boxes, link)
         return
     end
 
@@ -877,10 +877,10 @@ function ReaderDictionary:stardictLookup(word, dict_names, fuzzy_search, box, li
         return
     end
 
-    self:showDict(word, tidyMarkup(results), box, link)
+    self:showDict(word, tidyMarkup(results), boxes, link)
 end
 
-function ReaderDictionary:showDict(word, results, box, link)
+function ReaderDictionary:showDict(word, results, boxes, link)
     if results and results[1] then
         logger.dbg("showing quick lookup window", word, results)
         self.dict_window = DictQuickLookup:new{
@@ -893,7 +893,7 @@ function ReaderDictionary:showDict(word, results, box, link)
             -- selected link, if any
             selected_link = link,
             results = results,
-            word_box = box,
+            word_boxes = boxes,
             preferred_dictionaries = self.preferred_dictionaries,
             -- differentiate between dict and wiki
             is_wiki = self.is_wiki,

frontend/apps/reader/modules/readerhighlight.lua

@@ -2,6 +2,7 @@ local BD = require("ui/bidi")
 local ButtonDialog = require("ui/widget/buttondialog")
 local Device = require("device")
 local Event = require("ui/event")
+local Geom = require("ui/geometry")
 local InfoMessage = require("ui/widget/infomessage")
 local InputContainer = require("ui/widget/container/inputcontainer")
 local Notification = require("ui/widget/notification")
@@ -436,6 +437,7 @@ function ReaderHighlight:clear(clear_id)
         self.restore_page_mode_func()
         self.restore_page_mode_func = nil
     end
+    self.is_word_selection = false
     self.selected_text_start_xpointer = nil
     if self.hold_pos then
         self.hold_pos = nil
@@ -844,7 +846,16 @@ function ReaderHighlight:onHold(arg, ges)
     local ok, word = pcall(self.ui.document.getWordFromPosition, self.ui.document, self.hold_pos)
     if ok and word then
         logger.dbg("selected word:", word)
-        self.selected_word = word
+        -- Convert "word selection" table to "text selection" table because we
+        -- use text selections throughout readerhighlight.
+        self.is_word_selection = true
+        self.selected_text = {
+            text = word.word or "",
+            pos0 = word.pos0 or word.pos,
+            pos1 = word.pos1 or word.pos,
+            sboxes = word.sbox and { word.sbox },
+            pboxes = word.pbox and { word.pbox },
+        }
         local link = self.ui.link:getLinkFromGes(ges)
         self.selected_link = nil
         if link then
@@ -852,15 +863,13 @@ function ReaderHighlight:onHold(arg, ges)
             self.selected_link = link
         end
         if self.ui.document.info.has_pages then
-            local boxes = {}
+            self.view.highlight.temp[self.hold_pos.page] = self.selected_text.sboxes
-            table.insert(boxes, self.selected_word.sbox)
-            self.view.highlight.temp[self.hold_pos.page] = boxes
             -- Unfortunately, getWordFromPosition() may not return good coordinates,
             -- so refresh the whole page
             UIManager:setDirty(self.dialog, "ui")
         else
-            -- With crengine, getWordFromPosition() does return good coordinates
+            -- With crengine, getWordFromPosition() does return good coordinates.
-            UIManager:setDirty(self.dialog, "ui", self.selected_word.sbox)
+            UIManager:setDirty(self.dialog, "ui", Geom.boundingBox(self.selected_text.sboxes))
         end
         self:_resetHoldTimer()
         if word.pos0 then
@@ -992,6 +1001,7 @@ function ReaderHighlight:onHoldPan(_, ges)
 
     local old_text = self.selected_text and self.selected_text.text
     self.selected_text = self.ui.document:getTextFromPositions(self.hold_pos, self.holdpan_pos)
+    self.is_word_selection = false
 
     if self.selected_text and self.selected_text.pos0 then
         if not self.selected_text_start_xpointer then
@@ -1015,13 +1025,6 @@ function ReaderHighlight:onHoldPan(_, ges)
     logger.dbg("selected text:", self.selected_text)
     if self.selected_text then
         self.view.highlight.temp[self.hold_pos.page] = self.selected_text.sboxes
-        -- remove selected word if hold moves out of word box
-        if not self.selected_text.sboxes or #self.selected_text.sboxes == 0 then
-            self.selected_word = nil
-        elseif self.selected_word and not self.selected_word.sbox:contains(self.selected_text.sboxes[1]) or
-            #self.selected_text.sboxes > 1 then
-            self.selected_word = nil
-        end
     end
     UIManager:setDirty(self.dialog, "ui")
 end
@@ -1035,18 +1038,36 @@ You can download language data files for version 3.04 from https://tesseract-ocr
 
 Copy the language data files for Tesseract 3.04 (e.g., eng.traineddata for English and spa.traineddata for Spanish) into koreader/data/tessdata]])
 
-function ReaderHighlight:lookup(selected_word, selected_link)
+function ReaderHighlight:lookup(selected_text, selected_link)
+    -- convert sboxes to word boxes
+    local word_boxes = {}
+    for i, sbox in ipairs(selected_text.sboxes) do
+        word_boxes[i] = self.view:pageToScreenTransform(self.hold_pos.page, sbox)
+    end
+
     -- if we extracted text directly
-    if selected_word.word and self.hold_pos then
+    if selected_text.text and self.hold_pos then
-        local word_box = self.view:pageToScreenTransform(self.hold_pos.page, selected_word.sbox)
+        self.ui:handleEvent(Event:new("LookupWord", selected_text.text, false, word_boxes, self, selected_link))
-        self.ui:handleEvent(Event:new("LookupWord", selected_word.word, false, word_box, self, selected_link))
     -- or we will do OCR
-    elseif selected_word.sbox and self.hold_pos then
+    elseif selected_text.sboxes and self.hold_pos then
-        local word = self.ui.document:getOCRWord(self.hold_pos.page, selected_word)
+        local text = self.ui.document:getOCRText(self.hold_pos.page, selected_text.sboxes)
-        logger.dbg("OCRed word:", word)
+        if not text then
-        if word and word ~= "" then
+            -- getOCRText is not implemented in some document backends, but
-            local word_box = self.view:pageToScreenTransform(self.hold_pos.page, selected_word.sbox)
+            -- getOCRWord is implemented everywhere. As such, fall back to
-            self.ui:handleEvent(Event:new("LookupWord", word, false, word_box, self, selected_link))
+            -- getOCRWord.
+            text = ""
+            for _, sbox in ipairs(selected_text.sboxes) do
+                local word = self.ui.document:getOCRWord(self.hold_pos.page, { sbox = sbox })
+                logger.dbg("OCRed word:", word)
+                -- @fixme This might produce incorrect results on RTL text.
+                if word and word ~= "" then
+                    text = text .. word
+                end
+            end
+        end
+        logger.dbg("OCRed text:", text)
+        if text and text ~= "" then
+            self.ui:handleEvent(Event:new("LookupWord", text, false, word_boxes, self, selected_link))
         else
             UIManager:show(InfoMessage:new{
                 text = info_message_ocr_text,
@@ -1227,15 +1248,15 @@ function ReaderHighlight:onHoldRelease()
         end
         self.hold_last_tv = nil
     end
-    if self.selected_word then -- single-word selection
+    if self.is_word_selection then -- single-word selection
         if long_final_hold or G_reader_settings:isTrue("highlight_action_on_single_word") then
-            -- Force a 0-distance pan to have a self.selected_text with this word,
+            self.is_word_selection = false
-            -- which will enable the highlight menu or action instead of dict lookup
-            self:onHoldPan(nil, {pos=self.hold_ges_pos})
         end
     end
 
-    if self.selected_text then
+    if self.is_word_selection then
+        self:lookup(self.selected_text, self.selected_link)
+    else
         local default_highlight_action = G_reader_settings:readSetting("default_highlight_action", "ask")
         if long_final_hold or default_highlight_action == "ask" then
             -- bypass default action and show popup if long final hold
@@ -1257,9 +1278,6 @@ function ReaderHighlight:onHoldRelease()
             -- No self:onClose() to not remove the selected text
             -- which will have been the first search result
         end
-    elseif self.selected_word then
-        self:lookup(self.selected_word, self.selected_link)
-        self.selected_word = nil
     end
     return true
 end

frontend/ui/widget/dictquicklookup.lua

@@ -50,8 +50,8 @@ local DictQuickLookup = InputContainer:new{
     dict_index = 1,
     width = nil,
     height = nil,
-    -- box of highlighted word, quick lookup window tries to not hide the word
+    -- sboxes containing highlighted text, quick lookup window tries to not hide the word
-    word_box = nil,
+    word_boxes = nil,
 
     -- refresh_callback will be called before we trigger full refresh in onSwipe
     refresh_callback = nil,
@@ -130,7 +130,7 @@ function DictQuickLookup:init()
             --             -- don't pass self.highlight to subsequent lookup, we want
             --             -- the first to be the only one to unhighlight selection
             --             -- when closed
-            --             Event:new("LookupWord", word, self.word_box))
+            --             Event:new("LookupWord", word, true, {self.word_box}))
             --     end
             -- },
             -- Allow selection of one or more words (see textboxwidget.lua) :
@@ -619,16 +619,32 @@ function DictQuickLookup:init()
         local nb_lines = Math.round(self.definition_height / self.definition_line_height)
         self.definition_height = nb_lines * self.definition_line_height
         self.height = self.definition_height + others_height
-        if self.word_box then
+        if self.word_boxes and #self.word_boxes > 0 then
             -- Try to not hide the highlighted word. We don't want to always
             -- get near it if we can stay center, so that more context around
             -- the word is still visible with the dict result.
             -- But if we were to be drawn over the word, move a bit if possible.
-            local box = self.word_box
+
-            -- Don't stick to the box, ensure a minimal padding between box and window
+            -- In most cases boxes will be a single sbox, but handle multiple
-            local box_dict_padding = Size.padding.small
+            -- sboxes by taking the top and bottom y values.
-            local word_box_top = box.y - box_dict_padding
+            local word_box_top
-            local word_box_bottom = box.y + box.h + box_dict_padding
+            local word_box_bottom
+            for _, box in ipairs(self.word_boxes) do
+                local box_top = box.y
+                local box_bottom = box.y + box.h
+                if not word_box_top or word_box_top > box_top then
+                    word_box_top = box_top
+                end
+                if not word_box_bottom or word_box_bottom < box_bottom then
+                    word_box_bottom = box_bottom
+                end
+            end
+
+            -- Don't stick to the box, ensure a minimal padding between box and
+            -- window.
+            word_box_top = word_box_top - Size.padding.small
+            word_box_bottom = word_box_bottom + Size.padding.small
+
             local half_visible_height = (avail_height - self.height) / 2
             if word_box_bottom > half_visible_height and word_box_top <= half_visible_height + self.height then
                 -- word would be covered by our centered window