Add key_repeat support to kindle NT by Commodore64user · Pull Request #13328 · koreader/koreader

Commodore64user · 2025-02-26T17:05:25Z

what's new

frontend/device/input.lua: Enhanced the key repeat logic to include cursor keys for Kindle devices and improved the responsiveness by using timestamps for key repeat events.
frontend/device/kindle/device.lua: Added a new method toggleKeyRepeat to enable or disable key repeat functionality.
frontend/device/kindle/device.lua: Updated the init method to set up key repeat events for devices with a DPad, with configurable delay and period settings.
frontend/ui/elements/physical_buttons.lua: Modified the insertion order of the key repeat disable option to ensure it appears at the top of the list.

screenshots

This change is

Frenzie · 2025-02-26T17:08:08Z

        return Event:new("KeyPress", key)
    elseif ev.value == KEY_REPEAT then
-        -- NOTE: We only care about repeat events from the pageturn buttons...
+        -- NOTE: We only care about repeat events from the page-turn buttons (kobo) and cursor keys (kindle)...


Adding Kobo/Kindle to the comment seems more confusing than elucidating (i.e., what happens on other platforms).

nothing since the if not rep_delay or rep_delay == 0 then return end check should kick 'em out.

…what? Why? O.o

Because i can only guarantee that it works there, i can't test other platforms. To avoid potential crashes best to err on the side of caution. Furthermore if the value is zero and we don't return early... well any length of time is greater than zero

Nothing about enabling support for Kindle implies disabling support for all other platforms. I should not have to find out about that by accident.

So to be clear you're not changing anything other than making a comment confusing which wasn't confusing before?

But something about all that code further down still doesn't make sense. Either they can end up here and they should be treated normally rather than blocked or they can't and there's no need to guard against anything.

So to be clear you're not changing anything other than making a comment confusing which wasn't confusing before?

That sounds quite like what might be happening here. I was only trying to convey that only kobo supports repeats on page-turn buttons and kindle on cursor keys

On another note, it would be cool if you could test this on your kobo.

But something about all that code further down still doesn't make sense. Either they can end up here and they should be treated normally rather than blocked or they can't and there's no need to guard against anything.

Well as i said the equal zero neutralises a user toggling it off , but what exactly doesn't make sense to you?

It sounds like you're saying it's mostly a somewhat convoluted way of writing something like:

Device:canKeyRepeat() and G_reader_settings:isNilOrFalse("input_no_key_repeat")

Which you can probably just reduce to:

elseif ev.value == KEY_REPEAT and G_reader_settings:isNilOrFalse("input_no_key_repeat") then

Frenzie · 2025-02-26T21:22:04Z

+            local rep_delay = self.device.key_repeat[C.REP_DELAY]
+            if not rep_delay or rep_delay == 0 then return end
+            local rep_period = self.device.key_repeat[C.REP_PERIOD]
+            if not rep_period or rep_period == 0 then return end


Okay, having looked at the rest of the code now this won't work quite right as written, since it'll error due to self.device.key_repeat being nil.

For the moment I suggest to change it as follows (in conjunction with the elseif ev.value == KEY_REPEAT and G_reader_settings:isNilOrFalse("input_no_key_repeat") up above).

Suggested change

local rep_delay = self.device.key_repeat[C.REP_DELAY]

if not rep_delay or rep_delay == 0 then return end

local rep_period = self.device.key_repeat[C.REP_PERIOD]

if not rep_period or rep_period == 0 then return end

local rep_period = self.device.key_repeat and self.device.key_repeat[C.REP_PERIOD] or 120

if rep_period == 0 then return end

In the Kobo implementation, it seems to be changed with ioctl, meaning with 0 there simply won't be any KEY_REPEAT events:

koreader/frontend/device/kobo/device.lua

Lines 668 to 672 in 926eb9d

if C.ioctl(self.ntx_fd, C.EVIOCSREP, key_repeat) < 0 then

local err = ffi.errno()

logger.warn("Device:toggleKeyRepeat: EVIOCSREP ioctl on fd", self.ntx_fd, "failed:", ffi.string(C.strerror(err)))

return false

end

I think that with this PR as written, the

self.key_repeat = ffi.new("unsigned int[?]", C.REP_CNT) if G_reader_settings:isTrue("input_no_key_repeat") then self.key_repeat[C.REP_DELAY] = 0 self.key_repeat[C.REP_PERIOD] = 0 else self.key_repeat[C.REP_DELAY] = 400 self.key_repeat[C.REP_PERIOD] = 120 end

Should be put in generic device.

But for the moment, let's just make the aforementioned two small changes to this PR and wait to see what @NiLuJe thinks with regard to the underlying design.

applied your two other suggestions, but we are now letting in anyone with cursor keys...

I am also not sure this if rep_period == 0 then return end does anything at all now. I mean the only case in which that is zero now is when the G-set is true, which is handled at the top.

but we are now letting in anyone with cursor keys...

Exactly! Anything else is unacceptable.

Commodore64user · 2025-02-27T00:31:36Z

okay sorry @Frenzie but I am bringing back the loop, although it looks needless as you said (I think), it turns out that it is not. It is in fact, the driver of quite an improvement boost. Let me rephrase that: with the previous implementation and simply adding the cursor keys to the if check, moving the crosshairs from the topmost of the screen to the bottommost takes around 11 seconds. With the same old if (and cursor keys added) and the new time implementation, now we travel the same trajectory in 7 seconds. But if we use the loop and the time implementation, now it takes 4 seconds to go from top to bottom.

Frenzie · 2025-02-27T09:26:35Z

A quick glance suggests the if … or … or is ~70-80 times faster than ipairs. Interestingly, in regular Lua it's only ~7 times faster, same in LuaJIT with -joff. It's essentially impossible for it to be otherwise, since ipairs here is achieving the same end result with extra steps.

If there were a lot of values a table lookup might become faster eventually, if you write it like if allowed_repeat_keys[keycode].

But all that I said above should be irrelevant in practice since we're only talking about one repeat every tens of milliseconds, for Kindle NT specifically every 60 ms or so according to the linked issue. I can only guess that the table definition compared with ipairs introduces a minuscule delay that has happy results in combination with the cursor's own timing on your particular device.

It's potentially also worth noting that you reversed the order, but that shouldn't compensate for the ipairs overhead.

local function test_if_or(iterations)
    local a, b, c = false, false, true
    local start = os.clock()
    for i = 1, iterations do
        if a or b or c then
            -- do nothing
        end
    end
    return os.clock() - start
end

local function test_ipairs(iterations)
    local t = {false, false, true}
    local start = os.clock()
    for i = 1, iterations do
        for _, v in ipairs(t) do
            if v then
                break
            end
        end
    end
    return os.clock() - start
end

local iterations = 1000000
local time_if_or = test_if_or(iterations)
local time_ipairs = test_ipairs(iterations)

print(string.format("Time taken (if ... or ...): %.6f seconds", time_if_or))
print(string.format("Time taken (ipairs): %.6f seconds", time_ipairs))
print(string.format("Ratio (ipairs / if or): %.2f", time_ipairs / time_if_or))

Commodore64user · 2025-02-27T13:04:03Z

here is some empirical data running on the K4: starting from the topmost of the screen, running all the way down, and back up again (x 2, so 4 legs total)

on key press

self._start = time.now()
logger.dbg("Test #: xxx start")

on key release

local now = time.now()
logger.dbg("Test #: xxx end",  time.to_s(now - self._start), "sec")

02/27/25-13:33:49 DEBUG Test 3: loop start 
02/27/25-13:33:54 DEBUG Test 3: loop end 4.591861 sec 
02/27/25-13:33:55 DEBUG Test 3: loop start 
02/27/25-13:34:00 DEBUG Test 3: loop end 4.636983 sec 
02/27/25-13:34:02 DEBUG Test 3: loop start 
02/27/25-13:34:07 DEBUG Test 3: loop end 4.660367 sec 
02/27/25-13:34:10 DEBUG Test 3: loop start 
02/27/25-13:34:15 DEBUG Test 3: loop end 4.888034 sec

02/27/25-13:41:25 DEBUG Test 4: if start 
02/27/25-13:41:30 DEBUG Test 4: if end 4.666953 sec 
02/27/25-13:41:32 DEBUG Test 4: if start 
02/27/25-13:41:36 DEBUG Test 4: if end 4.827938 sec 
02/27/25-13:41:40 DEBUG Test 4: if start 
02/27/25-13:41:45 DEBUG Test 4: if end 4.711301 sec 
02/27/25-13:41:47 DEBUG Test 4: if start 
02/27/25-13:41:52 DEBUG Test 4: if end 4.656943 sec

so there is, empirical evidence of little to no statistical significance (within the realms of this small test, sometimes either comes on top)... what???? I swear that every time I've tried to use the loop it seems relatively slower, what's going on?

and for reasons, they do take longer sometimes, but it happens to both cases...

02/27/25-13:52:10 DEBUG Test 5: if start 
02/27/25-13:52:17 DEBUG Test 5: if end 7.264478 sec

Commodore64user · 2025-02-27T21:41:29Z

as usual, I once again just happen to bump into yet another nest of rats lurking in a dark corner. It turns out that the dicquicklookup widget does not support key repeats when doing text selection, not sure why atm.

logs show that the events are being generated but it appears they are not being processed... any ideas?

Frenzie · 2025-02-27T21:54:42Z

Does it depend on KeyRelease or something?

Commodore64user · 2025-02-27T22:09:17Z

Does it depend on KeyRelease or something?

what do you mean? I think that some children widgets don't have an implementation for key_repeats (that is just speculation on my part though).

Frenzie · 2025-02-27T22:14:48Z

If it depends on InputContainer (which they basically all do?) they get their key events and key repeat events from there without distinction.

I suspect this will fix your problem. (FocusManager:onKeyPress was added in #13232)

diff --git a/base b/base
--- a/base
+++ b/base
@@ -1 +1 @@
-Subproject commit d2c1d37da563125df18efce152a0b2db9af1f7f6
+Subproject commit d2c1d37da563125df18efce152a0b2db9af1f7f6-dirty
diff --git a/frontend/ui/widget/focusmanager.lua b/frontend/ui/widget/focusmanager.lua
index 9460ef367..8b17e4938 100644
--- a/frontend/ui/widget/focusmanager.lua
+++ b/frontend/ui/widget/focusmanager.lua
@@ -526,4 +526,6 @@ function FocusManager:onKeyPress(key)
     return InputContainer.onKeyPress(self, key)
 end
 
+FocusManager.onKeyRepeat = FocusManager.onKeyPress
+
 return FocusManager

Commodore64user · 2025-02-28T00:31:27Z

interesting, that indeed does the job.

Commodore64user · 2025-03-05T23:01:15Z

after A/B testing both the ipairs loop and the if statement, I can confidently say that neither is better or worse in this case, sometimes one performs flawlessly, the next time it staggers like a limping gazelle. I am not sure why, but since we are still awaiting @NiLuJe's input here... we'll see

Commodore64user · 2025-03-10T15:17:24Z

seeing as NiLuJe does not seem interested in joining us here (sad occasion), what direction do you reckon this should take @Frenzie ?

NiLuJe · 2025-03-05T07:22:43Z

        return Event:new("KeyPress", key)
    elseif ev.value == KEY_REPEAT then
-        -- NOTE: We only care about repeat events from the pageturn buttons...
+        if G_reader_settings:isTrue("input_no_key_repeat") then return end


If the ioctl is supported, this needs to go. Ideally, even if it doesn't, this needs to go ;p.

Say, with a device-specific event hook that swallows KEY_REPEAT events ;).

NiLuJe · 2025-03-05T07:23:48Z

-        or keycode == "RPgBack"
-        or keycode == "LPgFwd"
-        or keycode == "RPgFwd" then
+        local allowed_repeat_keys = {


Could probably be done with a key group

NiLuJe · 2025-03-10T15:45:27Z

Big green GH button for reviews struck again -_-".

Frenzie · 2025-03-10T15:49:19Z

Ah yeah, I've done that. Easy to forget. :-)

Frenzie · 2025-03-10T18:11:43Z

+    -- keys with key-repeat events
+    KeyRepeat = {
+        Up = true, Down = true, Left = true, Right = true,
+        LPgBack = true, RPgBack = true, LPgFwd = true, RPgFwd = true
+    },


I'm not a fan of this. It dresses up something I don't think we should want as if it were a proper thing.

I presume that means bring back the if

I'd prefer to just blacklist sleepcover events, but the safe approach is to keep it as is in a way that signals to the reader that it's a bit hacky rather than working exactly as intended. :-)

koreader/frontend/device/kobo/device.lua

Lines 828 to 829 in cfbb676

[35] = "SleepCover", -- KEY_H, Elipsa

[59] = "SleepCover",

right so if not sleepcover it is.

Commodore64user · 2025-03-11T14:24:44Z

+        if keycode == "Up" or keycode == "Down" or keycode == "Left" or keycode == "Right"
+         or keycode == "RPgBack" or keycode == "RPgFwd" or keycode == "LPgBack" or keycode == "LPgFwd" then


Suggested change

if keycode == "Up" or keycode == "Down" or keycode == "Left" or keycode == "Right"

or keycode == "RPgBack" or keycode == "RPgFwd" or keycode == "LPgBack" or keycode == "LPgFwd" then

if keycode ~= "SleepCover" then

@Frenzie

Conceptually I think that's a significantly better approach, yes. But it requires careful thought and testing (e.g., what about power events?) while we know the current situation is fine. So if you're still aiming for the upcoming stable I wouldn't be inclined to do that yet.

Well, what is holding back the next release? I suppose we could keep it like so for the next one and then test this approach and switch to it later, no biggie.

Time for some testing and writing release notes (i.e., same as always). I nearly always use a recent nightly on my Kobo but my regular use doesn't touch most features.

Are we still on for 2024.03?

I believe @NiLuJe indicated being unhappy with this approach compared to doing something with an event hook, unless I misremember.

Yes removing the G_sett, which was done and we are using the hook to swallow the key repeats already.

Something about the whole affair strikes me as odd (i.e., shouldn't this manual approach only be done if it can't be set the other way), but afaict it's the better version of what's already there, so for me it's good enough. Still I'll wait to see what NiLuJe says. :-)

NiLuJe

Looks good beside the reset ;).

NiLuJe · 2025-03-15T17:56:25Z

+
+        -- We can't easily clear existing hooks, but we can overwrite the eventAdjustHook
+        -- with the default empty implementation to effectively remove previous hooks
+        self.input.eventAdjustHook = self.input.gestureAdjustHook


Should be a reset to the class Input.eventAdjustHook (c.f., frontend/device/android/device.lua which does something like this ;)).

Frenzie · 2025-03-15T18:34:30Z

Did you check that it's still working as it should? ^_^

Commodore64user · 2025-03-15T19:03:59Z

Did you check that it's still working as it should? ^_^

Mmm, not really, no. I am blindly trusting NiLuJe here. Will do when i get home...

edit: @Frenzie, it works just fine.

Frenzie · 2025-03-15T22:10:29Z

Interesting, it looks like GH now notifies of edits.

Commodore64user · 2025-03-15T22:12:57Z

Interesting, it looks like GH now notifies of edits.

I believe it only does if a tag is added (double tag wouldn't), which is the case.

mergen3107 · 2025-03-15T23:37:08Z

Interesting, it looks like GH now notifies of edits.

I am pretty sure if the edits contain mentions, you would be notified anyway

mergen3107 · 2025-03-15T23:37:25Z

Interesting, it looks like GH now notifies of edits.

I believe it only does if a tag is added (double tag wouldn't), which is the case.

You beat me to it lol

Commodore64user added 3 commits February 26, 2025 12:40

handle key_repeat initial commit

37cd332

move repeat setting to the top of physical-keys menu

ea87e33

remove for-loop

b60a4ed

Frenzie reviewed Feb 26, 2025

View reviewed changes

Frenzie added this to the 2025.02 milestone Feb 26, 2025

Commodore64user added 2 commits February 26, 2025 17:32

remove comments, mainly

53e7bc7

one more

ddb49e8

Frenzie reviewed Feb 26, 2025

View reviewed changes

Commodore64user added 3 commits February 26, 2025 23:35

Frezie's suggestions

9e390e1

bring back the loop, PLEASE

962f7c0

glaring ommision

482bb55

add support for keyRepeat in FocusManager

13e8dfd

Commodore64user mentioned this pull request Mar 4, 2025

[FocusManager] correct cursor keys in RTL #13362

Merged

revert ipairs

f5f7189

Merge remote-tracking branch 'upstream/master' into key-repeat

65841be

NiLuJe reviewed Mar 10, 2025

View reviewed changes

add hooks and a key-group

7383e0d

Frenzie reviewed Mar 10, 2025

View reviewed changes

revert KeyRepeat group

843368e

Commodore64user commented Mar 11, 2025

View reviewed changes

Commodore64user requested review from Frenzie and NiLuJe March 11, 2025 16:44

Frenzie approved these changes Mar 15, 2025

View reviewed changes

NiLuJe requested changes Mar 15, 2025

View reviewed changes

Commodore64user commented Mar 15, 2025

View reviewed changes

Comment thread frontend/device/kindle/device.lua Outdated

Commodore64user added 2 commits March 15, 2025 18:07

Update device.lua

d527689

Update device.lua

27a0db9

Commodore64user added 2 commits March 15, 2025 22:05

move Input

051ce36

Merge remote-tracking branch 'upstream/master' into key-repeat

b2cf8dc

Commodore64user requested a review from NiLuJe March 15, 2025 22:06

NiLuJe approved these changes Mar 15, 2025

View reviewed changes

Frenzie merged commit 1354537 into koreader:master Mar 15, 2025
4 checks passed

Commodore64user deleted the key-repeat branch March 15, 2025 22:39

	if C.ioctl(self.ntx_fd, C.EVIOCSREP, key_repeat) < 0 then
	local err = ffi.errno()
	logger.warn("Device:toggleKeyRepeat: EVIOCSREP ioctl on fd", self.ntx_fd, "failed:", ffi.string(C.strerror(err)))
	return false
	end

		if keycode == "Up" or keycode == "Down" or keycode == "Left" or keycode == "Right"
		or keycode == "RPgBack" or keycode == "RPgFwd" or keycode == "LPgBack" or keycode == "LPgFwd" then

	if keycode == "Up" or keycode == "Down" or keycode == "Left" or keycode == "Right"
	or keycode == "RPgBack" or keycode == "RPgFwd" or keycode == "LPgBack" or keycode == "LPgFwd" then
	if keycode ~= "SleepCover" then

Uh oh!

Conversation

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

Uh oh!

what's new

screenshots

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!

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!

Uh oh!

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!

Commodore64user commented Feb 27, 2025

Uh oh!

Frenzie commented Feb 27, 2025

Uh oh!

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

Uh oh!

Uh oh!

Commodore64user commented Feb 27, 2025

Uh oh!

Frenzie commented Feb 27, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Commodore64user commented Feb 28, 2025

Uh oh!

Commodore64user commented Mar 5, 2025

Uh oh!

Commodore64user commented Mar 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!

NiLuJe commented Mar 10, 2025

Uh oh!

Frenzie commented Mar 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Commodore64user commented Feb 26, 2025 •

edited

Loading

Commodore64user commented Feb 27, 2025 •

edited

Loading

Commodore64user commented Feb 27, 2025 •

edited

Loading

Frenzie commented Feb 27, 2025 •

edited

Loading

Commodore64user Mar 11, 2025 •

edited

Loading

Commodore64user commented Mar 15, 2025 •

edited

Loading