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

[feat] Gesture manager multiswipes #4607

Merged
merged 1 commit into from Feb 17, 2019

Conversation

Projects
None yet
3 participants
@Frenzie
Copy link
Member

Frenzie commented Feb 17, 2019

Follow-up to #4606. This one's a little more hackish and unfinished, but it's got a basic demo configuration interface that works. My point in already opening this PR is to provide context for the other one.

The main problem here lies in the configuration aspect. For the first 16 I could add them manually, and I could procedurally generate a whole lot more, but that doesn't seem terribly user friendly. My thought is to include a set of defaults (e.g., I was thinking something like down left for back to previous location), but at the same time a user should also be able to add a gesture like up down up down or up right down left.

Unimplemented: the first time the user makes a multiswipe gesture they'll be presented with a popup asking if they want to enable multiswipes.

screenshot_2019-02-17_10-15-06
screenshot_2019-02-17_10-15-12

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 17, 2019

Looks great!
The only place i ever needed such gestures is in the web browser on Android. Got some extension for old android firefox that allowed that. And from a one handed perspective, the most useful would include south and west/east: south_west, south_east, east_south, west_south. So, might be worth adding.

Regarding your UI, I'd probably move the Multiswipe> just below Enable multiswipe, in the same separator'ed section, so they are not split and spread to the start and end.

@Frenzie Frenzie changed the title [WIP] Gesturemanager multigestures [WIP] GestureManager multiswipes Feb 17, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 17, 2019

Here's an arrow sample:
screenshot_2019-02-17_12-57-59

@Frenzie Frenzie changed the title [WIP] GestureManager multiswipes [WIP] Gesture manager multiswipes Feb 17, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 17, 2019

screenshot_2019-02-17_15-14-23

@Frenzie Frenzie requested a review from robert00s Feb 17, 2019

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 17, 2019

@poire-z It's basically finished now.

@Frenzie Frenzie changed the title [WIP] Gesture manager multiswipes [feat] Gesture manager multiswipes Feb 17, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 17, 2019

lgtm, but it still includes the same gesturedetector.lua stuff as in #4606.

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Feb 17, 2019

Haven't read through everything yet, would that account for diagonal swipes, too? (f.g., a V shaped gesture).

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 17, 2019

@NiLuJe I actually probably spent most of my effort in GestureDetector earlier this morning on disabling diagonal swipes because I found them too error-prone. You can easily re-enable them like so:

diff --git a/frontend/device/gesturedetector.lua b/frontend/device/gesturedetector.lua
index b70d28c0..41220b57 100644
--- a/frontend/device/gesturedetector.lua
+++ b/frontend/device/gesturedetector.lua
@@ -522,11 +522,12 @@ function GestureDetector:handlePan(tev)
                 }
             end
 
-            local msd_direction, msd_distance = self:getPath(slot, true, fake_first_tev)
+            local msd_direction, msd_distance = self:getPath(slot, false, fake_first_tev)
 
             if msd_distance > self.MULTISWIPE_THRESHOLD
-               and (msd_cnt == 0
-                    or not pan_direction:match(msd_direction_prev))
+               --and (msd_cnt == 0
+               --     or not pan_direction:match(msd_direction_prev))
             then
                 if msd_direction ~= msd_direction_prev then
                     self.multiswipe_directions[msd_cnt+1] = {

Admittedly in using the emulator for primary testing I'm (intentionally) using more or less the worst case scenario (diagonals on a mouse…), so on a real touch device you might be less inclined to get results like this:

# here we want northwest southwest
02/17/19-19:38:23 DEBUG multiswipe north northwest north south
[…]
# and here we're going for southeast northeast
02/17/19-19:39:13 DEBUG multiswipe south southeast south northeast north northeast north

The checks are less necessary for regular up/down/left/right, but it still makes it a lot harder to do larger chained gestures.

So I'd propose to merge the GestureDetector PR as is and to consider diagonal stuff as a future update.

The main thing missing in this PR here is a few defaults. Those can be added in a nearby follow-up PR or this whole thing might have to wait for next weekend. ;-)

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Feb 17, 2019

@Frenzie : Good to know, thanks ;).

@Frenzie Frenzie force-pushed the Frenzie:gesturemanager-multigestures branch from eb24c58 to 42a06c0 Feb 17, 2019

[feat] ReaderGesture: add multiswipe support
This basic initial implementation offers an introductory message
on the first multiswipe with the option to disable, as well as
a few example multiswipes.

Custom multiswipes can be added to `settings/multiswipes.lua`.

@Frenzie Frenzie force-pushed the Frenzie:gesturemanager-multigestures branch from 42a06c0 to 675d2f5 Feb 17, 2019

@Frenzie Frenzie merged commit 6359272 into koreader:master Feb 17, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:gesturemanager-multigestures branch Feb 17, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 18, 2019

Some early feedback after a quick test:

  • horizontal+vertical and vertical+horizontal are hard to trigger: it seems they work better the smaller the first swipe is (ie: short south then long east).
  • up/down & down/up work quite well no matter both swipes' distances
  • I set one to Screen rotation (just out of curiosity, i never use that), and these multiswipe were quite screwed up after that :)

UI wise: the menu should probably show what function the gesture is assigned to, for quick reference, and to see which ones are still available (+ possible minor inconsistency: nothing is unchecked when the thing has not yet been assigned to something else)

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 18, 2019

horizontal+vertical and vertical+horizontal are hard to trigger: it seems they work better the smaller the first swipe is (ie: short south then long east).

You could try reducing the threshold. The basic problem is probably that you're triggering a pan instead.

I set one to Screen rotation (just out of curiosity, i never use that), and these multiswipe were quite screwed up after that :)

Ah, well, obviously neither do I. :-D Just very seldom, not never. But since this electronic PDF magazine I used to read disappeared I don't think I've used it.

  • possible minor inconsistency: nothing is unchecked when the thing has not yet been assigned to something else

Yes, they still need defaults (including auto-generated nothing).

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 18, 2019

@poire-z

I wrote a fix for other rotations. I'll just need to expand it to all directions.

diff --git a/frontend/device/gesturedetector.lua b/frontend/device/gesturedetector.lua
index b699ba71..78e89aad 100644
--- a/frontend/device/gesturedetector.lua
+++ b/frontend/device/gesturedetector.lua
@@ -662,6 +662,28 @@ function GestureDetector:holdState(tev, hold)
     end
 end
 
+local ges_coordinate_translation_270 = {
+    north = "east",
+    south = "west",
+    east = "south",
+    west = "north",
+    northeast = "southeast",
+    northwest = "northeast",
+    southeast = "southwest",
+    southwest = "northwest",
+}
+local function translateGesDirCoordinate(direction, translation_table)
+    if translation_table[direction] then
+        return translation_table[direction]
+    end
+end
+local function translateMultiswipeGesDirCoordinate(multiswipe_directions, translation_table)
+    for orig, trans in pairs(translation_table) do
+        multiswipe_directions = multiswipe_directions:gsub(orig, trans)
+    end
+    return multiswipe_directions
+end
+
 --[[--
   Changes gesture's `x` and `y` coordinates according to screen view mode.
 
@@ -675,24 +697,13 @@ function GestureDetector:adjustGesCoordinate(ges)
             ges.pos.x, ges.pos.y = (self.screen:getWidth() - ges.pos.y), (ges.pos.x)
         end
         if ges.ges == "swipe" or ges.ges == "pan"
+            or ges.ges == "multiswipe"
             or ges.ges == "two_finger_swipe"
-            or ges.ges == "two_finger_pan" then
-            if ges.direction == "north" then
-                ges.direction = "east"
-            elseif ges.direction == "south" then
-                ges.direction = "west"
-            elseif ges.direction == "east" then
-                ges.direction = "south"
-            elseif ges.direction == "west" then
-                ges.direction = "north"
-            elseif ges.direction == "northeast" then
-                ges.direction = "southeast"
-            elseif ges.direction == "northwest" then
-                ges.direction = "northeast"
-            elseif ges.direction == "southeast" then
-                ges.direction = "southwest"
-            elseif ges.direction == "southwest" then
-                ges.direction = "northwest"
+            or ges.ges == "two_finger_pan"
+        then
+            ges.direction = translateGesDirCoordinate(ges.direction, ges_coordinate_translation_270)
+            if ges.ges == "multiswipe" then
+                ges.multiswipe_directions = translateMultiswipeGesDirCoordinate(ges.multiswipe_directions, ges_coordinate_translation_270)
             end
             if ges.relative then
                 ges.relative.x, ges.relative.y = -ges.relative.y, ges.relative.x

Frenzie added a commit to Frenzie/koreader that referenced this pull request Feb 18, 2019

[fix] GestureDetector: multiswipe rotation
Reported by @poire-z in <koreader#4607 (comment)>.

Also refactor slightly along the way to make the translations more straightforward.

Frenzie added a commit that referenced this pull request Feb 18, 2019

[fix] GestureDetector: multiswipe rotation (#4613)
Reported by @poire-z in <#4607 (comment)>.

Also refactor slightly along the way to make the translations more straightforward.
@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Feb 18, 2019

./luajit: frontend/device/gesturedetector.lua:175: attempt to index a nil value
stack traceback:
        frontend/device/gesturedetector.lua:175: in function 'getPath'
        frontend/device/gesturedetector.lua:525: in function <frontend/device/gesturedetector.lua:479>
        frontend/device/gesturedetector.lua:110: in function 'feedEvent'
        frontend/device/input.lua:576: in function 'waitEvent'
        frontend/ui/uimanager.lua:1018: in function 'handleInput'
        frontend/ui/uimanager.lua:1066: in function 'run'
        ./reader.lua:210: in main chunk
        [C]: at 0x000139c1

~

(Quickly swipe-panning in a full-screen ImageViewer) ;).

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 18, 2019

horizontal+vertical and vertical+horizontal are hard to trigger: it seems they work better the smaller the first swipe is (ie: short south then long east).

You could try reducing the threshold. The basic problem is probably that you're triggering a pan instead.

I don't think it's threshold related. It looks like the gesture direction is guessed (by original handlePan code) always from the start position. So, if you do a big first south swipe, and then a medium or as big east swipe, you always get a "southeast" swipe (which is ignored by multiswipe code), and never a real straight "east".
Something like that makes my first swipe south followed by a swipe east work, but then comes another problem... if the swipe east is too short, it does not work :)

--- a/frontend/device/gesturedetector.lua
+++ b/frontend/device/gesturedetector.lua
@@ -505,10 +505,21 @@ function GestureDetector:handlePan(tev)

         local msd_cnt = #self.multiswipe_directions
         local msd_direction_prev = (msd_cnt > 0) and self.multiswipe_directions[msd_cnt][1] or ""
+        logger.warn(pan_direction, pan_distance, self.multiswipe_directions)
+
+        if msd_cnt > 0 then -- recompute a more accurate pan_direction in multiswipe context
+            prev_ms_ev = self.multiswipe_directions[msd_cnt][2]
+            fake_first_tev = {
+                [slot] = {
+                    ["x"] = prev_ms_ev.pos.x,
+                    ["y"] = prev_ms_ev.pos.y,
+                    ["slot"] = slot,
+                },
+            }
+            pan_direction = self:getPath(slot, true, fake_first_tev)
+        end

-        if msd_cnt == 0
-           or pan_direction ~= msd_direction_prev
-        then
+        if msd_cnt == 0 or pan_direction ~= msd_direction_prev then
             local prev_ms_ev, fake_first_tev

(This last if or then was a psychological blocker :) I had to flatten it to move on :)

Anyway, it seems there's a bit more thought to give to that part - and that's not the kind of stuff I like thinking about (x, y, relative stuff...).

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 18, 2019

Oh, I see what you mean now. While refactoring I apparently incorrectly simplified that part too much. When I was testing I had little trouble making gestures like up, right or even rather complex ones like up, right, down, left, but it looks like now it's indeed actually virtually impossible.

I think functionality-wise it was probably very similar if not identical to how you wrote it now. I had two if blocks with code duplication.

Stupidly, I didn't save a pre-refactored branch, only a pre-final touches branch. The thing is, if I don't clean up my branches I end up with dozens upon dozens of the things in no time, so I toss them out when I'm done. :-/ (And I just OTA-updated my older testing file on my H2O earlier today, too.)

Frenzie added a commit to Frenzie/koreader that referenced this pull request Feb 22, 2019

[fix] GestureDetector: fix multiswipe direction detection code
Unfortunately a mistake snuck into the final steps of refactoring <koreader#4607>.

Thanks to @poire-z for pointing it out [here](koreader#4607 (comment)).

Frenzie added a commit that referenced this pull request Feb 22, 2019

[fix] GestureDetector: fix multiswipe direction detection code (#4640)
Unfortunately a mistake snuck into the final steps of refactoring <#4607>.

Thanks to @poire-z for pointing it out [here](#4607 (comment)).
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.