BookMap on devices with useDPadAsActionKeys()

[Commodore64user]

as first discussed here #11908. This PR brings the book map to non-touch devices that useDPadAsActionKeys().

Book map can be accessed from the menu or by using the following shortcut: ScreenKB + Down or Shift + Down depending on whether you use a K4 device or a kindle with keyboard respectively.

Inside the book map, a user can toggle the hamburger menu by pressing the Menu key and make any adjustment from there. ScreenKB (or Shift) + Up/Down allows it to scroll and Page turn buttons to move by whole full page turns. Back key allows user to exit the map.

---

This change is 

[poire-z]

I somehow read "Book map" as a thing, like "iPad Air", so "The book map" sounds odd to me.
Also, "provides a summary" feels misleading, summary implying a bunch of words to me.
"overview" (as a view over, from above, like a plan) feels more true to the feature.

"showing chapters and pages visually" summarize with less info the 2 list items you removed below. I get someone will understand that anyway, but still, again, style vs. precision.

[Commodore64user]

Is not always the case that the sum of its parts equals the meaning of a word, regarding overview: yes it originally meant that (view from above) but now that is archaic, on its way out, and the word means primarily “ to review, summarise, take an overview of.”

summary is “the essence or essential part of something”

[poire-z]

@Frenzie @NiLuJe : any more thoughts about these wording (this comment, and the one above, which has become Page-bar width since).

[Frenzie]

Seems fine to me, provided it's all technically accurate but you'd have said something about that.

[poire-z]

You're ok with?

-Book map displays an overview of the book content
+The book map provides a summary of the book's content

You don't share my non-native-english feelings?

[Frenzie]

Define it. The current wording, or the diff chunk in question in this PR? (i.e., ack or nack on the change ;)).

I wouldn't change "book map" myself because it's a thing we decided on a couple of years ago that I'm fine with. It's like how in Excel you'd say "AutoSum does this and that" rather than "The AutoSum." But "the book map" sounds fine too.

[poire-z]

I don't buy these the shoes street cred/crap subtleties. That's now how we do marketing here !

Anyway, the only reason for going with "The book map" is that translators already went that way :/
"De paginakaart toont een overzicht van de boekinhoud.\n"
"O mapa do livro exibe uma visão geral do conteúdo do livro.\n"
"Die Buchkarte zeigt eine Ã~\bersicht über den Inhalt des Buches.\n" (at least, German kept the Uppercase :))
"El mapa del libro muestra una visión general del contenido del libro.\n"
"La carte du livre affiche un aperçu du contenu du livre.\n"
"Il navigatore delle pagine mostra miniature delle pagine.\n" (Italian mixed Book map with Page browser)

[Commodore64user]

so what is the final verdict here then...?

[Commodore64user]

do I change it or what?

[Frenzie]

Let's go with "Book map" without the determiner. @poire-z wrote it after all. ;-)

[poire-z]

Don't forget to check (and fix) the red cross

and follow the rabbit:

Checking frontend/apps/reader/modules/readerthumbnail.lua 2 warnings

    frontend/apps/reader/modules/readerthumbnail.lua:24:43: accessing undefined variable 'useDPadAsActionKeys'
    frontend/apps/reader/modules/readerthumbnail.lua:80:8: accessing undefined variable 'useDPadAsActionKeys'

Checking frontend/ui/widget/bookmapwidget.lua     1 warning

    frontend/ui/widget/bookmapwidget.lua:1356:249: trailing whitespace in a string

[Frenzie]

If yes, could i please get some time to test it

I'm mildly confused. Doesn't the PR imply you've already tested it?

But anyway, so you're done with it?

[Commodore64user]

If yes, could i please get some time to test it

I'm mildly confused. Doesn't the PR imply you've already tested it?

But anyway, so you're done with it?

Yes, done here.

by testing i mean, testing it for real, as a golden master or release candidate as apple calls them. Not just locally.

[Frenzie]

Alright, I'll leave the "Left" situation unresolved.

[Commodore64user]

thank god we are testing this, so there is a problem: KOReader won't open any ePubs or PDFs, I'm getting the following error "No reader engine for this file or invalid file"

frontend/apps/reader/modules/readerthumbnail.lua:72: attempt to index field 'key_events' (a nil value)
stack traceback:
	frontend/apps/reader/modules/readerthumbnail.lua:24: in function 'init'
	frontend/ui/widget/widget.lua:46: in function 'new'
	frontend/apps/reader/readerui.lua:403: in function 'init'
	frontend/ui/widget/widget.lua:46: in function 'new'
	frontend/apps/reader/readerui.lua:686: in function 'doShowReader'
	frontend/apps/reader/readerui.lua:639: in function <frontend/apps/reader/readerui.lua:638>

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

Line 72 in 585afda

self.key_events.ShowBookMap = { { "ScreenKB", "Down" } }

[poire-z]

by testing i mean, testing it for real, as a golden master or release candidate as apple calls them. Not just locally.

How do you test when you make changes to an open PR ?
It looks to me that you should obviously have witnessed these "No reader engine for this file or invalid file" if you just copied your changed files to your device? How come you didn't witness them?

[poire-z]

Looks like you just need to change the base class (and change local InputContainer = require("ui/widget/container/inputcontainer")):

- local ReaderThumbnail = WidgetContainer:extend{}
+ local ReaderThumbnail = InputContainer:extend{

InputContainer does create self.key_events

[Frenzie]

The issue probably lies in keeping track of changes without git. It's a good thing to try to gain a little bit more familiarity with.

[Commodore64user]

by testing i mean, testing it for real, as a golden master or release candidate as apple calls them. Not just locally.

How do you test when you make changes to an open PR ?

It looks to me that you should obviously have witnessed these "No reader engine for this file or invalid file" if you just copied your changed files to your device? How come you didn't witness them?

What i think happened here, was that this 'impromptu' change never actually made it to my local build. But chill, this is why I wanted to test the actual release.

I will update it in a bit once back home.

[poire-z]

But chill, this is why I wanted to test the actual release.

No, an "actual release" is not for that. You should check your Lua files on your device(s) before we merge and ship it to the world of nightliy users.
This one screwed only NT users, with just "no engine", so not stictly screwing KOReader launch, still allowing users to update from inside KOReader to the next release.
But it could have been worse, with a KOReader not starting, and the need for users to manually fix the mess, and install a previous releease - and such users then staying away from nightlies and us getting less feedback.

So no more "impromptus"... We asked you if you had tested it, you should have tested it.

[Commodore64user]

I just want to make something very clear. I have been testing all of it. I have exclusively used the K4 for the last two or three weeks, putting everything through its paces. I am glad to have found issues and to have been able to fix them before other people get to update their devices.

Having said, as I said, the move from readerUI to rederthumbnail (which is what I called the "impromptu change") wasn't originally planned and somehow I missed doing it on my Kindle. I am not blaming anyone of course, but yes, I should have caught it earlier, but alas, here we are.

So what is the point of testing things if not to find issues? I am genuinely confused by that.

In any case, thank you! (once again)

should I remove

- local WidgetContainer = require("ui/widget/container/widgetcontainer")

?

[Frenzie]

Merging a PR is never without danger and niche conditions can fly under the radar, but such basic testing should occur prior to merging a PR.

[Commodore64user]

I’ll consider this my “lesson learnt” moment and will make sure that it does not happen again.

and well, at least we can print on a Tuesday.

[Commodore64user]

so I just realised that this is happening

for reference that "1" should be: Speed-up rate interval: 1 second

I am not sure if that was introduced here or something else is messing it up. Nevertheless any advice highly appreciated

[NiLuJe]

Does this help?

diff --git a/frontend/apps/reader/modules/readerhighlight.lua b/frontend/apps/reader/modules/readerhighlight.lua
index 57c9c5489..ab665ddca 100644
--- a/frontend/apps/reader/modules/readerhighlight.lua
+++ b/frontend/apps/reader/modules/readerhighlight.lua
@@ -639,7 +639,7 @@ Except when in two columns mode, where this is limited to showing only the previ
         table.insert(menu_items.long_press.sub_item_table, {
             text_func = function()
                 local highlight_non_touch_interval = G_reader_settings:readSetting("highlight_non_touch_interval") or 1
-                return T(N_("Speed-up rate interval: %1 second", "Speed-up rate interval: %1 seconds", highlight_non_touch_interval), highlight_non_touch_interval)
+                return T(N_("Speed-up rate interval: 1 second", "Speed-up rate interval: %1 seconds", highlight_non_touch_interval), highlight_non_touch_interval)
             end,
             enabled_func = function()
                 return not self.view.highlight.disabled and G_reader_settings:nilOrTrue("highlight_non_touch_spedup")

[Commodore64user]

yeah, there were two things actually, PRing the fix now, the other was with the local N_. Thanks!