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

Ignore oblique swipes if they do not have a command set. This improve… #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mherzberg
Copy link

…s recognition if only vertical and horizontal swipes are used.

…s recognition if only vertical and horizontal swipes are used.
@Coffee2CodeNL
Copy link
Owner

I think it's better to add a [[general]] section to the configuration file and add a toggle there like oblique = true, for performance reasons

@oryasida
Copy link
Contributor

Or check if one of the oblique commands is set, and then set an oblique flag, which can be checked in the if statement checking for oblique swipes.

@Coffee2CodeNL
Copy link
Owner

@oryasida that would have a nasty side effect, what if only that oblique command isn't set and the rest is?

@oryasida
Copy link
Contributor

oryasida commented Feb 23, 2019

How is that different from what you're proposing? If one of the oblique commands is set, we check for all of them, if none we don't check for any.
Otherwise, this commit does (as far as I can tell) the other possibility, if the found oblique swipe is set we do it, if not, it does the nearest non oblique swipe.

@Coffee2CodeNL
Copy link
Owner

I rather meant that checking for one then all is implicit configuration, I'm more a fan of explicit configuration.

if oblique = true is set in [[general]], then run the oblique commands and check for oblique motion.
if oblique = false is set in [[general]], then ignore the oblique commands and don't check for oblique motion.

This also gives the advantage of being able to toggle the oblique commands on and off without having to remove them from the config file.

Or I must be misunderstanding things and badly explaining things because it's getting quite late here and I'm quite tired 😛

@mherzberg
Copy link
Author

With my changes I had the following in mind: If you don't have oblique commands set, you probably want a bit more flexibility with making your horizontal/vertical gestures and only want to detect those. Maybe you now add one oblique command; you want that one recognized, but still want a bit more flexibility drawing your gestures into the other directions. If you have all four oblique commands set, my changes do not change the current behavior.

@oryasida
Copy link
Contributor

@Coffee2CodeNL
I see what you mean, explicit configuration does sound like a better solution.
But it would still have the disadvantage of either having oblique commands entirely off or entirely on.
For example:
If the only oblique command set is left_down, and up,down,left,right are also set, then there is no reason to check for the right_up gesture, and it could fall back to either up or right (depending on whats more dominant). With a global oblique toggle, if it is set to true then the right_up gesture would get recognized for no reason.
As far as I can tell this commit in this case would ignore the right_up gesture since it is not set (even though the leg_down gesture is set).

@Coffee2CodeNL
Copy link
Owner

Coffee2CodeNL commented Feb 23, 2019

I'll think about it for a bit for the rest of the evening and tomorrow, probably some hybrid/combined solution and/or splitting up the responsibilities (detection vs checking vs execution)

Who knows, we could add multi-threading and separate input from execution completely for performance reasons 🤷‍♂️

Or is that too crazy? 😛

I'd also like to invite both of you to the Discord server, makes brainstorming a bit easier and keeps PRs straight to the point and on topic, see the Readme for the link.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants