NT Device: BookMap and PageBrowser

[comphilip]

Fix #11908

Changes Berif

• BookMap and PageBrowser menu items available on NT devices
• add focus_inner_border to FrameContainer, to use inner border as focused style to void focused widget height and width changed
• Use invisible but focus-able widget for page slot in BookMapRow widget
• About dialog in PageBrowser: describe keyboard shortcuts

---

This change is 

[Frenzie]

I think this may be based on a slightly older version of a PR (or at least includes its commits) that changed slightly before it was merged?

[Commodore64user]

I think this may be based on a slightly older version of a PR (or at least includes its commits) that changed slightly before it was merged?

is this about the home event? I forgot about it. I'll remove it later.

[Frenzie]

That jumps out, but as such I don't know what else might be slightly outdated, which isn't good. ;-)

[Commodore64user]

Then i am not sure what you are referring to.

[Frenzie]

That's exactly the point. I don't know. Or are you trying to say that none of these commits come from other PRs and they merely happen to look similar?

[Commodore64user]

well I guess I am going to have to plead ignorance here because I am completely baffled.

Or are you trying to say that none of these commits come from other PRs and they merely happen to look similar?

I suppose so, what do they look similar to? I mean besides, the home event (which incidentally predates the other one) nothing (as far as I know), comes from anywhere else.

[Frenzie]

You can base a branch on another branch, or import (cherry pick) commits from another branch. I thought that's what had happened here.

[Frenzie]

colon?

[Frenzie]

Suggested change

	Use settings in this menu to change the number of columns and rows to be shown, whether to display page numbers and, different chapter-levels in bottom ribbon.
	The settings can be used to change the number of rows and columns, whether to display page numbers, and to display different chapter-levels in the bottom ribbon.

[Frenzie]

Are those the going user friendly(ish) terms as will also be used in the user guide by @offset-torque?

[offset-torque]

We only use the specific terms "rows" and "columns" in the page browser section of the guide. As they are kept here, I don't see a problem. Please point out if I missed something Frenzie, this PR view is still a bit foreign to me.

[Frenzie]

I meant the LPgBack/RPgBack in the line I commented on. It's somewhat self-explanatory as left page back and right page back, but I'm not sure if it's sufficiently so.

[offset-torque]

Ok now I see. So we are still playing the "cryptic buttons" game. Those buttons have much more clearer official names and I will use these names in our guide of course. I can't say what to do here but I suggest avoiding this camel-case-variable-like naming especially in a help text. When I searched for "LPgBack" to check, Google brought me "liquefied petroleum gas back pressure check valves" so that's not even a searchable word.

[Frenzie]

Agreed. I figured maybe they came from the Kindle manual.

[Frenzie]

Same comment as https://github.com/koreader/koreader/pull/12579/files#r1784487514

[Frenzie]

It feels like more of a "that page" than a "this page," doesn't it?

[offset-torque]

Agree

[poire-z]

There are a bunch of English typos, ie. invisiable or foucs (yes, I do give a fouc :))

Also, on devices/platforms where we won't be using keyboard navigation, I'd like to request the focus_layout to not be populated at all.
It feels it may already be happening (thanks for the thought) thanks to local enable_invisiable_focus_page_slot = Device:hasDPad() for the page slots, but it could also be made for the span_w toc items.
My idea is that we don't want memory on Kobo and similar devices occupied by tables of tables of possibly thousands of items (1000 page slots on a 1000 pages book, but possible 500 toc spans on a book with lots of TOC entries).

[poire-z]

I guess this adds some padding around the TOC title in flat mode, just so they can be highlighted.
I guess the nice handmade alignment I tried to achieve:

will then be lost as every TOC title will get more air on all sides. Not tested, but not happy with that if it happens.

[comphilip]

Fixed

[Frenzie]

ie. invisiable or foucs

Perhaps we should indeed look into this codespell thing, so obvious ones like invisible will be caught by the machine (cf. #12566)

[Commodore64user]

hey @comphilip I understand that you might not have time to make this better but, could you look at fixing the issues pointed out here? also the one about the sticky focus I mentioned in #11908 (comment) (the first screenshot). Again thanks for everything.

[comphilip]

@Commodore64user Take a look. I can't think out a better solution yet.

[Commodore64user]

I expect @NiLuJe could provide better advice de637d0 😊😜

Also, not sure if you have seen this one as well #12579 (comment) @comphilip

[Commodore64user]

@poire-z is this the bit you say you would prefer not to be loaded on touch devices? i.e would this do?

-- NT: update layout
    if not Device:isTouchDevice() then
        -- remove existing BookMapRow from layout
        if #self.layout > self.nb_rows then
            for i = self.nb_rows + 1, #self.layout do
                self.layout[i] = nil
            end
        end
        -- add new BookMapRow layout
        for _, focus_row in ipairs(row.focus_layout) do
            table.insert(self.layout, focus_row)
        end
    end

[poire-z]

One of the bitS.
Yes, this and its brothers would do.
Rather create a self.build_focus_layout = true/false early, so if we ever need to change the condition, it will be in a single place.

[Commodore64user]

If you could single them out, it would make my job easier ;)

[poire-z]

Well, I'm not re-reading the whole thing.
I'd like anything that build some stuff and table.insert() it into a something_layout to be wrapped by this if self.build_focus_layout then.

Also, I think the rendering/shifting issue with using the border to highlight the focused item - cf #12583 (comment) - should be fixed.
ie FrameContainer could have a draw_only_border that would not interact with the sizing, but would just be drawn - outside or inside, I dunno, possibly overflowing on the inner content or outer neighbours, avoiding that being the job of the designer to provide some extra margin or padding.

[NiLuJe]

The VirtualKeyboard keys have a highlight frame that's drawn inside, as a point of reference (hell, that might even be a FrameContainer feature, can't recall ;p).

[poire-z]

that might be the (your) inner_border I mentionned yesterday :)

[Commodore64user]

I believe the whole wrap business is done (I hope I didn't miss anything), could you look into that border thing @poire-z, I have no clue what's going on there... please. ;)

[poire-z]

I spent a few hours today working on adjusting the bookmapwidget from this PR, I fixed the border thing (you'll have to tell later if and how thiner we can go than 3*Size.border.thin).
Haven't yet looked at the PageBrowser widget.
So, good if you're done - don't add stuff to this PR so we don't conflict.

[Commodore64user]

understood.

[poire-z]

Been trying it to see how much I could help without drowning. Just some notes for now:

• there seems to be some random key mapping ordering: sometimes my up/down arrows work to navigate focus in bookmap, sometimes they don't (and even sometimes "down" works while "up" doesn't) and cause scrolling of the bookmap instead (their previous behaviour - how one would do that now ?)

• the FrameContainer tweak, using inner_border for focus, seems to be the right thing to do, and should be what should be used to fix Menu widget: draw focus on focused item #12583 (comment)
  We should probably not add any padding for that inner border, to not mess with the general geometry, and live with it if it overflows the inner content.

• there is some geometry issue - which just removing the padding added to toc_title does not solve - causing the appearance of a bottom scrollbar:
  

• What's the alternative for navigation on not hasDPad() ? What are these devices ?
  local enable_invisible_focus_page_slot = Device:hasDPad() makes it so that only DPad devices can navigate page slots. non-DPad devices can still navigate chapter boxes: how a user achieve that on such devices ?

[Commodore64user]

@poire-z right now I can only answer your last point

• What's the alternative for navigation on not hasDPad() ? What are these devices ?
  local enable_invisible_focus_page_slot = Device:hasDPad() makes it so that only DPad devices can navigate page slots. non-DPad devices can still navigate chapter boxes: how a user achieve that on such devices ?

mmm, using a touch screen? The thing to remember is that, this is not adding support for non-touch devices in general, it is adding it to non-touch devices that have D-Pads and can spare them to be used for other stuff besides core functionality (i.e useDPadAsActionKeys()), so in short, kindle.

[poire-z]

What I see in the changes to bookmapwidget.lua is that:
table.insert(focus_row, span_w) + table.insert(self.focus_layout, focus_row) (adding toc chapter to focus layout) is done unconditionally (but of course usable only on devices where the focusmanager is able to process some navigation keys)
while:
table.insert(self.focus_layout, invisible_focusable_page_slots) (adding page slots to focus layout) is done only when enable_invisible_focus_page_slot = Device:hasDPad().

My question is: are there devices/configurations where people will get the first but not the second ?

[poire-z]

I think it's best to just close this PR

Well, let's not do that yet.
Most of the interesting and hard work (that you and I don't really need to understand) has been done, and seems to work fine.
There are just a few other things like the ones I mentionned that may or may not be hard to fix, but I'd need more time to have the time to look at it.

[Commodore64user]

Dear @poire-z, we are all in tenterhooks waiting for your contribution, in the meantime, could you please rebase this (I don't want to do it so as not to mess up with your stuff) so I can easily re-apply these changes to a 2024.11-44 build?

[poire-z]

Well, I have no real idea how this could happen.
But it if happens, there are probably bad dimen somewhere, may be rounding errors (it happened in the past when we forgot to math.floor/ceil some resulting computations, so some x/y/w/h were passed as not-integers, and the framebuffer code was rounding them, so sometimes having the refresh area smaller that what was really painted.
Should be investigated in this module, if it happens also when we don't position/move the widget, check if Back really get to call this, may be try "ui" or "fast" instead of "flashui", ...:

koreader/frontend/ui/widget/buttondialog.lua

Lines 281 to 285 in 9dbcde4

	function ButtonDialog:onCloseWidget()
	UIManager:setDirty(nil, function()
	return "flashui", self.movable.dimen
	end)
	end

[Commodore64user]

may be try "ui" or "fast" instead of "flashui", ...:

neither makes a difference, I also don't think it calls that anyway, as the key event used is

koreader/frontend/ui/widget/buttondialog.lua

Lines 287 to 293 in 9dbcde4

	function ButtonDialog:onClose()
	if self.tap_close_callback then
	self.tap_close_callback()
	end
	UIManager:close(self)
	return true
	end

[poire-z]

UIManager:close(self) should get self's :onCloseWidget() called. You could add a logger.warn() to be sure.

[Commodore64user]

UIManager:close(self) should get self's :onCloseWidget() called. You could add a logger.warn() to be sure.

in any case, "ui" or "fast" don't change anything.

[NiLuJe]

also, when closing the Hold menu (only visible on e-ink) the bottom and right edges remain visible until a further key press

Closing how ? I don't see any edge when taping outside.

Did you have that issue before this PR or is it something new ? (haven't yet read your changes).

Back button, it is not replicable on the emulator. Also you get the bottom and right edge on k4 but only the bottom one on k3. I don't remember this happening before although, i had only tested on k3 up to now...

Check if the extra bits make it into a screenshot (that'll rule out a simpler paint snafu).

[Commodore64user]

Nope, it clears itself when one tries to capture a screenshot (to be more precise, it clears with any non-modifier key press), which is why i had to take a real photograph of the screen in #12579 (comment)

[NiLuJe]

Check the logs (and possibly pepper some good old printf around to help) to compare the widget's position and dimensions against the requested refresh region; there might be a rounding error somewhere (or a float that gets truncated somewhere).

On which device is that? (And what's the screen resolution?)

[Commodore64user]

On which device is that? (And what's the screen resolution?)

kindle 3/4 #12579 (comment) 600x800

[Commodore64user]

    self.movable.dimen = self.movable:getSize()
    logger.dbg("ButtonDialog dimensions:",
        self.movable.dimen.x,
        self.movable.dimen.y,
        self.movable.dimen.w,
        self.movable.dimen.h
    )

DEBUG ButtonDialog dimensions: 0 0 256 138 
...
DEBUG _refresh: Enqueued flashui update for region 104.5 155.5 256 138 dithering: false 
DEBUG triggering refresh {
  mode = "flashui",
  region = "256x138+104.5+155.5"
} --[[table: 0x4124f878]] 

why is x and y 0?

[poire-z]

Where are they 0 ?

flashui update for region 104.5 155.5 256 138 
region = "256x138+104.5+155.5"

means x=104.5 and y=155.5
And there, you have your not-rounded not-integer x/y and probably the reason for your refreshes issues (the fact that it happens on some devide and not others is probably because on some devices/screen sizes/dpi, the computations ends up integer or not :))
So, follow that rabbit !

[NiLuJe]

why is x and y 0?

Not familiar with the widget OTOH, but some widgets plain don't update their positions at all (pet peeve of mine).

Although it's more likely this falls in the other, predominant category, where dimensions are updated during painTo, which is why setDirty can take a closure: it ensures the dimensions are evaluated post paintTo (and as such, are actually up to date).

---

In any case, as @poire-z just said, you just have to find what's missing a floor call on positions ;)

[NiLuJe]

Given the .5, I'm assuming it's a division by two (or maybe four) for centering purposes somewhere ;).

[Commodore64user]

had a single bathroom eureka moment... sorted now

edit: for those of you, loo-connoisseurs out there #12951 (comment)

[Commodore64user]

thank you @NiLuJe for fixing the certificates (how long will those last?), working now on both k3 and k4. now only waiting on an awesome code review here... ;)

[Commodore64user]

polite but firm remainder to @Frenzie and @poire-z that this is ready ;)

[poire-z]

Impressive work!

[poire-z]

Aren't the comment and the check contradictory?

[Commodore64user]

this was added by comphillip for testing i believe, removing it as no longer needed

[poire-z]

No condition at all ?
In dispatcher just below, you have condition=Device:isTouchDevice() or (Device:hasDPad() and Device:useDPadAsActionKeys())},

[Commodore64user]

we don't need this stop anymore... or perhaps i don't understand your point.

[poire-z]

Every device will get this menu item ? (= "no condition at all")
What about non-touch but not have/use dpadasactionkeys (which is the condition elsewhere for enabling focus layout).

(My point is the same as all along: all these device caps checks need to be consistent.)

[Commodore64user]

this was only here when BookMap was available but not PageBrowser on kindle, nobody else enters this realm. we don't need this check anymore since kindle goes all in now. and noone else enters partially.

What about non-touch but not have/use dpadasactionkeys

such device would not be running this code because it would have been already stopped at the door (by the early return)

(My point is the same as all along: all these device caps checks need to be consistent.)

they are, not the exact same, but consistent.

[poire-z]

This condition expects Device:hasDPad() , more than the ones mentionned in a previous comment.
Is that ok ? (if hadDPad is always true when useDPadAsActionKeys, remove it, so all conditions look consistent.)

[Commodore64user]

mmm... this has been @Frenzie's preference to always use hasDPad and useDPad...

[Frenzie]

It is true that useDPadAsActionKeys implies hasDPad but presumably I preferred to have the overview.

[poire-z]

Different from useDPadAsActionKeys here, is that correct ?
(Same in PageBrowser.)

[Commodore64user]

it is correct. note that if a device is neither isTouchDevice nor useDPad..., it would not be running any of this code, let alone registering key events.

[poire-z]

You can remove my debugging logger.warn() (and their else branch if I added it just for that), there are 4 of them.

[poire-z]

I guess the commented stuff was done while I was experimenting, you can remove it.

[poire-z]

I guess the commented stuff was done while I was experimenting, you can remove it.

[poire-z]

May be add a comment above, something like:
-- Override FocusManager internal methods, so we can scroll the view instead of wrap around

[poire-z]

To be added to the squashed commit message:

Also:
FrameContainer: fix focus border handling, and draw inner border after the
content (to prevent it from being overridden by the content bgcolor).

[Frenzie]

I generally say it's important to avoid being smart in localization, but that's in reference to things like plurals. For reusing localization being smart is good.

Something like this:

elseif Device:hasKeyboard() then
    local lines = {
        _("The settings (in this menu) can be used to change the number of rows and columns, whether to display page numbers, and to display different chapter-levels in the bottom ribbon."),
        _("Press Shift+Up to move up by one row, or either previous-page-turn-button to move one screen."),
        _("Press Shift+Down to move down by one row, or either next-page-turn-button to move one screen."),
        _("Press Shift+Press on a thumbnail, to open more options."),
        _("Press Shift+Back closes all instances of Page Browser and Book Map."),
        _("Select a thumbnail to read that page.")
    }
    text = table.concat(lines, "\n\n")
elseif Device:hasScreenKB() then
    local lines = {
        _("The settings (in this menu) can be used to change the number of rows and columns, whether to display page numbers, and to display different chapter-levels in the bottom ribbon."),
        _("Press ScreenKB+Up to move up by one row, or either previous-page-turn-button to move one screen."),
        _("Press ScreenKB+Down to move down by one row, or either next-page-turn-button to move one screen."),
        _("Press ScreenKB+Press on a thumbnail, to open more options."),
        _("Press ScreenKB+Back closes all instances of Page Browser and Book Map."),
        _("Select a thumbnail to read that page.")
    }
    text = table.concat(lines, "\n\n")
end

[Frenzie]

@poire-z What precisely needs to be in the commit message? (Or alternatively, can you squash it?)

[Commodore64user]

@poire-z What precisely needs to be in the commit message? (Or alternatively, can you squash it?)

#12579 (comment)

And: Thus do we strive to make Kindle great once more. 😂

I would call it, "liberation commit"

[Commodore64user]

Well now that Liberation day has come and kindle has finally been made great again, i would like to take a moment to thank @comphilip and @poire-z for their efforts and relentless dedication to the cause. Many a poire died in the process but hey, they do grow on trees so, there is that ;)