Dictionary, footnotes: highlight the selected text by TnS-hun · Pull Request #12948 · koreader/koreader

TnS-hun · 2024-12-27T09:53:57Z

When selecting text in the dictionary (including Wikipedia) or the pop-up footnotes highlight the selected text using an inverting rectangle. After closing the opened dictionary remove the highlight after a short delay just like in the reader.

Depends on koreader/koreader-base#2013

This change is

poire-z

Lots of stuff at the bottom of TextBoxWidget.lua, that looks like it's just my stuff mostly unchanged and just moved around - hard to get back into all that, so trusting you on this logic.

poire-z · 2024-12-27T18:38:24Z

+    -- If display_highlight is true then highlight will be displayed.
+    -- If false then highlight will work as it did originally: no highlight will be displayed, and the
+    -- highlight will be updated only on hold release (so hold time might be incorrect).


You need to be clearer here with "highlight" and "text selection".

Suggested change

-- If display_highlight is true then highlight will be displayed.

-- If false then highlight will work as it did originally: no highlight will be displayed, and the

-- highlight will be updated only on hold release (so hold time might be incorrect).

-- If highlight_text_selection is true then text selection will be highlighted.

-- If false then text selection will work as it did originally: no highlighting, and the

-- text selection will be updated only on hold release (so hold time might be incorrect).

Not sure I understand the second sentence: do we still get highlighting on hold release ? If not, what is updated on hold release ? And why hold time might be incorrect.

Hold time works incorrectly in the original version (and currently when the highlight display is off) because it is not reset when you change the selection, so for example if you select an entire line slowly (>3s) then Wikipedia comes up instead of the expected dictionary.

I can easily fix this to work correctly regardless of the highlight display. What do you think?

If it's indeed easy to fix I don't see why not.

Oh, indeed. Yes, if you can, please fix it :)

Done. It even made the code clearer. :)

poire-z · 2024-12-27T19:20:45Z

+    if start_line_num < 1 or end_line_num > #self.vertical_string_list then
+        return nil, nil, nil, nil


(I don't remember how all this works when we start or end text selection in a margin - and if you changed anything to this behaviour. I rarely do text selection in these text/htmlbox widgets, but in Reader, I often end them in the right or botto margin, so some people may be used to do it like me.)

Yes, it works better in Reader. I will try to match its behavior.

poire-z · 2024-12-27T19:33:14Z

+        for _, xglyph in ipairs(line.xglyphs) do
+            if xglyph.text_index >= text_start_idx and (xglyph.text_index + xglyph.cluster_len) <= text_end_idx and (not xglyph.no_drawing) then
+                if draw_line then
+                    if xglyph.x0 < line_x0 then line_x0 = xglyph.x0 end
+                    if xglyph.x1 > line_x1 then line_x1 = xglyph.x1 end
+                else
+                    draw_line = true
+                    line_x0 = xglyph.x0
+                    line_x1 = xglyph.x1
+                end
+            end
+        end


I think all this is new code - and I'm not keen on digging into that :) so trusting you.
I also don't know how this behave with BiDi text, and our text selection with fingers, and the resulting selected text in logical indices order - but I guess it's not impossible we should have multiple rectangles on a same line - your code would logically only emit one rectangle per line.
(Trying to see how this works in an Arabic with English bits EPUB, I don't get such multiple rectangles, so it's possible I screwed that recently...)
Anyway, just mentionning that; if the text selection was already crappy on BiDi text, let the highlighting be it too.

Selecting within a single Arabic line does not work correctly. It is caused by koptinterface:getTextFromBoxes, I will fix that. But otherwise I have no idea how exactly it should behave. :)

Just a sample of a single text selection on some mixed arabic/english search result:

I did not get notification for the replies. :(

Yeah, this is definitely problematic. I will have to change the drawing from per line rectangle to per word rectangles.

poire-z · 2024-12-27T19:39:49Z

Can you share some screenshots/videos of how these highlights look ?

Also in case you didn't see it, CI complained with:

Luacheck results
Checking frontend/ui/widget/textboxwidget.lua     2 warnings
    frontend/ui/widget/textboxwidget.lua:2366:41: variable start_line_num was previously defined on line 2348
    frontend/ui/widget/textboxwidget.lua:2366:57: variable end_line_num was previously defined on line 2349

TnS-hun · 2024-12-28T19:13:42Z

Can you share some screenshots/videos of how these highlights look ?

It looks like this currently:

You can notice the vertical overlap below the words "something; a trial:".

TnS-hun · 2025-01-01T16:54:26Z

I made a new commit that makes selection behave very close to Reader.

But for right-to-left text I could not achieve the same behavior when holding+panning after the end of the line. In TextBoxWidget that selects the visually last word for RTL text, but in HtmlBoxWidget it selects the entire line.

To match TextBoxWidget's selection behavior this should be "line_index, 1" but then the selection will jump between the full row and the visually last word when hitting a vertical gap. If we extend the line vertically till the next one then selection will be weird around new paragraphs. The solution might require getPageText() to add empty lines.

Also I just noticed poire-z's comment about the BiDi selection, so to that will require more changes.

poire-z · 2025-01-01T16:57:35Z


-        local text_boxes = koptinterface:getTextFromBoxes(self.page_boxes, self.hold_start_pos, self.hold_end_pos)
-        local changed = not HtmlBoxWidget:areTextBoxesEqual(self.highlight_rects, self.highlight_text, text_boxes.boxes, text_boxes.text)
+        local text, rects = HtmlBoxWidget:getSelectedText(self.page_boxes, self.hold_start_pos, self.hold_end_pos)


If you are not using self in these utilities functions, it's best to not document/expose them as method with the :, and this HtmlBoxWidget:getSelectedText() looking non-classic :)
You can either:

make them module local function if you guess we'll never have to use them from elsewhere

make the class function with a . instead of the :, so you would use here HtmlBoxWidget.getSelectedText() (which would read more classic and we would know from reading its a class function and not a method).

poire-z · 2025-01-01T17:05:12Z

First check how text selection currently work with RTL and BiDi: if it's already messy, let it be messy, no need to make the drawing better if the text selection does not correspond to what's drawn.
If you want to fix both, good luck :) Reading your latest commit, I don't even want to understand it :) If I had thought about doing that, having to think around all that, I would have given up. So, trusting you.
(If was a nightmare when I had to do that for crengine - where it matters more than with TextBoxWidget - and I think I had more info/context in crengine... see https://github.com/koreader/crengine/blob/1aa6b9ecfd4d1ad0d7013ad72fab60f16691f7a7/crengine/src/lvtinydom.cpp#L10073-L10492 in case it can give you some thoughts.)

TnS-hun · 2025-01-02T19:18:38Z

Okay, I will do that. I will check how BiDi text behaves and whether the drawing needs per word rects or not.

TnS-hun · 2025-01-05T18:18:27Z

More changes:

highlight now uses per word rectangle drawing and tries to merge the rects, it falls back to non-merged rectangles if it detects mixed direction text (both left-to-right and right-to-left). The detection is not too sophisticated.
highlight drawing now uses darkenRect instead of invertRect. It looks better but overlaps are more visible with it. I think it is not a big issue because multi-line selection is cannot be too common in the dictionary/Wikipedia lookups.
hold duration now works correctly even if highlight drawing is not enabled

I tried using MuPDF's fz_highlight_selection for highlighting but it did not look too good for Arabic texts:

benoit-pierre · 2025-01-05T18:22:15Z

I don't think darkening is good idea, IMHO invert is the only option that should not mess too much with contrast and work in all cases, no matter the text / background color.

TnS-hun · 2025-01-05T18:35:38Z

This is how it looks now:

There are overlaps and gaps between the lines. To be able fix that I think more info is needed from page_mt.__index:getPageText(). Either the empty lines or paragraph numbers.

poire-z · 2025-01-09T20:44:17Z

Not sure what's the status of this PR is (no time to reread/review).
But as your base PR has been inadvertendly merged, and we may want to bump base, can you make another quick PR with just the change in textboxwidget.lua to use getSelectedWordIndices instead of getSelectedWords and a bump of base up to your commit, so it doesnt break ?

TnS-hun · 2025-01-10T19:55:25Z

I think this PR is ready for merging, but if needed then I can do another PR for getSelectedWordIndices tomorrow.

Frenzie · 2025-01-10T19:58:20Z

I'd rather just merge this one. Could you rebase it on master?

poire-z · 2025-01-10T20:18:10Z

+
+    -- If highlight_text_selection is true then text selection will be highlighted.
+    -- If false then text selection will work as it did originally: no highlighting, and the
+    -- text selection will be updated only on hold release (so hold time might be incorrect).


Is (so hold time might be incorrect) still valid ? (Thought you had fixed something related while we discussed it.)

and the text selection will be updated only on hold release reads odd: on hold release, there is nothing different than when highlight_text_selection = true, we just don't highlight it, and in both cases, we update text_selection as we pan - and when we release, we return the last update of text_selection, isn't it?

Both here and in textboxwidget.lua.

Nice catch. I forgot to update the comments.

poire-z · 2025-01-10T20:27:21Z

+    self.highlight_clear_and_redraw_function = function ()
+        self.highlight_clear_and_redraw_function = nil


There is a little convention (not sure it's always followed) for these kind of scheduled function, we name them ending with _action, so possibly here self.highlight_clear_and_redraw_action.

poire-z · 2025-01-10T20:39:42Z

+                end
+
+                x = x + width
+            end
+        end
+    end
+
+    return rects


(Not a fan of these random blank lines in these 4 functions where it does not really separate bits of work.)

I reduced the number of blank lines in the new code.

poire-z · 2025-01-10T20:43:06Z

+function TextBoxWidget:getXtextHighlightIndices(start_x, start_y, end_x, end_y)
+    -- With xtext and fribidi, words may not be laid out in logical order,
+    -- so the left of a visual word may be its end in logical order,
+    -- and the right its start.
+    -- So, just find out charpos (text indice) of both points and
+    -- find word edges in the logical order text/charlist.


No specific RTL/mixed BiDi stuff needed in textboxwidget.lua (unlike what you added in htmlboxwidget) ? It's messy, or it just all works fine because xtext does the right thing?
(When non-xtext, I guess we don't care as RTL/BiDi is not supported, that's why we introduced xtext.)

With the BiDi text I tried the selection seemed to work correctly with xtext, so I did not add the code that I added to HtmlBoxWidget to prevent joining word rects.

When selecting text in the dictionary (including Wikipedia) or the pop-up footnotes highlight the selected text using an inverting rectangle. After closing the opened dictionary remove the highlight after a short delay just like in the reader.

TnS-hun · 2025-01-11T14:19:51Z

I made the requested changes, rebased it on master, and set base to that xtext getSelectedWordIndices change.

Frenzie · 2025-01-11T15:34:04Z

I guess it's probably not related but I seem to get this when I try to test it:

./luajit: ./ffi/util.lua:673: bad argument #1 to 'pairs' (table expected, got nil)
stack traceback:
	[C]: in function 'pairs'
	./ffi/util.lua:673: in function '__genOrderedIndex'
	./ffi/util.lua:696: in function '(for generator)'
	plugins/gestures.koplugin/main.lua:472: in function 'genCustomMultiswipeSubmenu'
	plugins/gestures.koplugin/main.lua:722: in function 'addToMainMenu'
	frontend/apps/filemanager/filemanagermenu.lua:889: in function 'pre_guard'
	frontend/dbg.lua:46: in function 'setUpdateItemTable'
	frontend/apps/filemanager/filemanagermenu.lua:1028: in function '_getTabIndexFromLocation'
	frontend/apps/filemanager/filemanagermenu.lua:1047: in function 'handler'
	frontend/ui/widget/container/inputcontainer.lua:258: in function 'handleEvent'
	frontend/ui/widget/container/widgetcontainer.lua:83: in function 'propagateEvent'
	frontend/ui/widget/container/widgetcontainer.lua:101: in function 'handleEvent'
	frontend/ui/uimanager.lua:896: in function 'sendEvent'
	frontend/ui/uimanager.lua:53: in function '__default__'
	frontend/ui/uimanager.lua:1423: in function 'handleInputEvent'
	frontend/ui/uimanager.lua:1523: in function 'handleInput'
	frontend/ui/uimanager.lua:1567: in function 'run'
	reader.lua:280: in main chunk
	[C]: at 0x55dd93f7a79c
make: *** [make/emulator.mk:20: run] Error 1

Edit: also an issue on master. That's strange, I wonder when that was introduced. O.o

poire-z · 2025-01-11T21:56:18Z

+        end
+


Bunch of random blank lines around here and the previous local functions - and in HtmlBoxWidget:setPageNumber() and here and there. In a large functions or loops doing multiple things in steps, it's fine to separate the steps - in little ones, it's just useless and makes it bigger than it could be.
(And think about people with small computer screens reviewing :))

Otherwise, fine with this PR.

Okay, I removed even more blanks lines.

Frenzie · 2025-01-12T11:20:47Z

Edit: also an issue on master. That's strange, I wonder when that was introduced. O.o

Never mind, I still had a purposefully broken gestures.lua from #13009 (comment)

poire-z

Still many random blank lines, but I won't spend time pinpointing them.
I let you @Frenzie merge it if it's fine by you.

poire-z · 2025-11-30T21:19:40Z

Little bug witnessed:
When viewing a Wikipedia lookup page, we can indeed highlight text.
But if I long-press on an image (on the top right of each page), and move a little bit while keeping it pressed, some text on the left is highlighted. When I release my long-press, I see the image in ImageViewer. When I close it and get back to the wikipedia article text, the highlight is still there.
Moreover, as I change page, the highlight is still there, highlighting some different bits of text.
Probably just some flag to set or clear when we notice we hold/pan/release on an image. No time to really look at it, so this note for later.

When selecting text in the dictionary (including Wikipedia) or the pop-up footnotes highlight the selected text using an inverting rectangle. After closing the opened dictionary remove the highlight after a short delay just like in the reader. Depends on koreader/koreader-base#2013

poire-z reviewed Dec 27, 2024

View reviewed changes

poire-z reviewed Jan 1, 2025

View reviewed changes

Frenzie added this to the 2025.01 milestone Jan 10, 2025

poire-z reviewed Jan 10, 2025

View reviewed changes

TnS-hun added 8 commits January 11, 2025 14:59

Review feedback changes

1d495af

Better selection

ea3b487

Use per word rects and merging, darken highlight

032492e

Correct hold duration without highlight display

717b513

Use local functions

a93bedc

Less empty lines, action suffix, updated comment

b207c99

Bump base

5aa280c

TnS-hun force-pushed the dictionary-highlight branch from f01a4b7 to 5aa280c Compare January 11, 2025 14:01

poire-z reviewed Jan 11, 2025

View reviewed changes

Removed more blank lines

29e8684

poire-z approved these changes Jan 12, 2025

View reviewed changes

Frenzie reviewed Jan 12, 2025

View reviewed changes

Comment thread frontend/ui/widget/textboxwidget.lua Outdated

remove a newline

f3beaf0

Frenzie reviewed Jan 12, 2025

View reviewed changes

Comment thread frontend/ui/widget/htmlboxwidget.lua Outdated

another newline

fe6952e

Frenzie merged commit bf35875 into koreader:master Jan 12, 2025

poire-z mentioned this pull request Feb 12, 2025

[DictQuickLookup] NT: add text selection to the dictionary widget #13232

Merged

poire-z mentioned this pull request Dec 7, 2025

Style tweaks, Menu, TextBoxWidget: various small fixes #14698

Merged

		if start_line_num < 1 or end_line_num > #self.vertical_string_list then
		return nil, nil, nil, nil

		self.highlight_clear_and_redraw_function = function ()
		self.highlight_clear_and_redraw_function = nil

Uh oh!

Uh oh!

Conversation

TnS-hun commented Dec 27, 2024 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z commented Dec 27, 2024

Uh oh!

TnS-hun commented Dec 28, 2024

Uh oh!

TnS-hun commented Jan 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z commented Jan 1, 2025

Uh oh!

TnS-hun commented Jan 2, 2025

Uh oh!

TnS-hun commented Jan 5, 2025

Uh oh!

benoit-pierre commented Jan 5, 2025

Uh oh!

TnS-hun commented Jan 5, 2025

Uh oh!

poire-z commented Jan 9, 2025

Uh oh!

TnS-hun commented Jan 10, 2025

Uh oh!

Frenzie commented Jan 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TnS-hun commented Jan 11, 2025

Uh oh!

Frenzie commented Jan 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TnS-hun commented Dec 27, 2024 •

edited by Frenzie

Loading

Frenzie commented Jan 11, 2025 •

edited

Loading