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

Make the reader bottom menu compatible with keys navigation #3785

Merged
merged 12 commits into from Mar 22, 2018

Conversation

Projects
None yet
3 participants
@onde2rock
Contributor

onde2rock commented Mar 21, 2018

What work :

  • Every toggle switch
  • The fine font toggle switch have been replaced by two text for non touch-devices
  • The Press key open the menu in addition to AA (alt-gr on sdl)

What don't work :

  • Contrast is not useable : wIll not fix
  • No way to get out of page crop. It's not responding to any keypress. So it's disabled on non touch-devices

2018-03-22-190704_1000x1500_scrot
2018-03-22-190644_1000x1500_scrot

onde2rock added some commits Mar 17, 2018

[toggleswitch] Add support for key naviguation to this widget
Add the onFocus an onUnfocus event handler
add a new function that just circle the switch if not touch event is
detected
Add key naviguation to the readermenu
Every toggleSwitch work except fine font tuning which have been
implemented using toggleswitch which was probably a nice little trick in
83eb90c

Contrast is not useable

Page crop cause to get locked in

The shortcut is still Alt-gr on sdl, to be defined on Kindle

@onde2rock onde2rock changed the title from Make the reader bottom menu compatible with keys naviguation to [WIP] Make the reader bottom menu compatible with keys naviguation Mar 21, 2018

@@ -61,11 +62,19 @@ function OptionTextItem:init()
}
else
self.active_key_events = {
Select = { {"Press"}, doc = "chose selected item" },
--Select = { {"Press"}, doc = "chose selected item" },

This comment has been minimized.

@Frenzie

Frenzie Mar 21, 2018

Member

Comment or remove? For now if you're changing the line you might as well fix the typo from chose to choose in the process ;-)

This comment has been minimized.

@onde2rock

onde2rock Mar 21, 2018

Contributor

Hum, I'm still in the process of deciding, because I think this whole process of setting active_key_events when onFocus is not used anymore and does nothing.

onde2rock added some commits Mar 21, 2018

Remove the old method of handling the Press key.
Now the event is handled by the main widget who implement focusmanager
and then dispatched to the currently focused item.
Potential fix for the fine font control
Replace the old switch menu with two icons plus and minus

@onde2rock onde2rock force-pushed the onde2rock:key-bottom-menu branch from d607a44 to 133553d Mar 21, 2018

@onde2rock onde2rock changed the title from [WIP] Make the reader bottom menu compatible with keys naviguation to [WIP] Make the reader bottom menu compatible with keys navigation Mar 21, 2018

@onde2rock onde2rock changed the title from [WIP] Make the reader bottom menu compatible with keys navigation to Make the reader bottom menu compatible with keys navigation Mar 21, 2018

@@ -33,6 +33,7 @@ local KoptOptions = {
alternate = false,
values = {0, 1, 2},
default_value = DKOPTREADER_CONFIG_TRIM_PAGE,
enabled_func = function() return require("device"):isTouchDevice() end,

This comment has been minimized.

@Frenzie

Frenzie Mar 22, 2018

Member

It's probably better just to include Device at the top (and change Screen to Device.screen)

Then here you should be able to do enabled_func = Device.isTouchDevice,

@Frenzie

This comment has been minimized.

Member

Frenzie commented on frontend/ui/data/koptoptions.lua in c3e156c Mar 22, 2018

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 22, 2018

I'd miss the config toggle, just because it made config panels similar.
Anyway, these images would need resizing (function of screen dpi?). On my small emulator:
image

Can't really test from my work PC, as I have no real Home or End key :|
But I can with arrows navigate the different a a a A A A (can't select because of no End key). Wouldn't that be enough for Kindle NT? Or do you really need Fine tuning? Or is is just that you are locked in with it?

I also see some french here and there :) for key, valeur in pairs(option_items_group) do.
(we have many simple for k, v in pairs elsewhere)

@onde2rock

This comment has been minimized.

Contributor

onde2rock commented Mar 22, 2018

I'd miss the config toggle

Yes, me too, it look really good.

Or do you really need Fine tuning? Or is is just that you are locked in with it?

Ideally, i'd like fine tuning. Currently it crash if I try to click on the toggleswitch, since it can't toggle, since it's not used as a toggleswich.

The best way would be to implement a new widget that look like the old toggleswitch but behave like two buttons. Lot of code for just one control in one menu.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 22, 2018

Currently it crash if I try to click on the toggleswitch, since it can't toggle, since it's not used as a toggleswich.

I guess you have looked at it, but just in case: can't you just make it not crash, possibly with some added if not Device:isTouchDevice() around your tricks ?

Otherwise, if we can't fix the current toggleswitch, as I see these simple changes:

-                toggle = {S.DECREASE, S.INCREASE},
+                item_icons = { ... }

If that's enough to change the thing type, can't you do:

-                toggle = Device:isTouchDevice() and {S.DECREASE, S.INCREASE} or nil,
+                item_icons = not Device:isTouchDevice() and { ... } or nil,

so we lucky touchdevicers can still see the old toggles, and only your kindleNT will get these -/+ ? :)

@onde2rock

This comment has been minimized.

Contributor

onde2rock commented Mar 22, 2018

so we lucky touchdevicers can still see the old toggles, and only your kindleNT will get these -/+ ? :)

Nice, good idea. Should be pretty simple to implement.

@@ -33,7 +34,7 @@ local KoptOptions = {
alternate = false,
values = {0, 1, 2},
default_value = DKOPTREADER_CONFIG_TRIM_PAGE,
enabled_func = function() return require("device"):isTouchDevice() end,
enabled_func = function() return Device:isTouchDevice() end,

This comment has been minimized.

@Frenzie

Frenzie Mar 22, 2018

Member

Is there a problem enabled_func = Device.isTouchDevice,?

This comment has been minimized.

@onde2rock

onde2rock Mar 22, 2018

Contributor

I did it like the widget above

                name = "screen_mode",
                name_text = S.SCREEN_MODE,
                toggle = {S.PORTRAIT, S.LANDSCAPE},
                alternate = false,
                args = {"portrait", "landscape"},
                default_arg = "portrait",
                current_func = function() return Screen:getScreenMode() end,
                event = "SetScreenMode",
            }

Plus it doesn't work, fail with

./luajit: error loading module 'ui/data/koptoptions' from file 'frontend/ui/data/koptoptions.lua':
        frontend/ui/data/koptoptions.lua:37: function arguments expected near ','

This comment has been minimized.

@Frenzie

Frenzie Mar 22, 2018

Member

Could you paste the entirety of what you're trying that fails?

onde2rock added a commit to onde2rock/koreader that referenced this pull request Mar 22, 2018

@onde2rock onde2rock force-pushed the onde2rock:key-bottom-menu branch from f8037e5 to c5f67c0 Mar 22, 2018

onde2rock added a commit to onde2rock/koreader that referenced this pull request Mar 22, 2018

@@ -34,7 +34,7 @@ local KoptOptions = {
alternate = false,
values = {0, 1, 2},
default_value = DKOPTREADER_CONFIG_TRIM_PAGE,
enabled_func = function() return Device:isTouchDevice() end,
enabled_func = Device:isTouchDevice,

This comment has been minimized.

@Frenzie

Frenzie Mar 22, 2018

Member

Module:functionName() is short for Module.functionName(self). Module.functionName is a reference to a function, which when possible is preferable to an anonymous function. Module:functionName is not a thing. ;-)

This comment has been minimized.

@Frenzie

Frenzie Mar 22, 2018

Member

PS I just meant put it in the reply, not push it.

This comment has been minimized.

@Frenzie

Frenzie Mar 22, 2018

Member

PPS Could you also change the above to current_func = Screen.getScreenMode,?

This comment has been minimized.

@onde2rock

onde2rock Mar 22, 2018

Contributor

Yes sure will do, once Git start to cooperate ...

@onde2rock onde2rock force-pushed the onde2rock:key-bottom-menu branch from c5f67c0 to c8af9e5 Mar 22, 2018

onde2rock added some commits Mar 22, 2018

@onde2rock

This comment has been minimized.

Contributor

onde2rock commented Mar 22, 2018

Hum, look like it's now broken, investigating.

edit : look like #3785 (comment) is not as simple ...
It fail with :

03/22/18-18:57:17 DEBUG CreDocument: set header font Noto Sans
03/22/18-18:57:17 DEBUG CreDocument: set font size 39
03/22/18-18:57:17 DEBUG CreDocument: toggle font bolder 0
03/22/18-18:57:17 DEBUG CreDocument: set font hinting mode 1
03/22/18-18:57:17 DEBUG CreDocument: set interline space 100
03/22/18-18:57:17 DEBUG CreDocument: set gamma index 15
03/22/18-18:57:17 DEBUG CreDocument: set hyphenation dictionary English_US_hyphen_(Alan).pdb
03/22/18-18:57:17 DEBUG CreDocument: goto page 1
03/22/18-18:57:17 DEBUG CreDocument: goto xpointer /body/DocFragment[1]/body/div.0
03/22/18-18:57:17 DEBUG CreDocument: set bookmarks highlight and internal history false
03/22/18-18:57:17 DEBUG show widget ReaderUI
03/22/18-18:57:17 DEBUG painting widget: ReaderUI
03/22/18-18:57:17 DEBUG refresh on physical rectangle -1 -1 1002 1502
03/22/18-18:57:17 DEBUG key event => type: 1, code: 77(Press), value: 0, time: 1521741437.739649
03/22/18-18:57:17 DEBUG AutoSuspend: onInputEvent
03/22/18-18:57:18 DEBUG key event => type: 1, code: 230(AA), value: 1, time: 1521741438.791093
03/22/18-18:57:18 DEBUG AutoSuspend: onInputEvent
03/22/18-18:57:18 DEBUG ImageWidget: _render'ing resources/icons/appbar.transform.rotate.right.large.png 58 58
03/22/18-18:57:18 DEBUG ImageWidget: initial offsets 0 0
03/22/18-18:57:18 DEBUG ImageWidget: _render'ing resources/icons/appbar.column.two.large.png 58 58
03/22/18-18:57:18 DEBUG ImageWidget: initial offsets 0 0
03/22/18-18:57:18 DEBUG ImageWidget: _render'ing resources/icons/appbar.text.size.large.png 58 58
03/22/18-18:57:18 DEBUG ImageWidget: initial offsets 0 0
03/22/18-18:57:18 DEBUG ImageWidget: _render'ing resources/icons/appbar.grade.b.large.png 58 58
03/22/18-18:57:18 DEBUG ImageWidget: initial offsets 0 0
03/22/18-18:57:18 DEBUG ImageWidget: _render'ing resources/icons/appbar.settings.large.png 58 58
03/22/18-18:57:18 DEBUG ImageWidget: initial offsets 0 0
./luajit: ./ffi/framebuffer.lua:275: attempt to index local 'self' (a nil value)
stack traceback:
        ./ffi/framebuffer.lua:275: in function 'current_func'
        frontend/ui/widget/configdialog.lua:294: in function 'init'
        frontend/ui/widget/widget.lua:48: in function 'new'
        frontend/ui/widget/configdialog.lua:504: in function 'init'
        frontend/ui/widget/widget.lua:48: in function 'new'
        frontend/ui/widget/configdialog.lua:699: in function 'update'
        frontend/ui/widget/configdialog.lua:656: in function 'init'
        frontend/ui/widget/widget.lua:48: in function 'new'
        frontend/apps/reader/modules/readerconfig.lua:65: in function 'handleEvent'
        frontend/ui/widget/container/widgetcontainer.lua:87: in function 'propagateEvent'
        frontend/ui/widget/container/widgetcontainer.lua:105: in function 'handleEvent'
        frontend/ui/uimanager.lua:455: in function 'sendEvent'
        frontend/ui/uimanager.lua:46: in function '__default__'
        frontend/ui/uimanager.lua:699: in function 'handleInputEvent'
        frontend/ui/uimanager.lua:760: in function 'handleInput'
        frontend/ui/uimanager.lua:804: in function 'run'
        ./reader.lua:195: in main chunk
        [C]: at 0x5578b0320050

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 22, 2018

The revert is fine: we cant't do as thought: Screen.getScreenMode, as it's not a simple dumb function (like the isTouchDevice and co are). It's a method of the Screen object, so Screen:getScreenMode() is actually exactly doing Screen.getScreenMode(self) (so, the self being nil error) , and you have to keep:
function() return Screen:getScreenMode() end,

@onde2rock

This comment has been minimized.

Contributor

onde2rock commented Mar 22, 2018

Ok, so unless further comments, I consider this PR done.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Mar 22, 2018

Looking and working fine for me on the emulator and my kobo.

@Frenzie Frenzie merged commit dfd8744 into koreader:master Mar 22, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@Frenzie

This comment has been minimized.

Member

Frenzie commented Mar 22, 2018

Didn't test but I trust @poire-z and it looks good to go. Thanks!

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