Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FR: Better D-pad support on K4/5 #11295

Closed
Commodore64user opened this issue Dec 28, 2023 · 64 comments · Fixed by #11749
Closed

FR: Better D-pad support on K4/5 #11295

Commodore64user opened this issue Dec 28, 2023 · 64 comments · Fixed by #11749
Labels
Milestone

Comments

@Commodore64user
Copy link
Contributor

Commodore64user commented Dec 28, 2023

Hi KoReader team,

I am a new KoR user, I am using a Black Kindle 4/5 (B023, K4) and running version 2023.10.
My issue concerns the D-pad, currently the D-pad is set to individual page turns (either forwards or backwards) for both up/down and left/right, however the kindle already has two sets of turn page buttons which makes it a total of FOUR sets of buttons doing exactly the same job.

I was wondering if you guys could make the D-pad behave the same way the stock firmware does, i.e left/right moves you to previous/next chapter and up/down allows you to select words for dictionary lookup or highlights.

This leads me to another issue and much in the same spirit as before, the only way of selecting words is using 'content selection' which is at best annoying on non-touch kindles. so once again, I am wondering whether 'content selection' could be added via d-pad instead in much the same way stock firmware does it (up/down).

Also, the keyboard button does not do anything whatsoever, it would be nice if you could map it to either 'full text search' or to give the user the option to select what they'd like to do. For example turn wifi on/off.

thank you for your time and consideration.
commodore64user

@Commodore64user
Copy link
Contributor Author

Commodore64user commented Mar 24, 2024

Thinking about this, would it be possible to re-map the left/right to next chapter (when inside a book), and up/down to trigger this feature from #8877 using a user patch?

navigation on non-touch is quite difficult the next/previous chapter would really help a lot.

@Frenzie
Copy link
Member

Frenzie commented Mar 24, 2024

If any of those are currently mapped to any button events it might be simpler still.
https://github.com/koreader/koreader/blob/ca8e9352ba962e48843cd1d86812e42bdade7708/frontend/device/kindle/event_map_keyboard.lua

I don't have time to investigate right now, but what you're asking should probably be relatively straightforward (although you never know when something's strongly dependent on something else).

@Commodore64user
Copy link
Contributor Author

If any of those are currently mapped to any button events it might be simpler still. https://github.com/koreader/koreader/blob/ca8e9352ba962e48843cd1d86812e42bdade7708/frontend/device/kindle/event_map_keyboard.lua

I don't have time to investigate right now, but what you're asking should probably be relatively straightforward (although you never know when something's strongly dependent on something else).

if you find some time to look into it, it would be amazing, as I don't know how to implement it.

@Frenzie
Copy link
Member

Frenzie commented Apr 9, 2024

You'll have to figure out with detailed logs what the output for those buttons is.

@Commodore64user
Copy link
Contributor Author

I have the logs, I think...
I opened a book and went: right, up, left, down, keyboard button.

Logs
04/18/24-00:46:53 DEBUG key event => code: 106 (Right), value: 0, time: 1713401213.543660 
04/18/24-00:46:53 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:46:54 DEBUG key event => code: 103 (Up), value: 1, time: 1713401214.799375 
04/18/24-00:46:54 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:46:54 DEBUG goto relative screen: -1 in mode: page 
04/18/24-00:46:54 DEBUG CreDocument: goto page 400 flow 0 
04/18/24-00:46:54 DEBUG _refresh: Enqueued partial update for region 0 0 600 800 dithering: false 
04/18/24-00:46:54 DEBUG setDirty partial from widget ReaderUI w/ NO region; dithering: nil 
04/18/24-00:46:54 DEBUG painting widget: ReaderUI 
04/18/24-00:46:54 DEBUG readerview painting "600x800+0+0" to 0 0 
04/18/24-00:46:54 DEBUG CreDocument: goto page 400 flow 0 
04/18/24-00:46:54 DEBUG blitFrom 251 786 0 0 5 5 
04/18/24-00:46:54 DEBUG triggering refresh {
  mode = "partial",
  region = "600x800+0+0"
} --[[table: 0x42b2dbf0]] 
04/18/24-00:46:54 DEBUG key event => code: 103 (Up), value: 0, time: 1713401214.971589 
04/18/24-00:46:54 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:46:56 DEBUG key event => code: 105 (Left), value: 1, time: 1713401216.207569 
04/18/24-00:46:56 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:46:56 DEBUG goto relative screen: -1 in mode: page 
04/18/24-00:46:56 DEBUG CreDocument: goto page 399 flow 0 
04/18/24-00:46:56 DEBUG _refresh: Enqueued partial update for region 0 0 600 800 dithering: false 
04/18/24-00:46:56 DEBUG setDirty partial from widget ReaderUI w/ NO region; dithering: nil 
04/18/24-00:46:56 DEBUG painting widget: ReaderUI 
04/18/24-00:46:56 DEBUG readerview painting "600x800+0+0" to 0 0 
04/18/24-00:46:56 DEBUG CreDocument: goto page 399 flow 0 
04/18/24-00:46:56 DEBUG blitFrom 251 786 0 0 5 5 
04/18/24-00:46:56 DEBUG triggering refresh {
  mode = "partial",
  region = "600x800+0+0"
} --[[table: 0x4146f3a8]] 
04/18/24-00:46:56 DEBUG key event => code: 105 (Left), value: 0, time: 1713401216.428488 
04/18/24-00:46:56 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:46:58 DEBUG key event => code: 108 (Down), value: 1, time: 1713401217.997695 
04/18/24-00:46:58 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:46:58 DEBUG goto relative screen: 1 in mode: page 
04/18/24-00:46:58 DEBUG CreDocument: goto page 400 flow 0 
04/18/24-00:46:58 DEBUG _refresh: Enqueued partial update for region 0 0 600 800 dithering: false 
04/18/24-00:46:58 DEBUG setDirty partial from widget ReaderUI w/ NO region; dithering: nil 
04/18/24-00:46:58 DEBUG painting widget: ReaderUI 
04/18/24-00:46:58 DEBUG readerview painting "600x800+0+0" to 0 0 
04/18/24-00:46:58 DEBUG CreDocument: goto page 400 flow 0 
04/18/24-00:46:58 DEBUG blitFrom 251 786 0 0 5 5 
04/18/24-00:46:58 DEBUG triggering refresh {
  mode = "partial",
  region = "600x800+0+0"
} --[[table: 0x414b9398]] 
04/18/24-00:46:58 DEBUG key event => code: 108 (Down), value: 0, time: 1713401218.170597 
04/18/24-00:46:58 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:47:00 DEBUG input event => type: 4 (EV_MSC), code: 4 (nil), value: 0, time: 1713401220.147873 
04/18/24-00:47:00 DEBUG key event => code: 29 (ScreenKB), value: 1, time: 1713401220.147999 
04/18/24-00:47:00 DEBUG input event => type: 0 (EV_SYN), code: 0 (SYN_REPORT), value: 0, time: 1713401220.148722 
04/18/24-00:47:00 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:47:00 DEBUG input event => type: 4 (EV_MSC), code: 4 (nil), value: 0, time: 1713401220.425290 
04/18/24-00:47:00 DEBUG key event => code: 29 (ScreenKB), value: 0, time: 1713401220.425303 
04/18/24-00:47:00 DEBUG input event => type: 0 (EV_SYN), code: 0 (SYN_REPORT), value: 0, time: 1713401220.425306 
04/18/24-00:47:00 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:47:01 DEBUG Explicit widgetRepaint: table: 0x41479d30 @ 0 0 
04/18/24-00:47:01 DEBUG blitFrom 251 786 0 0 5 5 
04/18/24-00:47:01 DEBUG setDirty via a func from widget nil 
04/18/24-00:47:01 DEBUG ReaderFooter: scheduled autoRefreshFooter 
04/18/24-00:47:01 DEBUG _refresh: Enqueued ui update for region 0 786 600 14 dithering: false 
04/18/24-00:47:01 DEBUG triggering refresh {
  mode = "ui",
  region = "600x14+0+786"
} --[[table: 0x43e37450]] 
04/18/24-00:47:04 DEBUG input event => type: 4 (EV_MSC), code: 4 (nil), value: 7, time: 1713401224.036201 
04/18/24-00:47:04 DEBUG key event => code: 158 (Back), value: 1, time: 1713401224.036287 
04/18/24-00:47:04 DEBUG input event => type: 0 (EV_SYN), code: 0 (SYN_REPORT), value: 0, time: 1713401224.036958 
04/18/24-00:47:04 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:47:04 DEBUG ImageWidget: _render'ing resources/icons/mdlight/notice-question.svg 40 40 
04/18/24-00:47:04 DEBUG renderSVG resources/icons/mdlight/notice-question.svg 0.83333333333333 48 48 > 40 40 0 0 
04/18/24-00:47:04 DEBUG cache image|resources/icons/mdlight/notice-question.svg|40|40true 
04/18/24-00:47:04 DEBUG ImageWidget: initial offsets 0 0 
04/18/24-00:47:04 DEBUG Found font: NotoSans-Bold.ttf in ./fonts/noto/NotoSans-Bold.ttf 
04/18/24-00:47:04 DEBUG FocusManager: Move focus position to: 1 , 1 
04/18/24-00:47:04 DEBUG show widget: table: 0x4260fc60 
04/18/24-00:47:04 DEBUG setDirty nil from widget table: 0x4260fc60 w/ NO region; dithering: nil 
04/18/24-00:47:04 DEBUG setDirty via a func from widget table: 0x4260fc60 
04/18/24-00:47:04 DEBUG painting widget: table: 0x4260fc60 
04/18/24-00:47:04 DEBUG blitFrom 75 355 0 0 40 40 
04/18/24-00:47:04 DEBUG _refresh: Enqueued ui update for region 63 343 474 114 dithering: false 
04/18/24-00:47:04 DEBUG triggering refresh {
  mode = "ui",
  region = "474x114+63+343"
} --[[table: 0x4107f4b0]] 
04/18/24-00:47:04 DEBUG input event => type: 4 (EV_MSC), code: 4 (nil), value: 7, time: 1713401224.255304 
04/18/24-00:47:04 DEBUG key event => code: 158 (Back), value: 0, time: 1713401224.255318 
04/18/24-00:47:04 DEBUG input event => type: 0 (EV_SYN), code: 0 (SYN_REPORT), value: 0, time: 1713401224.255321 
04/18/24-00:47:04 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:47:05 DEBUG key event => code: 106 (Right), value: 1, time: 1713401225.372019 
04/18/24-00:47:05 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:47:05 DEBUG FocusManager cursor position is: 2 , 1 
04/18/24-00:47:05 DEBUG _refresh: Enqueued fast update for region 0 0 600 800 dithering: false 
04/18/24-00:47:05 DEBUG setDirty fast from widget table: 0x4260fc60 w/ NO region; dithering: nil 
04/18/24-00:47:05 DEBUG painting widget: table: 0x4260fc60 
04/18/24-00:47:05 DEBUG blitFrom 75 355 0 0 40 40 
04/18/24-00:47:05 DEBUG triggering refresh {
  mode = "fast",
  region = "600x800+0+0"
} --[[table: 0x4082bad8]] 
04/18/24-00:47:05 DEBUG key event => code: 106 (Right), value: 0, time: 1713401225.525515 
04/18/24-00:47:05 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:47:07 DEBUG key event => code: 194 (Press), value: 1, time: 1713401227.073807 
04/18/24-00:47:07 DEBUG AutoSuspend: onInputEvent 
04/18/24-00:47:07 DEBUG FocusManager: Send tap to 412 , 428 
04/18/24-00:47:07 DEBUG clear selection 
04/18/24-00:47:07 DEBUG closing reader 
04/18/24-00:47:07 DEBUG closing document 
04/18/24-00:47:07 DEBUG ReaderFooter: unschedule autoRefreshFooter 
04/18/24-00:47:07 DEBUG ReaderCoptListener.headerRefresh unscheduled 
04/18/24-00:47:07 DEBUG cre cache used: ./cache/cr3cache/Kite_Runner_-ni_105_.epub.a4133664.1.cr3 
04/18/24-00:47:07 DEBUG close widget: ReaderUI 
04/18/24-00:47:07 DEBUG DocSettings: Writing to /mnt/us/ePubs/Kite Runner, The - Khaled Hosseini (105).sdr/metadata.epub.lua 
04/18/24-00:47:07 DEBUG AutoSuspend: onCloseWidget 
04/18/24-00:47:07 DEBUG AutoSuspend: unschedule suspend/shutdown timer 
04/18/24-00:47:07 DEBUG AutoSuspend: unschedule t1 timeout timer 
04/18/24-00:47:07 DEBUG Tearing down ReaderUI table: 0x4296bd98 
04/18/24-00:47:07 DEBUG setDirty nil from widget table: 0x4260fc60 w/ NO region; dithering: nil 
04/18/24-00:47:07 DEBUG _refresh: Enqueued full update for region 0 0 600 800 dithering: false 
04/18/24-00:47:07 DEBUG close widget: table: 0x4260fc60 
04/18/24-00:47:07 DEBUG setDirty via a func from widget nil 
04/18/24-00:47:07 INFO  UIManager: No dialogs left to show 
04/18/24-00:47:07 INFO  Tearing down UIManager with exit code: 0 
[ko-input] Closed input device with fd: 6 @ idx: 2
[ko-input] Closed input device with fd: 5 @ idx: 1
[ko-input] Closed input device with fd: 4 @ idx: 0
lipc-wait-event exited normally with status: 0

@Frenzie
Copy link
Member

Frenzie commented Apr 18, 2024

Okay, so simply Left, Right, Up, Down, and Press.

For the reader we can probably simply remove the not hasFewKeys from these lines and add some new ones for the chapter stuff.

self.key_events.GotoNextPage = {
{ { "RPgFwd", "LPgFwd", not Device:hasFewKeys() and "Right" } },
event = "GotoViewRel",
args = 1,
}
self.key_events.GotoPrevPage = {
{ { "RPgBack", "LPgBack", not Device:hasFewKeys() and "Left" } },
event = "GotoViewRel",
args = -1,
}

The rest of what you said I don't have time to think about right this moment. ;-)

@Commodore64user
Copy link
Contributor Author

I've removed the hasFewKeys but continues to work the same way, I've also created two more but don't know what to use in the event field.

self.key_events.GotoNextPage = {
    { "RPgFwd", "LPgFwd" },
    event = "GotoViewRel",
    args = 1,
}
self.key_events.GotoPrevPage = {
    { "RPgBack", "LPgBack" },
    event = "GotoViewRel",
    args = -1,
}
-- self.key_events.GotoNextChapter = {
--     { "Right" },
--     event = "",
--     args = 1,
-- }
-- self.key_events.GotoPrevChapter = {
--     { "Left" },
--     event = "",
--     args = -1,
-- }

@Frenzie
Copy link
Member

Frenzie commented Apr 18, 2024

Ah sorry, that was slightly more of a note to self, but you're probably testing ReaderRolling:

if Device:hasKeys() then
self.key_events.GotoNextView = {
{ { "RPgFwd", "LPgFwd", "Right" } },
event = "GotoViewRel",
args = 1,
}
self.key_events.GotoPrevView = {
{ { "RPgBack", "LPgBack", "Left" } },
event = "GotoViewRel",
args = -1,
}
end

It's broadly speaking PDF/DjVu/CBZ/etc. ("paged") vs EPUB/FB2/etc (er, "rolling" in the code but something like "dynamic" might be more descriptive).

@Frenzie
Copy link
Member

Frenzie commented Apr 18, 2024

PS I think GotoNextChapter should be correct.

@Commodore64user
Copy link
Contributor Author

Ah sorry, that was slightly more of a note to self

no worries, there is still the up/down that triggers "start content selection". But that would require writing a new function and I can't do that one. should I make a PR for the left/right stuff?

@Frenzie
Copy link
Member

Frenzie commented Apr 18, 2024

hasFewKeys primarily implies that a device doesn't have a back button, which really gets in the way of regular navigation. For a proper interface at a minimum you want four directions, back, and enter. Unfortunately that doesn't imply also having page up/down.

Annoyingly, I don't really see a way out of that without hasPageUpDownKeys, although it's possible that it's better left as a user patch.

@Commodore64user
Copy link
Contributor Author

this is what I did:

    if Device:hasKeys() then
        self.key_events.GotoNextView = {
            { { "RPgFwd", "LPgFwd" } },
            event = "GotoViewRel",
            args = 1,
        }
        self.key_events.GotoPrevView = {
            { { "RPgBack", "LPgBack" } },
            event = "GotoViewRel",
            args = -1,
        }
    end
    if Device:hasDPad() then
        self.key_events.MoveUp = {
            { "Up" },
            event = "Panning",
            args = {0, -1},
        }
        self.key_events.MoveDown = {
            { "Down" },
            event = "Panning",
            args = {0,  1},
        }
        self.key_events.GotoNextChapter = {
            { "Right" },
            event = "GotoNextChapter",
            args = 1,
        }
        self.key_events.GotoPrevChapter = {
            { "Left" },
            event = "GotoPrevChapter",
            args = -1,
        }
    end

@Commodore64user
Copy link
Contributor Author

Commodore64user commented Apr 18, 2024

Kindles definitely have back buttons, and a d-pad

@Frenzie
Copy link
Member

Frenzie commented Apr 18, 2024

It's not Kindles I'm talking about. My point is we don't want to make something else unusable where the left/right thing isn't redundant but actively necessary.

@Commodore64user
Copy link
Contributor Author

Commodore64user commented Apr 18, 2024

It's not Kindles I'm talking about. My point is we don't want to make something else unusable where the left/right thing isn't redundant but actively necessary.

What about applying the changes only to isKindle() that way no other devices would be affected.

So even this #11295 (comment) could affect someone using a random unknown non-touch device?

@Frenzie
Copy link
Member

Frenzie commented Apr 18, 2024

That's the bad version of hasPageUpDownKeys. ;-)

@Commodore64user
Copy link
Contributor Author

But isn’t hasPageUpDownKeys a subgroup of hasDPad?

@Frenzie
Copy link
Member

Frenzie commented Apr 18, 2024

Currently it's merely the outline of a potential concept.

But I think it is (or rather, would be) its own independent thing. It's only the combination of hasKeys and/or hasDPad together with hasPageUpDownKeys that would enable a repurposing of left/right for chapter navigation.

@Frenzie
Copy link
Member

Frenzie commented Apr 18, 2024

But to explain a bit more, something like isKindle() should almost never be seen in code, or at least not at this level of abstraction. canDoSwipeAnimation() is currently Kindle-exclusive but that way it's useful in the future rather trouble in the future.

local Device = {
screen_saver_mode = false,
screen_saver_lock = false,
is_cover_closed = false,
model = nil,
powerd = nil,
screen = nil,
input = nil,
home_dir = nil,
-- For Kobo, wait at least 15 seconds before calling suspend script. Otherwise, suspend might
-- fail and the battery will be drained while we are in screensaver mode
suspend_wait_timeout = 15,
-- hardware feature tests: (these are functions!)
hasBattery = yes,
hasAuxBattery = no,
hasKeyboard = no,
hasKeys = no,
canKeyRepeat = no,
hasDPad = no,
hasExitOptions = yes,
hasFewKeys = no,
hasWifiToggle = yes,
hasSeamlessWifiToggle = yes, -- Can toggle Wi-Fi without focus loss and extra user interaction (i.e., not Android)
hasWifiManager = no,
hasWifiRestore = no,
isDefaultFullscreen = yes,
isHapticFeedbackEnabled = no,
isDeprecated = no, -- device no longer receive OTA updates
isTouchDevice = no,
hasFrontlight = no,
hasNaturalLight = no, -- FL warmth implementation specific to NTX boards (Kobo, Cervantes)
hasNaturalLightMixer = no, -- Same, but only found on newer boards
hasNaturalLightApi = no,
hasClipboard = yes, -- generic internal clipboard on all devices
hasEinkScreen = yes,
hasExternalSD = no, -- or other storage volume that cannot be accessed using the File Manager
canHWDither = no,
canHWInvert = no,
canDoSwipeAnimation = no,
canModifyFBInfo = no, -- some NTX boards do wonky things with the rotate flag after a FBIOPUT_VSCREENINFO ioctl
canUseCBB = yes, -- The C BB maintains a 1:1 feature parity with the Lua BB, except that is has NO support for BB4, and limited support for BBRGB24
hasColorScreen = no,
hasBGRFrameBuffer = no,
canImportFiles = no,
canShareText = no,
hasGSensor = no,
isGSensorLocked = no,
canToggleMassStorage = no,
canToggleChargingLED = no,
_updateChargingLED = nil,
canUseWAL = yes, -- requires mmap'ed I/O on the target FS
canRestart = yes,
canSuspend = no,
canStandby = no,
canPowerSaveWhileCharging = no,
total_standby_time = 0, -- total time spent in standby
last_standby_time = 0,
total_suspend_time = 0, -- total time spent in suspend
last_suspend_time = 0,
canReboot = no,
canPowerOff = no,
canAssociateFileExtensions = no,
-- Start and stop text input mode (e.g. open soft keyboard, etc)
startTextInput = function() end,
stopTextInput = function() end,
-- use these only as a last resort. We should abstract the functionality
-- and have device dependent implementations in the corresponting
-- device/<devicetype>/device.lua file
-- (these are functions!)
isAndroid = no,
isCervantes = no,
isKindle = no,
isKobo = no,
isPocketBook = no,
isRemarkable = no,
isSonyPRSTUX = no,
isSDL = no,
isEmulator = no,
isDesktop = no,
-- some devices have part of their screen covered by the bezel
viewport = nil,
-- enforce portrait orientation of display when FB defaults to landscape
isAlwaysPortrait = no,
-- On some devices (eg newer pocketbook) we can force HW rotation on the fly (before each update)
-- The value here is table of 4 elements mapping the sensible linux constants to whatever
-- nonsense the device actually has. Canonically it should return { 0, 1, 2, 3 } if the device
-- matches <linux/fb.h> FB_ROTATE_* constants.
-- See https://github.com/koreader/koreader-base/blob/master/ffi/framebuffer.lua for full template
-- of the table expected.
usingForcedRotation = function() return nil end,
-- needs full screen refresh when resumed from screensaver?
needsScreenRefreshAfterResume = yes,
-- set to yes on devices that support over-the-air incremental updates.
hasOTAUpdates = no,
-- For devices that have non-blocking OTA updates, this function will return true if the download is currently running.
hasOTARunning = no,
-- set to yes on devices that have a non-blocking isWifiOn implementation
-- (c.f., https://github.com/koreader/koreader/pull/5211#issuecomment-521304139)
hasFastWifiStatusQuery = no,
-- set to yes on devices with system fonts
hasSystemFonts = no,
canOpenLink = no,
openLink = no,
canExternalDictLookup = no,
}

@Commodore64user
Copy link
Contributor Author

I see, so a little more complex than expected. What would I need to do to re-map locally (user patch) the up/down buttons to 'start content selection' and the keyboard button to do something? For now at least.

@Commodore64user
Copy link
Contributor Author

Commodore64user commented Apr 18, 2024

Screenshots, that too. Capturing them i mean.

@Commodore64user
Copy link
Contributor Author

Wait a second, but the hasFewKeys is only used for PDF/DjVu/CBZ/etc. Seems to me ePubs are not constrained by that? Or am i reading something wrong?

@Frenzie
Copy link
Member

Frenzie commented Apr 18, 2024

That is likely an oversight on my end. Quoting from #6195:

In the reader, the right is repurposed to open the menu. Left in turn closes it.

I'm building the program right now, but on this older laptop it'll take a while.

So on a normal device, left/right and up/down are already in use for page turning vs relative position changing. The former is somewhat similar in purpose to what you'd prefer to be chapters instead. (A PDF page is typically taller than the screen, meaning it'll take two page turns to get through fully.)

@Commodore64user
Copy link
Contributor Author

another thing, for some reason the top menu does not respect the hierarchy of the 'Home' button. When you go deep into the menus the 'home' button doesn't do anything, when it should take you back to the file browser.

@Frenzie
Copy link
Member

Frenzie commented Apr 19, 2024

Home button?

@Commodore64user
Copy link
Contributor Author

Commodore64user commented Apr 19, 2024

Screenshot 2024-04-19 at 15 48 56

Here is a picture of a kindle 4/5 non-touch. They have: back, keyboard, menu and Home buttons on top of the D-Pad and Enter. They are all underutilised in KOReader.

and two sets of page turn button of course.

@Frenzie
Copy link
Member

Frenzie commented May 4, 2024

okay, how about this https://github.com/koreader/koreader/compare/master...Commodore64user:KOReader_fork:hasPageUpDownKeys?expand=1

Is the inversion between the conditions on purpose?

@Commodore64user
Copy link
Contributor Author

Is the inversion between the conditions on purpose?

do you mean if hasPageUpDownKeys and hasDPad then and if hasDPad and hasPageUpDownKeys then? if so, yes, kind of, I think it makes it easier for someone else to understand (maybe I am wrong?) that if hasDPad and hasPageUpDownKeys is a subset of hasDPad, same for the other one where hasPageUpDownKeys is a subset of hasKeys, although that might be less clear to someone unfamiliar with those functions/tags.

I thought about having the whole thing under one single if but then it would make the whole thing more complicated as nesting would be involved.

so self.ui.highlight:onStartHighlightIndicator() should work.

well, well, well. Look who decided to show up. My favourite pear (i.e poire), version -z, saving the day once again. Thanks for the explanation!

@Frenzie
Copy link
Member

Frenzie commented May 4, 2024

I think it makes it easier for someone else to understand (maybe I am wrong?)

I don't think that's wrong per se, but it does harm greppability.

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 4, 2024

but it does harm greppability.

not sure what it means, but it sounds terrible. In that case they could both be hasPageUpDownKeys and hasDPad, does not bother me at all.

https://github.com/koreader/koreader/compare/master...Commodore64user:KOReader_fork:hasPageUpDownKeys?expand=1

now it is time to have that conversation, my recent commit does now break panning when on continuous mode. what to do? I tried adding a nested if self.view.view_mode == "scroll" and reverting up/down back to panning but it didn't seem to like that.

@Frenzie
Copy link
Member

Frenzie commented May 4, 2024

not sure what it means, but it sounds terrible. In that case they could both be hasPageUpDownKeys and hasDPad, does not bother me at all.

It means that if you search for "this and that" to see if it occurs anywhere you'll miss "that and this". grep in its various forms is the standard text searching program. So yes, make them the same please.

now it is time to have that conversation, my recent commit does now break panning when on continuous mode. what to do? I tried adding a nested if self.view.view_mode == "scroll" and reverting up/down back to panning but it didn't seem to like that.

That wouldn't work; the event handlers are registered on load.

How bothered are we about continuous mode there @poire-z, if at all?

@poire-z
Copy link
Contributor

poire-z commented May 4, 2024

How bothered are we about continuous mode there @poire-z, if at all?

Haven't followed what's all this all about (happily not having a non-touch device, so not bothering), and not sure what you're asking me :/
Quick looking at the PR, if I get it right:
For devices with pageUp/pageDown, PageUp/pageDown moves one page (in page and in scroll mode). And because these PgUP/PgDown are available, we repurpose ArrowUp/Down (which did next/prev pages too, in page mode) to do "content selection" (= start text seection ?) ? And because of that, we lose the effect arrowUp/Down had in scroll mode where it moved by a fraction of the screen height (so, a few lines)?
If I get it right, I do think it's sad to lose that scroll by a few lines. I guess I'll miss that on the emulator.

If I got it right and you want to fix that, I guess you should send new events in your new code for ArrowUp/Down, and in the new handler(s), dispatch new events depending on the mode.

I'm not really familiar with the non-touch stuff, and I don't really know the differences between hasKeys/hasDpad - and what the emulator "has".
And what keys NT Kindle users do use to change page - the equivalent of PgUp or ArrowUp? No risk of breaking anyone workflows if both did the same work? And they will have now different effects whether in page mode (content selection) and scroll mode (scroll by a few lines), so people possibly getting a confusing and non-consistent behaviour in one mode or the other.

And it seems it's all again different with PDF (no content selection?) :/

Again, not sure if I got it right and if it changes that much stuff, would be nice to ping some other Kindle NT users to discuss the changes and agree on some new behaviour if it's that much.

@Frenzie
Copy link
Member

Frenzie commented May 4, 2024

If I get it right, I do think it's sad to lose that scroll by a few lines. I guess I'll miss that on the emulator.

Yes, me too. I think it's the evidently correct behavior but that might differ per device.

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 4, 2024

For devices with pageUp/pageDown, PageUp/pageDown moves one page (in page and in scroll mode). And because these PgUP/PgDown are available, we repurpose ArrowUp/Down (which did next/prev pages too, in page mode) to do "content selection" (= start text seection ?) ? And because of that, we lose the effect arrowUp/Down had in scroll mode where it moved by a fraction of the screen height (so, a few lines)?
If I get it right, I do think it's sad to lose that scroll by a few lines. I guess I'll miss that on the emulator.

exactly that, only this change should target kindles exclusively, in the words of Frenzie hasPageUpDownKeys and hasDPad is a rich man's isKindle, so the emulator should be unaffected.

@Frenzie
Copy link
Member

Frenzie commented May 4, 2024

What I said was closer to calling that code smell.

But the fact is:

a. we like the current behavior
b. the current behavior was originally implemented for Kindle and only for Kindle

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 4, 2024

a. we like the current behavior

with all due respect, you might do, but you don't use non-touch kindles.

b. the current behavior was originally implemented for Kindle and only for Kindle

yes, but there was no "content selection" at that time. why anyone thought having four sets of buttons doing the same was a great idea is beyond me. #4329 here is another example a somewhat similar suggestion btw

all this is doing is simply somewhat replicating stock firmware behaviour, which is superior in this case. having said that, it is possibly less urgent to have both (one is probably enough) up/down calling content selection (btw this is dict lookup and highlights) here but for now can't think of another thing to add there.

@Frenzie
Copy link
Member

Frenzie commented May 4, 2024

with all due respect, you might do, but you don't use non-touch kindles.

Exactly, that's my point. :-) Everything has to be carefully considered, not just non-touch Kindles. And on non-touch Kindles you also have to consider everything, not just one particular document type in one particular view mode.

But we'll get there.

@Frenzie
Copy link
Member

Frenzie commented May 4, 2024

This might do the trick:

    if not Device:hasKeyboard() and Device:hasDPad() and Device:hasPageUpDownKeys() then
  • hasKeyboard has better ways to trigger functionality (e.g., press h for highlight), so we want to maintain d-pad convenience when hasKeyboard() is true
  • hasDPad by itself doesn't guarantee separate page up/down buttons (even if that might potentially be the case today), so:
  • hasPageUpDownKeys means we can get rid of page turning functionality on the d-pad

@Frenzie
Copy link
Member

Frenzie commented May 4, 2024

    if not Device:hasKeyboard() and Device:hasDPad() and Device:hasPageUpDownKeys() then

It might be preferable to abstract that away in a different manner to make it explicit rather than fully emergent. That would probably make it more obvious to future readers why it's doing these things.

Something along these lines for example:

diff --git a/frontend/device/generic/device.lua b/frontend/device/generic/device.lua
index a2f24426c..0e83dfe9b 100644
--- a/frontend/device/generic/device.lua
+++ b/frontend/device/generic/device.lua
@@ -43,6 +43,7 @@ local Device = {
     hasKeys = no,
     canKeyRepeat = no,
     hasDPad = no,
+    hasDPadWeaponSelect = no,
     hasExitOptions = yes,
     hasFewKeys = no,
     hasWifiToggle = yes,
@@ -231,6 +232,10 @@ function Device:init()
         end
     end
 
+    if not Device:hasKeyboard() and Device:hasDPad() and Device:hasPageUpDownKeys() then
+        self.hasDPadWeaponSelect = yes
+    end
+
     if self:hasGSensor() then
         -- Setup our standard gyro event handler (EV_MSC:MSC_GYRO)
         if G_reader_settings:nilOrFalse("input_ignore_gsensor") then

@poire-z
Copy link
Contributor

poire-z commented May 4, 2024

Is that the device ? http://www.btobey.com/review/kindle-4-buttons.jpg
And the PgUP/Down buttons are on the ones on the side ?
Do people really use them ?! I'd find using the ones on the middle 5xsubbuttons-button easier, holding the device by its bottom. And if I do, other people may.
(So, may be it should be an option ? :/)

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 4, 2024

This might do the trick:

    if not Device:hasKeyboard() and Device:hasDPad() and Device:hasPageUpDownKeys() then
  • hasKeyboard has better ways to trigger functionality (e.g., press h for highlight), so we want to maintain d-pad convenience when hasKeyboard() is true

  • hasDPad by itself doesn't guarantee separate page up/down buttons (even if that might potentially be the case today), so:

  • hasPageUpDownKeys means we can get rid of page turning functionality on the d-pad

But this is fragmenting everything even more, and i'll be the devil's advocate this time, that does not seem good. I might as well say "model == Kindle4" or whatever it is. I think it is better to cover all non-touch equally in terms of dPad options.

How about adding an option under "cog > navigation > physical buttons" where you choose either panning or content selection, even more options could be added (and force a restart like when activating/deactivating plugins). But that would require some effort from your end as i doubt i could pull that off.

Edit: should i make a PR now so we can all commit stuff?

edit 2: I want to make extremely clear, content selection isn't just highlights. It is probably 80% dictionary lookups, 15% full text searches, 4% highlights and 1% others (yes I made up the numbers, but largely the idea holds true).

@Frenzie
Copy link
Member

Frenzie commented May 4, 2024

But this is fragmenting everything even more, and i'll be the devil's advocate this time, that does not seem good. I might as well say "model == Kindle4" or whatever it is. I think it is better to cover all non-touch equally in terms of dPad options.

People who like to use the d-pad would beg to differ. But I never said that adapting to the situation on the ground was bad, only that == Kindle4 is almost always the wrong way to do so.

How about adding an option under "cog > navigation > physical buttons" where you choose either panning or content selection, even more options could be added (and force a restart like when activating/deactivating plugins). But that would require some effort from your end as i doubt i could pull that off.

I suppose so. The defaults should still be as stated above or much like it regardless though.

Edit: should i make a PR now so we can all commit stuff?

At least as far as code goes that's easier to talk about, yes.

@Commodore64user
Copy link
Contributor Author

Is that the device ?

Yes, it is. There was a picture in an earlier #11295 (comment)

And the PgUP/Down buttons are on the ones on the side?

Yes, on both sides for right or left hand holding.

Do people really use them?

Yes, although I personally prefer the page-turn-button style on the previous kindle 3 (keyboard), the device is light and quite easy to hold.

I'd find using the ones on the middle 5xsubbuttons-button easier, holding the device by its bottom. And if I do, other people may.

I can't argue with that, preferences are indeed a personal thing.

(So, may be it should be an option ? :/)

That seems to be where we all have finally landed. yay all of us. #11749 is now open, so if you find yourself in need of helping someone out, feel free to drop by.

@poire-z
Copy link
Contributor

poire-z commented May 6, 2024

#11749 is now open, so if you find yourself in need of helping someone out, feel free to drop by.

Unlike Little Timmy, I'm quite disciplined: it's written "Non-touch", so non-touching it :)

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 7, 2024

Hello all,

So I am trying to bring screenshots to non-touch devices but I was found wanting.

for reference this is our file and init()

function Screenshoter:init()
local diagonal = math.sqrt(Screen:getWidth()^2 + Screen:getHeight()^2)
self.ges_events = {
TapDiagonal = {
GestureRange:new{
ges = "two_finger_tap",
scale = {diagonal - Screen:scaleBySize(200), diagonal},
rate = 1.0,
}
},
SwipeDiagonal = {
GestureRange:new{
ges = "swipe",
scale = {diagonal - Screen:scaleBySize(200), diagonal},
rate = 1.0,
}
},
}
end

and I am adding something like this to it (i.e init()):

if not Device:isTouchDevice() then
    if Device:hasKeyboard() then
        -- self.key_events = {
        --     KeyPressShoot = {
        --         Screenshot = { { "Shift" and "S" } },
        --     },
        -- }
    elseif Device:hasKeys() then     
        self.key_events.KeyPressShoot = { { { "Press", "Menu" } }, event = "KeyPressShoot", args = 1 }
    end
end

and this function of course:

function Screenshoter:onKeyPressShoot()
    return self:onScreenshot()
end

but it is not working as expected. With that code, Press alone is triggering the screenshots. any help would be highly appreciated. BTW, I am focusing on the elseif just now, so ignore the hasKeyboard if you want.

ps: I pay the screenshot tax (kindle 4) yay!

@hius07
Copy link
Member

hius07 commented May 7, 2024

Try to remove one pair of curly brackets.

@Commodore64user
Copy link
Contributor Author

Try to remove one pair of curly brackets.

I added the extra one { } in the first place because it would not work without them, there is something else I am clearly ignoring but not sure what that might be.

@davigamer987
Copy link

Would be nice if there was a way to map the unused keyboard key on Kindle 4 to the selection mode

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 7, 2024

Would be nice if there was a way to map the unused keyboard key on Kindle 4 to the selection mode

there is a PR #11749 (comment) that will add better support, including selection mode tied to a press key, maybe not the ScreenKB key as it would not be consistent with the older kindles with keyboards.

@Commodore64user
Copy link
Contributor Author

Commodore64user commented May 7, 2024

trying to run this now

self.key_events.KeyPressShoot = { {Key:match({ "ScreenKB", "Menu" })}, event = "KeyPressShoot", args = 1, }

ps. I changed from "Press" to "ScreenKB", more inoffensive less offensive that way. ScreenKB + Menu = screenshot:
I get this:

05/07/24-13:33:59 DEBUG input event => type: 4 (EV_MSC), code: 4 (nil), value: 0, time: 1715088839.177812 
05/07/24-13:33:59 DEBUG key event => code: 29 (ScreenKB), value: 0, time: 1715088839.177827 
05/07/24-13:33:59 DEBUG input event => type: 4 (EV_MSC), code: 4 (nil), value: 1, time: 1715088839.177836 
05/07/24-13:33:59 DEBUG key event => code: 139 (Menu), value: 0, time: 1715088839.177964 
05/07/24-13:33:59 DEBUG input event => type: 0 (EV_SYN), code: 0 (SYN_REPORT), value: 0, time: 1715088839.177968 
05/07/24-13:33:59 DEBUG AutoSuspend: onInputEvent 

edit: I believe this does not work because neither of those keys are modifiers, hmmm. any advice?

@Commodore64user
Copy link
Contributor Author

Note to self:

self.key_events.Back = { { Input.group.Back } }

Frenzie pushed a commit that referenced this issue May 20, 2024
@Frenzie Frenzie added this to the 2024.06 milestone May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants