Skip to content
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

[UX] Gesture manager: add Exit and Restart action and a few gestures #4725

Merged
merged 5 commits into from Mar 4, 2019

Conversation

Projects
None yet
3 participants
@poire-z
Copy link
Contributor

poire-z commented Mar 4, 2019

Reference #4687 (comment)
Also:

  • add and show some separators in the gestures list
  • fix gesture removal, and also remove it from settings
  • add missing east west east
  • add 6 remaining of the 8 knob 3/4 rotations
  • add 3 easy knob full rotation (down + east + west)

These 3/4 or full knob rotations are really practical for stuff you don't want to do by error, but still easy to do when needed. Some may appeal more to left-handed persons.

image

image
(I let the "Zoom to fit" ones consecutive)

I did not let in these ones in yellow, because the drawing is really ambiguous:
image

@Frenzie: removing a gesture updated the current menu, but when going up and back, we were still seeing it. Also, it was not removed from the settings, so it was still active!

I think it needs another check somewhere to remove from settings any gesture that is referenced in none of the default or custom list (and to clean up the one recorded and played with and removed before this fix).
Dunno where you'd want to have that in your code.

Also, another thing: would be nice if the gestures settings would not include the gestures mapped to "nothing". We would then have something in settings.reader.lua a little shorted than:

    ["gesture_reader"] = {
        ["multiswipe_north_south_north"] = "prev_chapter",
        ["multiswipe_east_north_west"] = "zoom_contentwidth",
        ["multiswipe_southeast_northeast"] = "follow_nearest_link",
        ["multiswipe_east_north"] = "history",
        ["multiswipe_east_west_east"] = "nothing",
        ["multiswipe_south_west_north_east"] = "nothing",
        ["short_diagonal_swipe"] = "full_refresh",
        ["multiswipe_west_east"] = "previous_location",
        ["multiswipe_south_west"] = "show_frontlight_dialog",
        ["tap_right_bottom_corner"] = "nothing",
        ["multiswipe_east_south"] = "go_to",
        ["multiswipe_north_east"] = "toc",
        ["multiswipe_north_west"] = "bookmarks",
        ["multiswipe_southwest_northwest"] = "nothing",
        ["multiswipe_north_south"] = "nothing",
        ["multiswipe_west_north_east"] = "nothing",
        ["multiswipe_east_west"] = "latest_bookmark",
        ["multiswipe_west_south"] = "back",
        ["multiswipe"] = "nothing",
        ["multiswipe_northeast_southeast"] = "nothing",
        ["multiswipe_east_south_west"] = "nothing",
        ["multiswipe_north_west_south"] = "nothing",
        ["multiswipe_south_north"] = "skim",
        ["multiswipe_north_east_south"] = "nothing",
        ["multiswipe_east_north_west_east"] = "zoom_pagewidth",
        ["multiswipe_northwest_southwest"] = "nothing",
        ["multiswipe_south_east_north"] = "zoom_contentheight",
        ["multiswipe_south_north_south"] = "next_chapter",
        ["tap_left_bottom_corner"] = "toggle_frontlight",
        ["multiswipe_west_north"] = "nothing",
        ["multiswipe_south_west_north"] = "nothing",
        ["multiswipe_south_east_north_south"] = "zoom_pageheight",
        ["multiswipe_west_south_east_north"] = "nothing",
        ["multiswipe_west_south_east"] = "nothing",
        ["multiswipe_east_south_west_north"] = "full_refresh",
        ["multiswipe_south_east"] = "toggle_reflow",
        ["multiswipe_south_east_north_west"] = "nothing",
        ["multiswipe_west_east_west"] = "open_previous_document"
    },

poire-z added some commits Mar 4, 2019

[UX] Gesture manager: add action - Exit and Restart
Also:
- add and show some separators in the gestures list
- fix gesture removal, and also remove it from settings
- add missing east west east
- add 6 remaining of the 8 knob 3/4 rotations
- add 3 easy knob full rotation (down + east + west)
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 4, 2019

Also, another thing: would be nice if the gestures settings would not include the gestures mapped to "nothing". We would then have something in settings.reader.lua a little shorted than:

You could see if you can rework #4623, although I thought of it as potentially advantageous for manual config editing. :-)

if i < #multiswipes and multiswipes[i+1] == true then
separator = true
end
if type(multiswipes[i]) == "string" then -- skip separators (true)

This comment has been minimized.

@Frenzie

Frenzie Mar 4, 2019

Member

Or just if type(multiswipes[i]) ~= "string" then return end?

This comment has been minimized.

@poire-z

poire-z Mar 4, 2019

Author Contributor

I don't want to return :) But I would have used continue instead, if Lua had provided that useful construct, instead of needing imbricated if end...
I just want to skip the true entry, as it was carried to the previous item.

This comment has been minimized.

@Frenzie

Frenzie Mar 4, 2019

Member

Oh, you can actually do that (sort of).

if type(multiswipes[i]) ~= "string" then goto continue end
-- the whole shebang
::continue::

The bad part of this solution is that you need to keep coming up with new names for the darn things if you have even a little bit of nesting.

This comment has been minimized.

@poire-z

poire-z Mar 4, 2019

Author Contributor

Well, not fond of such constructs :)
(to review the small changes induced by this added if indentation, you can always use Diff settings > Hide whitespace changes)

This comment has been minimized.

@Frenzie

Frenzie Mar 4, 2019

Member

I didn't know about that, thanks for the tip. I wasn't talking about the diff though, although it is indeed easier to focus on the code that way. :-) Like you said, I think it'd be cleaner with a continue like in, C, PHP, Python, Java, etc.

(Anyway, it's good.)

@KenMaltby

This comment has been minimized.

Copy link

KenMaltby commented Mar 4, 2019

Not quite Witcher Signs, but getting there.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 4, 2019

I think it needs another check somewhere to remove from settings any gesture that is referenced in none of the default or custom list (and to clean up the one recorded and played with and removed before this fix).
Dunno where you'd want to have that in your code.

Well, instead of having them auto-removed, I just added them to the menu, so allowing to still see, re-assign or remove them.
Which lead me to think we would then not even need the custom multiswipes.lua: just store the recorded gestures into the settings, with "nothing" until they are assigned.

Also, another thing: would be nice if the gestures settings would not include the gestures mapped to "nothing".

I guess you need them to be there with "nothing" so you can bring back new ones with their default.
Well, no real idea for now. So let that be.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 4, 2019

Which lead me to think we would then not even need the custom multiswipes.lua: just store the recorded gestures into the settings, with "nothing" until they are assigned.

There are of course no technical reasons for it. But LuaData does happen to be crash-proof and as a user I like having separate config files for device-independent things like gestures. That is, I can copy them without concern, unlike my DPI/font-size/etc. device-specific settings (i.e., settings.lua).

I'd rather have the action assignments in there too, tbh. :-)

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 4, 2019

having separate config files for device-independent things like gestures

Makes sense indeed.

Well, I'm then done with this PR :)

@Frenzie Frenzie merged commit c510a5b into koreader:master Mar 4, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie added this to the 2019.03 milestone Mar 4, 2019

@Frenzie Frenzie added the UX label Mar 4, 2019

@poire-z poire-z deleted the poire-z:gesturemanager branch Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.