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

Kindle4NT improvements #3745

Merged
merged 22 commits into from Mar 14, 2018

Conversation

Projects
None yet
4 participants
@onde2rock
Contributor

onde2rock commented Mar 12, 2018

I try to add some minimal support for better experience on my old kindle 4:

  • better keybindings
  • touchmenu now support key naviguation
  • change to the focus manager to support the more complex layout of the touchmenu

onde2rock added some commits Mar 12, 2018

modify focumanager to allow for more complex layout
The focusmanager now naviguate the layout by avoiding nil value
instead of relying on table lenght. It should be completely backward
compatible
fix crash because virtualkeyboard on non touch device
the kindle4NT has no keyboard nor touch, the fix open the virtual
keyboard so koreader dont crash but it's not useable
@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 12, 2018

Thanks! I don't have time to take a look tonight and possibly not tomorrow either, so pinging @poire-z

At a glance there are a few minor stylistic issues, some of which you'll have to fix to reach the unit tests at all, but it looks very promising! :-)

@@ -241,7 +240,7 @@ function ReaderMenu:onShowReaderMenu(tab_index)
}
local main_menu
if Device:isTouchDevice() then
if Device:hasKeys() then

This comment has been minimized.

@poire-z

poire-z Mar 13, 2018

Contributor

(Here and elsewhere)
There are devices which are TouchDevices and don't have keys, actually most :)
With this (I guess), when I touch top, i don't get my touch menu, but a new window "Document menu" I have never ever seen :) .
May be if Device:isTouchDevice() or Device:hasKeys() then

This comment has been minimized.

@onde2rock

onde2rock Mar 13, 2018

Contributor

yes of course, fixing it asap

@@ -362,7 +362,8 @@ function FileManager:init()
end
if Device:hasKeys() then
self.key_events.Close = { {"Home"}, doc = "Close file manager" }
self.key_events.Home = { {"Home"}, doc = "go home" }
self.file_chooser.key_events.Close = nil

This comment has been minimized.

@poire-z

poire-z Mar 13, 2018

Contributor

I'm so used to Backspace to quit things on the emulator, which work(ed) on quite everything. You removed them here and elsewhere.
No real way to keep that while staying ok with what you want?

This comment has been minimized.

@onde2rock

onde2rock Mar 13, 2018

Contributor

Ha, I had the feeling this would not be ok.
At first, I tried to add a command parameter to reader.lua, something like "reader.lua --no-key-escape", but there were no example, so I had no idea were the variable sould be stored.
I will try to think of something.

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

I'm sure @poire-z can also get used to Alt+F4 instead. :-P I think backspace or escape makes sense to quit dialogs, but I'm not so sure about the program itself.

But I am somewhat confused by some of these changes, like what is Prec? Could you describe what's happening right now and how these keybindings are better?

This comment has been minimized.

@onde2rock

onde2rock Mar 13, 2018

Contributor

Yes sorry, "Prec" is supposed to be "Back", it should go to upper menu or close the touchmenu.
I tried to look to make the keybindings Escape to exit but the escape key doesn't seem to be defined in event_map_sdl2.lua

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

You can add it, of course. ;-)

[41] = "Back", -- Escape

(Or something like that.)

function FocusManager:onWrapFirst()
self.selected.y = #self.layout
return true
function FocusManager:warpAround(dy)

This comment has been minimized.

@poire-z

poire-z Mar 13, 2018

Contributor

warpAround or wrapAround ? :)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 13, 2018

I'm not really familiar with the "keypad" devices ecosystem, the only one I know is the emulator :)
So, I just had a test on the emulator and on my kobo with this patch, and it breaks touch menu on touch-only devices, and break a few habits on the emulator.

@@ -788,4 +789,9 @@ function FileManager:moveFile(from, to)
return util.execute(self.mv_bin, from, to) == 0
end
function FileManager:onHome()
return self:goHome()

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

stray newline

@@ -306,7 +306,7 @@ function FileManagerMenu:onShowMenu()
}
local main_menu
if Device:isTouchDevice() then
if Device:isTouchDevice() or Device:hasKeys() then

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

When will we reach the else branch? What's the intent behind this change exactly? Is this saying the else branch and presumably therefore also Menu:itemTableFromTouchMenu() is deprecated?

This comment has been minimized.

@poire-z

poire-z Mar 13, 2018

Contributor

There is also a hasDPad(). I dunno if we can have device that have this, but not hasKeys() neither isTouchDevice().
May be we need some kind of reference about devices capabilities, and their available physical keys and to what they map to.

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

I see, but if hasKeys can go up/down/left/right then hasDPad should be able to do that even better. :-P

Then there's hasKeyboard but in practice that seems to be additive.

@@ -241,7 +240,7 @@ function ReaderMenu:onShowReaderMenu(tab_index)
}
local main_menu
if Device:isTouchDevice() then
if Device:isTouchDevice() or Device:hasKeys() then

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

See remark above about else branch.

@@ -53,7 +53,6 @@ function ReaderMenu:init()
self.registered_widgets = {}
if Device:hasKeys() then
self.key_events = { Close = { { "Back" }, doc = "close menu" }, }

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

How will you close the menu?

This comment has been minimized.

@onde2rock

onde2rock Mar 13, 2018

Contributor

The back key is directly handled by touchemenu.

@@ -622,4 +616,9 @@ function ReaderUI:dealWithLoadDocumentFailure()
error("crengine failed recognizing or parsing this file: unsupported or invalid document")
end
function ReaderUI:onHome()
print("go home2")

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

You could potentially use logger.dbg("some relevant message") btw.

@@ -27,6 +27,8 @@ local util = require("util")
local _ = require("gettext")
local Screen = require("device").screen
local template = require("ffi/util").template
local Device = require("device")

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

stray newline

@@ -27,6 +27,8 @@ local util = require("util")
local _ = require("gettext")
local Screen = require("device").screen
local template = require("ffi/util").template
local Device = require("device")

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

Adding Device here requires putting it alphbetically in order (on line 5).

That in turn means local Screen = require("device").screen should be changed to local Screen = Device.screen

if Device:hasKeys() then
self.key_events = {
AnyKeyPressed = { { Device.input.group.Any },
seqtext = "any key", doc = "close dialog" }

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

Maybe add a comment that this is (presumably) so you won't get locked in?

left/right. also, you can go up from both to reach <textinput>,
and from that go down and (depending on internat coordinates)
reach either <okbutton> or <cancelbutton>.
Naviguate the layout by trying to avoid not set or nil value.

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

Nitpick: navigate. :-P

@@ -88,6 +88,12 @@ if Device.isTouchDevice() then
end)
end
end

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

stray newlines

elseif not Device.hasKeyboard() then
Keyboard = require("ui/widget/virtualkeyboard")

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

indentation

@@ -26,6 +26,10 @@ local util = require("ffi/util")
local _ = require("gettext")
local Screen = Device.screen
local getMenuText = require("util").getMenuText
local FocusManager = require("ui/widget/focusmanager")

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

See https://github.com/koreader/koreader-base/wiki/Coding-style#order-of-requires

FocusManager, Event and UnderlineContainer should go up above.

Input can stay here, one line above Screen.

end
function TouchMenuItem:onUnfocus()
self._underline_container.color = Blitbuffer.COLOR_WHITE

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

indentation (& some stray newlines up above)

@@ -706,5 +748,12 @@ end
function TouchMenu:onClose()
self:closeMenu()
end
function TouchMenu:onPrec()

This comment has been minimized.

@Frenzie

Frenzie Mar 13, 2018

Member

lack of newline before

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 14, 2018

Oh, thanks for the back backspace ! :)

Hm, I'm not convinced. :-)

Well, make it, instead of if Device:isSDL() then:
if G_reader_settings:isTrue("backspace_closes_reader") then
so, even those on keypad devices, who liked this back to close for years, can have it back too.

I have checked with screenshots before/after. and the small-change-that-looks-like-a-no-change in UnderlineContainer actually makes the TouchMenu looks like it did before, which is good! :)
Actually, there's now a few pixels shift down of filenames in filebrowser in classic mode (checked with 14 and 20 items per page), but that makes it actually look better - more visually correctly centered, to me !
In coverbrowser list mode, the little cover image actually gains 1 pixel (strange, sometimes in width, sometimes in height, must depend on some rounding), but that's also better because they look now more centered between the separator lines (the title/author/filename hhasn't moved at all).
edit: and the underline under the a...A font chooser is fine too

Anyway, all that is good/better, so that's a good to merge from me.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 14, 2018

if G_reader_settings:isTrue("backspace_closes_reader") then
so, even those on keypad devices, who liked this back to close for years, can have it back too.

Makes more sense. Alternatively or also, similar to something like the menu override, one could implement a user event map override.

@onde2rock

This comment has been minimized.

Contributor

onde2rock commented Mar 14, 2018

if G_reader_settings:isTrue("backspace_closes_reader") then
so, even those on keypad devices, who liked this back to close for years, can have it back too.

Ok gonna try that.

similar to something like the menu override, one could implement a user event map override.

Sorry I don't understand, implementing an user event map override, like add a widget to remap keybindings of koreader ?

@@ -82,6 +83,15 @@ function BookStatusWidget:init()
show_parent = self,
readonly = self.readonly,
}
if Device:hasKeys() then
self.key_events = {

This comment has been minimized.

@Frenzie

Frenzie Mar 14, 2018

Member

Four more spaces in this line and the next two please.

@@ -124,4 +124,21 @@ function IconButton:onHoldIconButton()
return true
end

This comment has been minimized.

@Frenzie

Frenzie Mar 14, 2018

Member

Since I'm just nitpicking nothings a remaining stray newline.

end
function IconButton:onUnfocus()
self.image.invert=false

This comment has been minimized.

@Frenzie

Frenzie Mar 14, 2018

Member

6 spaces?

@@ -124,4 +124,21 @@ function IconButton:onHoldIconButton()
return true
end
function IconButton:onFocus()
--quick and dirty, need better way to show focus

This comment has been minimized.

@Frenzie

Frenzie Mar 14, 2018

Member

but messy here :-P

end
function IconButton:onTapSelect()
self:onTapIconButton()

This comment has been minimized.

@Frenzie

Frenzie Mar 14, 2018

Member

and here

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 14, 2018

Okay, let's leave the backspace stuff for the future. Just a couple of minor final nitpicks.

@@ -46,6 +47,13 @@ function ReaderProgress:init()
UIManager:setDirty(self, function()
return "ui", self.dimen
end)
if Device:hasKeys() then
self.key_events = {

This comment has been minimized.

@Frenzie

Frenzie Mar 14, 2018

Member

indentation is missing a space?

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 14, 2018

Sorry I don't understand, implementing an user event map override, like add a widget to remap keybindings of koreader ?

I'd probably prefer that in another PR even if you felt obliged to do that. :-)

What I mean is that after this:

event_map = require("device/sdl/event_map_sdl2"),

You could call out to something like this:

function MenuSorter:readMSSettings(config_prefix)
if config_prefix then
local menu_order = string.format(
"%s/%s_menu_order", DataStorage:getSettingsDir(), config_prefix)
if lfs.attributes(menu_order..".lua") then
return require(menu_order) or {}
end
end
return {}
end
function MenuSorter:mergeAndSort(config_prefix, item_table, order)
local user_order = self:readMSSettings(config_prefix)
if user_order then
for user_order_id,user_order_item in pairs(user_order) do
order[user_order_id] = user_order_item
end
end
return self:sort(item_table, order)
end

A GUI is complex stuff. A simple custom override file is not.

Fix indentation
Damnit, I gotta find something that do this automatically
@Frenzie

This comment has been minimized.

Member

Frenzie commented on frontend/ui/widget/bookstatuswidget.lua in 664a682 Mar 14, 2018

cough this line is double-indented now ;)

This comment has been minimized.

Contributor

onde2rock replied Mar 14, 2018

Hum, yeah because it's a table.
I did it like other example I found in the code.

This comment has been minimized.

Member

Frenzie replied Mar 14, 2018

Right, my bad.

@Frenzie Frenzie merged commit e8aab49 into koreader:master Mar 14, 2018

1 check was pending

ci/circleci CircleCI is running your tests
Details
@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 14, 2018

Thanks! Of course only just after merging did I notice that there are some slight side effects of sorts but that's probably more my fault than yours. Concretely, you can't reach the bottom part of a ButtonDialog with hackish visual separation like this:

screenshot_2018-03-14_22-20-44

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 20, 2018

Btw @poire-z this happened when I ran DISABLE_TOUCH=1 ./kodev run -s=kobo-aura-one (with CoverBrowser enabled in settings), should you be interested.

./luajit: plugins/coverbrowser.koplugin/covermenu.lua:96: attempt to call method 'onFocus' (a nil value)
stack traceback:
	plugins/coverbrowser.koplugin/covermenu.lua:96: in function 'updateItems'
	frontend/ui/widget/menu.lua:964: in function 'switchItemTable'
	frontend/ui/widget/filechooser.lua:271: in function 'refreshPath'
	frontend/ui/widget/filechooser.lua:280: in function 'changeToPath'
	plugins/coverbrowser.koplugin/main.lua:402: in function 'refreshFileManagerInstance'
	plugins/coverbrowser.koplugin/main.lua:492: in function 'action'
	frontend/ui/uimanager.lua:534: in function '_checkTasks'
	frontend/ui/uimanager.lua:715: in function 'handleInput'
	frontend/ui/uimanager.lua:807: in function 'run'
	./reader.lua:195: in main chunk
	[C]: at 0x5572f86390b0
~/src/kobo/koreader

Now browsing the menu is a bit of a chore… F1 to activate, End to open a submenu… :-P

@Frenzie Frenzie referenced this pull request Mar 20, 2018

Merged

Tapplus shortcut #3773

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 20, 2018

Just adding this if around seems to solve the crash, and navigation with keys, and showing the accurate underline, seem to work:

            if self.item_group[select_number].onFocus then
                self.item_group[select_number]:onFocus()
            end

It crashed with select_number being 1, may be it gets called to early...
It happened before these mods by @onde2rock , so I guess this simple fix is fine.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 20, 2018

It happened before these mods

That's no surprise. No one ever runs the program with DISABLE_TOUCH=1. Or at least I don't. :-P

Frenzie added a commit to Frenzie/koreader-base that referenced this pull request Mar 30, 2018

[feat] SDL2: preliminary gamepad support
This is a "dumb" implementation that spits out fake keyboard events.

* Left trigger & d-pad: arrow keys
* Bumpers and right trigger: page up/down
* Menu button: menu
* A: enter
* B: back

This is sufficient to use most of the program.

Made possible by @onde2rock's recent efforts in koreader/koreader#3796
koreader/koreader#3785 koreader/koreader#3774
koreader/koreader#3765 and koreader/koreader#3745

Frenzie added a commit to koreader/koreader-base that referenced this pull request Mar 31, 2018

[feat] SDL2: preliminary gamepad support (#628)
This is a "dumb" implementation that spits out fake keyboard events.

* Left stick & d-pad: arrow keys
* Bumpers and right stick: page up/down
* Menu button: menu
* A: enter
* B: back

This is sufficient to use most of the program.

Made possible by @onde2rock's recent efforts in koreader/koreader#3796
koreader/koreader#3785 koreader/koreader#3774
koreader/koreader#3765 and koreader/koreader#3745

Frenzie added a commit to Frenzie/koreader that referenced this pull request Mar 31, 2018

[feat, UX] bump base for SDL2: preliminary gamepad support
* [feat] SDL2: preliminary gamepad support koreader/koreader-base#628

This is a "dumb" implementation that spits out fake keyboard events.

* Left stick & d-pad: arrow keys
* Bumpers and right stick: page up/down
* Menu button: menu
* A: enter
* B: back

This is sufficient to use most of the program.

Made possible by @onde2rock's recent efforts in koreader#3796
koreader#3785 koreader#3774
koreader#3765 and koreader#3745

Frenzie added a commit that referenced this pull request Mar 31, 2018

[feat, UX] bump base for SDL2: preliminary gamepad support (#3819)
* [feat] SDL2: preliminary gamepad support koreader/koreader-base#628

This is a "dumb" implementation that spits out fake keyboard events.

* Left stick & d-pad: arrow keys
* Bumpers and right stick: page up/down
* Menu button: menu
* A: enter
* B: back

This is sufficient to use most of the program.

Made possible by @onde2rock's recent efforts in #3796
#3785 #3774
#3765 and #3745
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment