[ReaderHighlight] preserve highlight when using highlight dialogue by Commodore64user · Pull Request #14437 · koreader/koreader

Commodore64user · 2025-10-10T12:07:08Z

This pull request introduces small improvements to the highlighting and clipboard features in the reader module, focusing on better user experience when copying text and performing dictionary lookups from the highlight dialogue. The changes ensure the dialogue is closed whilst preserving the highlighted text.

what's new

The onClose method in ReaderHighlight now accepts a keep_highlight parameter, allowing the highlight to persist when needed.
After copying selected text to the clipboard, the highlight is kept briefly and cleared after a 0.5 second delay, giving users time to see what was copied.
When performing a dictionary lookup, the highlight is preserved, and the lookupDict function now receives the highlight as an argument for better context management.
In the QR clipboard plugin, the dismiss callback now keeps the highlight and also delays clearing it by 0.5 seconds, improving user feedback for QR code generation.

This change is

poire-z · 2025-10-10T16:13:03Z

(You're not obliged to follow @Frenzie or myself's all suggestions.)

You name it in the code BACKLINK_HIGHLIGHT_DURATION (which reads bad: there is no link followed or backlink), DELAY_CLEAR_HIGHLIGHT_S reads truer.
And it has the _S we have elsewhere to document the unit.

Commodore64user · 2025-10-10T16:21:49Z

(You're not obliged to follow @Frenzie or myself's all suggestions.)

if I don't do as he says, he follows me around teasing me with his Ryzen 7 with zillions of cores that builds emulators in seconds not minutes. ;)

Co-authored-by: Frans de Jonge <fransdejonge@gmail.com>

Frenzie · 2025-10-10T20:20:37Z

Isn't there another one for if you follow a link / location btw?

Commodore64user · 2025-10-10T20:44:54Z

I believe that's all of them... (don't quote me)

Frenzie · 2025-10-10T21:25:50Z

@poire-z Is the little indicator for where a link points in there?

poire-z · 2025-10-10T22:03:19Z

It is in there and has a little world of his own:

koreader/frontend/apps/reader/modules/readerrolling.lua

Lines 786 to 796 in e84d63b

    
           -- Allow tweaking this marker behaviour with a manual setting: 
        
           --   followed_link_marker = false: no marker shown 
        
           --   followed_link_marker = true: maker shown and not auto removed 
        
           --   followed_link_marker = <number>: removed after <number> seconds 
        
           -- (no real need for a menu item, the default is the finest) 
        
           local marker_setting 
        
           if G_reader_settings:has("followed_link_marker") then 
        
               marker_setting = G_reader_settings:readSetting("followed_link_marker") 
        
           else 
        
               marker_setting = 1 -- default is: shown and removed after 1 second 
        
           end

I'd rather not send @Commodore64user on a journey into this world :)
(And it is not a highlight, and works when following link forward too.)

Frenzie · 2025-10-10T22:17:09Z

Alright.

Commodore64user · 2025-10-11T01:20:21Z

I'd rather not send @Commodore64user on a journey into this world :)

unsure if I should be grateful or offended...

hius07 · 2025-10-11T06:34:32Z

                callback = function()
-                    this:lookupDict(index)
-                    -- We don't call this:onClose(), same reason as above
+                    this:lookupDict(index, this)


this already there as self.

hius07 · 2025-10-11T06:35:24Z

        end
    end
-    UIManager:scheduleIn(0.5, self.highlight_clear_and_redraw_action)
+    UIManager:scheduleIn(self.ui.DELAY_CLEAR_HIGHLIGHT_S, self.highlight_clear_and_redraw_action)


Are you sure the widget got proper ui?

hius07 · 2025-10-11T06:41:19Z

 end

 function ReaderUI:init()
+    self.DELAY_CLEAR_HIGHLIGHT_S = 0.5


The constant, can be set once in the module, not in instances.

poire-z · 2025-10-11T22:22:22Z

+                local ReaderUI = require("apps/reader/readerui")
+                UIManager:scheduleIn(ReaderUI.DELAY_CLEAR_HIGHLIGHT_S, function() lookupCancelled() end)


That's quite ugly. All this to get a constant, because readerdict can be use in filemanager ?
I'd rather just see 0.5...

all that because @hius07 prefers it declared in the module, not as instance variable, but it was working fine everywhere with self.ui.WHATEVER even in FM. ¯\_(ツ)_/¯

I think he meant you could delare it in here:

koreader/frontend/apps/reader/readerui.lua

Lines 75 to 80 in e84d63b

local ReaderUI = InputContainer:extend{

name = "ReaderUI",

active_widgets = nil, -- array

-- if we have a parent container, it must be referenced for now

dialog = nil,

and you could access it just like if declared in :init() as self.ui.DELAY_CLEAR_HIGHLIGHT_S.

if that is what he meant, he should have said class variable. That is NOT what he said

poire-z · 2025-10-11T23:22:17Z

I think this constant has really nothing to do in ReaderUI (except for broader access).
If it relates to highlights, and ReaderHighlight is where hold/long-press is handled, it should be in ReaderHighlight.

poire-z · 2025-10-12T00:01:15Z

            callback = function(dialog)
                UIManager:close(dialog)
-                UIManager:scheduleIn(0.5, function() lookupCancelled() end)
+                UIManager:scheduleIn(self.ui.highlight.DELAY_CLEAR_HIGHLIGHT_S, function() lookupCancelled() end)


Can this codepath be taken if dict invoked from FileBrowser - there would be no self.ui.highlight ?
Same question for dictquicklookup ?

good catch, this one is actually problematic, the one on dictquick is under a self.highlight so safe...

Frenzie · 2025-10-13T09:51:00Z

+                local delay = self.ui.highlight and self.ui.highlight.DELAY_CLEAR_HIGHLIGHT_S or 0.5
+                UIManager:scheduleIn(delay, function() lookupCancelled() end)


What happens if you start a new search in the interval? (Not a new problem, if it is one.)

I tried with the current version, and nothing really. There are some factors, finger speed, by the time you start contact with the screen you are almost in the second half of the 0.5 and it clears just as normal and the next crengine selection begins.

As you said, not a new problem, which you can verify as well if you think you have faster fingering speed (FFS) 🫣

I'm asking more what the problem would be if it did happen. If it's just a minor visual glitch it probably doesn't really matter, but if there's some kind of memory race condition it could be more serious.

Commodore64user · 2025-10-13T10:12:28Z

I understand the thinking behind, so what should be done then?

Frenzie · 2025-10-13T10:29:07Z

I'd say probably either add it to https://github.com/koreader/koreader/blob/87445034bd2ddadb524b2db3bfc7d05c657e8546/frontend/ui/size.lua (granted, it's a duration) or just use local DELAY_CLEAR_HIGHLIGHT_S = 0.5. While the latter wouldn't have a single source of truth, it would immediately show up in a search, and I don't really expect the number of them to grow.

Commodore64user · 2025-10-13T12:36:40Z

    -- this should be self.ui.highlight.DELAY_CLEAR_HIGHLIGHT_S, but in FM we don't have access to self.ui
    local delay = 0.5
    UIManager:scheduleIn(delay, function() lookupCancelled() end)

@Frenzie?

Frenzie · 2025-10-13T13:19:51Z

What's the question precisely?

Commodore64user · 2025-10-13T13:48:50Z

Is that better? A suitable alternative? Any other suggestions

Frenzie · 2025-10-13T14:30:14Z

But I mean are you saying are happy with and prefer self.ui.highlight.DELAY_CLEAR_HIGHLIGHT_S in general, and that you'd prefer to keep this instance as an exception?

Commodore64user · 2025-10-13T14:50:12Z

I am saying we keep the self.WHATEVER and use this as the exception for when self is not accessible, that way we get the grep and it sort of explains to anyone reading the code what's happening instead of the simple 0.5

Frenzie · 2025-10-13T15:38:37Z

Hold on, if there's no highlight to clear then why is there any delay at all? Shouldn't that be:

if self.ui then
  -- delay
else
  -- immediate
end

Frenzie · 2025-10-13T16:02:42Z

 local Screen = Device.screen

 local ReaderHighlight = InputContainer:extend{
+    DELAY_CLEAR_HIGHLIGHT_S = 0.5,


Suggested change

DELAY_CLEAR_HIGHLIGHT_S = 0.5,

Commodore64user · 2025-10-13T16:30:36Z

That is only partially true in one case, searching from FM (or direct searches). In all other cases there are highlights, particular mention to meta-queries which also have highlights to clear

Frenzie · 2025-10-13T16:43:43Z

Alright, just keep it as is then (besides the line that should be removed).

Commodore64user · 2025-10-13T18:04:38Z

On mobile, but wouldn't removing that line mean it never gets declared? Then we are trying to access something that doesn't exist

Frenzie · 2025-10-13T18:27:40Z

Indeed, while on mobile I'd overlooked the comma.

Commodore64user · 2025-10-13T21:47:48Z

have a look @Frenzie, better now?, another thing would be to declare the class variable (constant) in FM as well...

Frenzie · 2025-10-13T21:49:22Z

I think I'd prefer to just do that, but let's see what the others think.

poire-z · 2025-10-13T23:24:00Z

I think a DELAY_CLEAR_HIGHLIGHT_S has absolutely nothing to do in filemanager.lua where it is not used directly, and would only be used by module far far away (textboxwidget.lua).

Maybe we can put it in that file we have forgotten: https://github.com/koreader/koreader/blob/master/defaults.lua - just because it's used by many unrelated bits, so it can be considered global :)

Commodore64user · 2025-10-13T23:35:29Z

I was about to comment that the FM declaration is actually not very helpful and was a terrible idea. I take it back.

in my opinion, the current form is the best compromise, it is still easily trackable with greps and doesn't bother the widgets with conundrums like "to ui or not to ui"

Frenzie · 2025-10-14T08:25:55Z

Maybe we can put it in that file we have forgotten: https://github.com/koreader/koreader/blob/master/defaults.lua - just because it's used by many unrelated bits, so it can be considered global :)

I already stated my preferred solution in #14437 (comment)

I regret bringing up the subject because I don't like this self.ui business at all.

Commodore64user · 2025-10-14T11:04:11Z

but you first proposed the self.ui though, are you sure adding it to the size file is better? like really?

hius07 · 2025-10-14T11:23:26Z

I'm also for

UIManager:scheduleIn(G_defaults:readSetting("DELAY_CLEAR_HIGHLIGHT_S"), function()...

hius07 · 2025-10-14T11:26:22Z

One more

koreader/frontend/apps/reader/modules/readerdictionary.lua

Lines 1187 to 1192 in d97e177

    
           if self.highlight then 
        
               local clear_id = self.highlight:getClearId() 
        
               UIManager:scheduleIn(0.5, function() 
        
                   self.highlight:clear(clear_id) 
        
               end) 
        
           end

Frenzie · 2025-10-14T11:35:35Z

but you first proposed the self.ui though, are you sure adding it to the size file is better? like really?

Well yes, I like that better than sprinkling "we can't access self.ui here" all over the place. :-) But in any case let's go with G_defaults:readSetting("DELAY_CLEAR_HIGHLIGHT_S"); being easy to customize is not a bad thing.

…oreader#14437)

handle highlight nicely when using highlight dialogue

293f862

Commodore64user marked this pull request as ready for review October 10, 2025 12:07

onClose before qr is shown

90d8008

Frenzie reviewed Oct 10, 2025

View reviewed changes

Comment thread frontend/apps/reader/modules/readerhighlight.lua Outdated

add self.ui.DELAY_CLEAR_HIGHLIGHT_S or whatever makes sense.

a167edd

DELAY_CLEAR_HIGHLIGHT_S

f865203

Frenzie reviewed Oct 10, 2025

View reviewed changes

Comment thread frontend/apps/reader/readerui.lua Outdated

Commodore64user and others added 2 commits October 10, 2025 18:32

Update highlight clear delay for footnotes

a0e59e4

Update readerui.lua

98211d2

Co-authored-by: Frans de Jonge <fransdejonge@gmail.com>

missing ones

ad7e04e

hius07 reviewed Oct 11, 2025

View reviewed changes

Comment thread frontend/ui/widget/textboxwidget.lua

hius07 reviewed Oct 11, 2025

View reviewed changes

declare constant in module, no need to pass this to lookup

28e1c8d

poire-z reviewed Oct 11, 2025

View reviewed changes

Commodore64user added 2 commits October 11, 2025 23:57

declare in class

7f73177

fix delay when not in self.ui

73df4dc

declare it in readerHighlight

a3b4f53

poire-z reviewed Oct 12, 2025

View reviewed changes

readerDict can be accessed from FM, account or that

2b154ac

Frenzie reviewed Oct 13, 2025

View reviewed changes

declare local var

d97e177

save as global default

747eee0

Frenzie added this to the 2025.10 milestone Oct 14, 2025

Frenzie merged commit 6d49c26 into koreader:master Oct 14, 2025
4 checks passed

Commodore64user deleted the high-dial branch October 14, 2025 13:31

poire-z mentioned this pull request Jan 8, 2026

Wikipedia: revamp languages handling #14791

Merged

0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026

[ReaderHighlight] preserve highlight when using highlight dialogue (k…

a2bd1c0

…oreader#14437)

		local ReaderUI = require("apps/reader/readerui")
		UIManager:scheduleIn(ReaderUI.DELAY_CLEAR_HIGHLIGHT_S, function() lookupCancelled() end)

	local ReaderUI = InputContainer:extend{
	name = "ReaderUI",
	active_widgets = nil, -- array

	-- if we have a parent container, it must be referenced for now
	dialog = nil,

		local delay = self.ui.highlight and self.ui.highlight.DELAY_CLEAR_HIGHLIGHT_S or 0.5
		UIManager:scheduleIn(delay, function() lookupCancelled() end)

Uh oh!

Uh oh!

Conversation

Commodore64user commented Oct 10, 2025 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

what's new

Uh oh!

Uh oh!

poire-z commented Oct 10, 2025

Uh oh!

Commodore64user commented Oct 10, 2025

Uh oh!

Uh oh!

Frenzie commented Oct 10, 2025

Uh oh!

Commodore64user commented Oct 10, 2025

Uh oh!

Frenzie commented Oct 10, 2025

Uh oh!

poire-z commented Oct 10, 2025

Uh oh!

Frenzie commented Oct 10, 2025

Uh oh!

Commodore64user commented Oct 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 Oct 11, 2025

Uh oh!

poire-z Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Commodore64user Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Commodore64user commented Oct 13, 2025

Uh oh!

Frenzie commented Oct 13, 2025

Uh oh!

Commodore64user commented Oct 13, 2025

Uh oh!

Frenzie commented Oct 13, 2025

Uh oh!

Commodore64user commented Oct 13, 2025

Uh oh!

Frenzie commented Oct 13, 2025

Uh oh!

Commodore64user commented Oct 13, 2025

Uh oh!

Frenzie commented Oct 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Commodore64user commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie commented Oct 13, 2025

Commodore64user commented Oct 10, 2025 •

edited by Frenzie

Loading

Commodore64user Oct 11, 2025 •

edited

Loading

poire-z Oct 12, 2025 •

edited

Loading

Commodore64user Oct 13, 2025 •

edited

Loading

Commodore64user commented Oct 13, 2025 •

edited

Loading

Commodore64user commented Oct 14, 2025 •

edited

Loading

Frenzie commented Oct 14, 2025 •

edited

Loading