Added debouncing to prevent from frequent triggering #20879
Conversation
There was a problem hiding this comment.
Thanks!!
Could you clean the diff (files changed tab) so only relevant changes are visible
Feel free to squash + force push, the unnecessary changes make it hard to understand the intent of the change
From the "files changed", this does not appear to impact the new study screen (ReviewerFragment)
|
Thank you @david-allison for guiding me , I am working on this . Will update it soon |
|
3000ms feels too much, doesn't it? Is 500ms or even 1000ms not enough to prevent the bug? |
|
@ZornHadNoChoice Yeah in new devices 1000ms is enough but i tested in my 2gb realme c30s and it was easily crossing 1000ms that too on frequent shakes. So , for keeping older and low ram devices in mind i changed it to 2500ms and its working fine now |
BrayanDSO
left a comment
There was a problem hiding this comment.
Subclassing ShakeDetector would be cleaner IMO
david-allison
left a comment
There was a problem hiding this comment.
Thanks!!
Architecturally, this would be better handled inside a custom ShakeDetector. See how we construct it here:
This also opens us up for future refactorings: removing sensorManager from ReviewerFragment and moving it into the custom class.
I'll need to test the debounce threshold on a physical phone. Regardless of the outcome, the constant should be extracted, and the reason that you selected the value should be documented.
Use a Duration for the time: val cooldown = 2500.milliseconds or val cooldown = 2.5.seconds
|
probably 1500ms or 2000ms is good enough |
|
@BrayanDSO yeah 1500ms or 2000ms can be good but 2500 is for old and laggy phones |
|
@david-allison i am creating the wrapper in com.ichi2.anki.utils directory |
|
@david-allison I have created the custom class for sensorManager. Requested for your review |
| private val sensorManager: SensorManager?, | ||
| private val sensorDelay: Int, | ||
| private val listener: ShakeDetector.Listener, | ||
| // The minimum time between two shakes in milliseconds . 2500 is used as default value |
There was a problem hiding this comment.
Why did you select this value? Due to your testing on what device?
There was a problem hiding this comment.
I chose this value because the setduedate dialog of AbstractFlashCardViewer (legacy screen) was appearing multiple times during frequent shaking in my realme c30s . It is the older 2gb ram phone , so i thought this problem can also arise in more older phones. That's why to be safe , i chose higher threshold
There was a problem hiding this comment.
the time logic should work the same independently of how slow the phone is
There was a problem hiding this comment.
This should be a comment in the code.
As Brayan said, timing should also be phone-independent, select a value which works for you here, don't make assumptions we'll need to be safe for legacy hardware.
|
@david-allison |
|
@david-allison fixed the requested changes . Require your review |
| * in rapid succession during a single physical shake event. | ||
| */ | ||
| class AnkiShakeDetector( | ||
| // The sensor manager to use for the shake detection |
There was a problem hiding this comment.
| // The sensor manager to use for the shake detection |
| private val sensorManager: SensorManager?, | ||
| private val sensorDelay: Int, | ||
| private val listener: ShakeDetector.Listener, | ||
| // The minimum time between two shakes in milliseconds . 2500 is used as default value |
There was a problem hiding this comment.
This should be a comment in the code.
As Brayan said, timing should also be phone-independent, select a value which works for you here, don't make assumptions we'll need to be safe for legacy hardware.
|
@david-allison your requested changes have already fixed . You reviewed the older code as your comments have outdated . Can you please recheck it . As i moved the AnkiShakeDetector from utils directory to android |
|
Thanks! #20879 (comment) is still pending |
|
@BrayanDSO I updated the code . Needs your review |
| import kotlin.time.Duration.Companion.milliseconds | ||
|
|
||
| /* | ||
| * Wrapper for a [ShakeDetector] to provide a cooldown mechanism. |
There was a problem hiding this comment.
| * Wrapper for a [ShakeDetector] to provide a cooldown mechanism. | |
| * Wrapper for a [ShakeDetector] to provide a cooldown mechanism. |
|
@david-allison @BrayanDSO Thank you for mentoring me in this issue . As this was my first open source contribution . Can i get the short feedback so that i can improve for future contributions. |
1dad002 to
be22fef
Compare
BrayanDSO
left a comment
There was a problem hiding this comment.
You mixed your commit with another one. Please rebase your branch with only our changes
|
@BrayanDSO I fixed that |
|
address David's comment about the copyright header |
|
@BrayanDSO It was just the small formatting . I already fixed that |
|
so push the changes. It looks the same here. |
|
@BrayanDSO Fixed that |
BrayanDSO
left a comment
There was a problem hiding this comment.
other cleanups can be made later. LGTM
|
@BrayanDSO Can this be merged now |
Purpose / Description
The shake gesture was triggering multiple times on frequent shakes
Fixes
Approach
Added debouncing so that there should be atleast 2500ms between two shake gestures to prevent frequent triggering
How Has This Been Tested?
ankidroid_shake_fix.mp4
You can set the shake gesture to any of the action specifically "set due date" and try to shake frequently on old screen and you can test with adding or editing notes in new study screen
Learning (optional, can help others)
Describe the research stage
Links to blog posts, patterns, libraries or addons used to solve this problem
Checklist
Please, go through these checks before submitting the PR.