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

Support the virtualKeyboard on non touch-device #3796

Merged
merged 14 commits into from Mar 30, 2018

Conversation

Projects
None yet
3 participants
@onde2rock
Contributor

onde2rock commented Mar 23, 2018

Now when opening a widget with the virtualkeyboard, the focus is first on the keyboard, then press Back to close the keyboard, then the focus is on the widget itself. Press to reopen the keyboard.

My solution is to change the order of the widget. The last one (on top) will be the virtualkeybard so it catch all the keybinding, and below it, make the dialog is_always_active = true so it can receive touch event.

There is still some visual weirdness with the focus on the keyboard but it's not a problem.

[VirtualKeyboard] Add support for keynaviguation
Also rename the variable "layout" to "keyboard_layout" because conflict
with the layout from the focusmanager

@onde2rock onde2rock changed the title from Support the virtualKeyboard on non touch-device to [WIP] Support the virtualKeyboard on non touch-device Mar 23, 2018

onde2rock added some commits Mar 23, 2018

Make the goto dialog compatible with key naviguation
My solution is to change the order of the widget. The last one will the
virtualkeybard so it catch all the keybinding, and below it, make the
dialog "is_always_active = true" so it can receive touch event.

@onde2rock onde2rock force-pushed the onde2rock:virtualkeyboard_key_navigation branch from ad23e69 to 258ff94 Mar 24, 2018

UIManager:show(self.settings_dialog)
self.settings_dialog:onShowKeyboard()

This comment has been minimized.

@Frenzie

Frenzie Mar 24, 2018

Member

Instead of changing the order, have you tried making it a modal widget instead with modal = true,? If that works it'd be simpler and less error prone.

This comment has been minimized.

@Frenzie

Frenzie Mar 24, 2018

Member

We can leave the order changed either way. It does seem slightly illogical to show the keyboard first. ;-) In any case it seems like a good thing to test. :-)

This comment has been minimized.

@onde2rock

onde2rock Mar 24, 2018

Contributor

Oh that's awesome, exactly what is needed for the virtualKeyboard.

I already switched all the line, so I kinda want to keep it, I will look into it.

@onde2rock onde2rock force-pushed the onde2rock:virtualkeyboard_key_navigation branch 3 times, most recently from 8b24a70 to 745f69f Mar 24, 2018

@onde2rock onde2rock force-pushed the onde2rock:virtualkeyboard_key_navigation branch from 46de90e to c2c7613 Mar 25, 2018

@@ -197,7 +197,7 @@ function InputDialog:init()
}
}
--little hack to piggyback on the layout of the button_table to handle the new InputText
table.insert(self.button_table.layout, #self.button_table.layout, {self._input_widget})
table.insert(self.button_table.layout, 1, {self._input_widget})

This comment has been minimized.

@Frenzie

Frenzie Mar 25, 2018

Member

Could you explain what this little hack is doing exactly? Seeing a specific number like 1 is a lot more worrisome than #self.button_table.layout. ;-)

@Frenzie

This comment has been minimized.

Member

Frenzie commented on frontend/ui/widget/inputdialog.lua in 3302c25 Mar 25, 2018

Sorry, never mind. I was looking at two commits at once so it was less clear.

@onde2rock onde2rock changed the title from [WIP] Support the virtualKeyboard on non touch-device to Support the virtualKeyboard on non touch-device Mar 25, 2018

@onde2rock onde2rock force-pushed the onde2rock:virtualkeyboard_key_navigation branch from 9a34084 to 9442331 Mar 25, 2018

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 25, 2018

Had a quick try on the emulator, did not notice side effects.
Had a quick try on my kobo, and it crashes when I open Dict lookup or Rename bookmark:

./luajit: frontend/ui/widget/inputdialog.lua:200: bad argument #1 to 'insert' (table expected, got nil)
stack traceback:
        [C]: in function 'insert'
        frontend/ui/widget/inputdialog.lua:200: in function 'init'
        frontend/ui/widget/widget.lua:48: in function 'new'
        frontend/ui/widget/container/inputcontainer.lua:215: in function 'onInput'

You can remove/comment these to make the emulator behave a bit more like a pure touch device:

hasKeyboard = yes,
hasKeys = yes,
hasDPad = yes,

Fix for the various combination of
hasKeys,hasDpad,isTouchDevice

@onde2rock onde2rock force-pushed the onde2rock:virtualkeyboard_key_navigation branch from 85d4c46 to 4be9a73 Mar 25, 2018

@onde2rock

This comment has been minimized.

Contributor

onde2rock commented Mar 25, 2018

Ok, I tried quickly on all on the variation of device and it should all work.

The device with only keyboard are a litlle bit more broken than before, but no crash.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 25, 2018

Looks ok now: no side effects on my Kobo.

@onde2rock

This comment has been minimized.

Contributor

onde2rock commented Mar 26, 2018

Ok, unless further comment, I consider this PR done.

@@ -115,6 +118,10 @@ function FocusManager:_verticalStep(dy)
x = self.selected.x
while not self.layout[self.selected.y + dy][x] do
x = x + 1
if x>100 then
logger.dbg("[FocusManager] : Something went very wrong")

This comment has been minimized.

@Frenzie

Frenzie Mar 26, 2018

Member

Should be a warning, shouldn't it? :-)

This comment has been minimized.

@Frenzie

Frenzie Mar 26, 2018

Member

Also we might want to make it say "too many steps without result" or something a little bit more descriptive like that.

This comment has been minimized.

@onde2rock

onde2rock Mar 26, 2018

Contributor

Hum actually, it really should never happen in normal operation. Only when changing the layout, we can end up with a malformed layout.

The best would be to crash koreader here, because in that state, it will crash not long after.

Or should I add check before to be sure it doesn't happen and continue execution ?

This comment has been minimized.

@Frenzie

Frenzie Mar 26, 2018

Member

You can crash, but then please do it with dbg:guard or dbg.is_on.

This comment has been minimized.

@Frenzie

Frenzie Mar 26, 2018

Member

I forgot, there's also the simpler dbg.dassert().

Dbg.dassert = function(check, msg)
assert(check, msg)
return check
end

So you could write something like

if dbg.dassert(x>100) then -- will crash in debug
  logger.warn("onoez!")
  break
end

No need to stick to that suggestion. But there are at least three ways to tackle the problem using dbg. :-)

This comment has been minimized.

@onde2rock

onde2rock Mar 26, 2018

Contributor

So I fix the problem by checking it first. So now it should print an error message and not move the cursor.

@onde2rock onde2rock force-pushed the onde2rock:virtualkeyboard_key_navigation branch 3 times, most recently from 62ef082 to 79ca8c5 Mar 26, 2018

@Frenzie Frenzie merged commit e502bf0 into koreader:master Mar 30, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@onde2rock onde2rock deleted the onde2rock:virtualkeyboard_key_navigation branch Mar 30, 2018

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 30, 2018

@onde2rock
Btw, just for fun I decided to implement gamepad support. Thanks to your work it's quite usable, but I really think we should do some remapping on the frontend… I think it's an unintuitive nightmare, lol.

(I'm taking the lazy route of just sending keyboard events. It could always be changed later.)

Would you by any chance mind looking at it and proposing some changes?

Also, what exactly is the difference between press and enter? Basically I feel that you have the best handle on this. I have a BeBook 2, but it doesn't run KOReader. ;-)

@onde2rock

This comment has been minimized.

Contributor

onde2rock commented Mar 30, 2018

Yes Press and Enter, I supposed that Enter was legacy since it was not used a lot. I thinked about modifiyng all the keybindinds so they do the same thing, but that useless and it already work.

Actually, with 4 directions, PgUp, PgDown, Press and Back, almost all of Koreader should just work. Except the numberPickerWidget, I'm working on it.
Edit: and Menu and maybe Home

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 30, 2018

Alright, in that case I would propose to:

  1. Deprecate events like "Enter" (or rename "Press" to Enter?). I haven't looked into the specifics. Right now my controller code looks like this and I don't much care for it:
            if button == SDL.SDL_CONTROLLER_BUTTON_A then
                -- send enter
                genEmuEvent(ffi.C.EV_KEY, 40, 1)
                -- send end (bound to press)
                genEmuEvent(ffi.C.EV_KEY, 77, 1)
  1. Remap:
    • the Enter key to press.
    • the End key to end?
    • Esc (41) to "Back"
    • Backspace to backspace (for typing use)
  2. Add bindings:
    • 101 (context menu) for menu?

Anyway, it's mainly the simplifying/deprecating stuff that I'm worried about messing up for no-touch Kindle. Remapping the emulator bindings is otherwise inconsequential.

@onde2rock

This comment has been minimized.

Contributor

onde2rock commented Mar 30, 2018

Deprecate events like "Enter"

Souldn't break anything on my side, I'm already using press for everything

the Enter key to press.

on SDL? I'm already doing this on my git version, so much better.

Backspace to backspace (for typing use)

I don't think typing work anymore, I think I broke it, will look into it.
Edit: It was never a thing for device that use the virtualKeyboard.

101 (context menu) for menu?

Sorry I forgot, the Menu event is also needed, it's already in use to open the menu.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 30, 2018

on SDL? I'm already doing this on my git version, so much better.

Yes please.

I don't think typing work anymore, I think I broke it, will look into it.

I have no idea if that ever worked. My point is more that it shouldn't do weird things.

Sorry I forgot, the Menu event is also needed, it already open the menu.

I'll await the PR. :-)

@onde2rock

This comment has been minimized.

Contributor

onde2rock commented Mar 30, 2018

I'll await the PR. :-)

Hum, like this ?

if Device:hasKeys() then
self.key_events = {
ShowMenu = { { "Menu" }, doc = "show menu" },

I have no idea if that ever worked. My point is more that it shouldn't do weird things.

Backspace to go back isn't weird for me, it's like in chrome or firefox.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 30, 2018

Hum, like this ?

No, about things like merging Press and Enter into a single event if that is indeed a thing that makes sense, and using Enter instead of End.

I'd also either switch the menu trigger from F1 to context menu or add context menu as an alternative, but all that kind of stuff is much less relevant although I'd appreciate some help in that area. But just remapping won't potentially break anything on Kindle.

Backspace to go back isn't weird for me, it's like in chrome or firefox.

We shouldn't confuse the name of the event with what it actually does. In browsers with bad UX backspace still doesn't perform the behavior of Escape and Ctrl+W.

Backspace to close the keyboard is not even funny. That being said, it's not as bad as accidentally losing focus in a browser and pressing backspace so you lose what you were typing.

The direct equivalent to some browser's behavior would be previous page/location.

@onde2rock

This comment has been minimized.

Contributor

onde2rock commented Mar 30, 2018

Done in #3815

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