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] GestureDetector: add multiswipes #4606

Merged
merged 1 commit into from Feb 17, 2019

Conversation

Projects
None yet
2 participants
@Frenzie
Copy link
Member

Frenzie commented Feb 17, 2019

This morning I finished my thought from about a year ago that I mentioned in passing last night #4601 (comment)

I haven't quite got the GestureDetector end of it sorted out. I'll probably write it up GUI-less for now, other than an on/off switch and a first time use confirmation dialog.


The basic idea is that you gain an infinite amount of extra gestures, although in practice you're probably mostly limited to your 16 basic two swipe combinations and maybe a few three swipe ones.

@Frenzie Frenzie requested review from NiLuJe and poire-z Feb 17, 2019

[feat] GestureDetector: add multiswipes
The basic idea is that you gain an infinite amount of extra gestures,
although in practice you're probably mostly limited to your 16 basic
two swipe combinations and maybe a few three swipe ones.

@Frenzie Frenzie force-pushed the Frenzie:multigesture branch from 866c671 to 7949dfd Feb 17, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 17, 2019

Your other PR #4607 seems to include this one (?). Where do you want our comments?

and math.abs(y_diff) < 1.732*math.abs(x_diff) then
if not simple
and math.abs(y_diff) > 0.577*math.abs(x_diff)
and math.abs(y_diff) < 1.732*math.abs(x_diff)

This comment has been minimized.

@poire-z

poire-z Feb 17, 2019

Contributor

Not your numbers I guess, but no comment says where 0.577 and 1.732 come from. Any idea?

This comment has been minimized.

@Frenzie

Frenzie Feb 17, 2019

Author Member

Looks like it's from the early days of the program in 15eccb4 but that does not, of course, answer the question.

This comment has been minimized.

@Frenzie

Frenzie Feb 17, 2019

Author Member

Also see here for the first. I knew it looked familiar.

The other one seems to be the result of √3.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 17, 2019

Comments for GestureDetector here please. :-)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 17, 2019

OK, but I have no more comment :) (I guess the tricky stuff is in GestureDetector:handlePan() , and I trust you it works well :)

-- use first pan tev coordination as swipe start point
pos = start_pos,
direction = swipe_direction,
multiswipe_directions = multiswipe_directions,

This comment has been minimized.

@Frenzie

Frenzie Feb 17, 2019

Author Member

@poire-z Perhaps this should just be direction. My original concept was to sneak multiswipes into regular swipes, but the kind of gestures that are broken by this are actually bugs anyway.

(E.g., for a "long diagonal swipe" you can swipe by two edges in what's now properly detected as a "multiswipe".)

This comment has been minimized.

@Frenzie

Frenzie Feb 17, 2019

Author Member

That is, the ges = ges line was something I changed last minute before creating the PR.

This comment has been minimized.

@poire-z

poire-z Feb 17, 2019

Contributor

Well, I don't have much of an opinion, I've never gone deep enough in the gesture code.
Question: two same-direction swipes (with a slight angle variation) still continue being a single swipe? or can you get multiswipe southeast_southeast?

This comment has been minimized.

@Frenzie

Frenzie Feb 17, 2019

Author Member

You can give it a go with diagonal multigestures but I found it a bit error-prone. But perhaps I still had the threshold too small then.

There's the line not pan_direction:match(msd_direction_prev) which matches southeast to south or east (whichever the currently going direction is). So only something completely different will go through at all.

Then later in if msd_direction ~= msd_direction_prev then it's ensured that the same direction isn't added twice.

To try diagonal multigestures you'd need to get rid of the "simple" path and that :match line.

This comment has been minimized.

@poire-z

poire-z Feb 17, 2019

Contributor

I guess we don't really need to support diagonal multi gestures, and I somehow think it's best not to, to ease distinguish them and avoid mis-decisions.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 17, 2019

Yes, that part works well with the addition of the newly added safety checks. My draft implementation from last year had some issues along these lines:

So I needed a minimum threshold, for which I needed arbitrary path detection. That all works perfectly.

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 17, 2019

Btw, any thoughts on this one or its follow-up @pazos ?

@Frenzie

This comment has been minimized.

Copy link
Member Author

Frenzie commented Feb 17, 2019

Alright, I'll merge this tonight so the feedback can start pouring in.

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

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:multigesture branch Feb 17, 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.