-
-
Notifications
You must be signed in to change notification settings - Fork 111
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(web): gesture processing - path memory-use optimization 🐵 #10169
feat(web): gesture processing - path memory-use optimization 🐵 #10169
Conversation
User Test ResultsTest specification and instructions User tests are not required |
06eeb96
to
a8e7320
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Models the path over time through coordinate space taken by a touchpoint during | ||
* its active lifetime. | ||
* | ||
* _Supported events_: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see Event mentioned in this class and so I have a hard time understanding how the events described in the comment and GestureDebugPath
connect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this started off by duplicating GesturePath
.
Note that since GesturePath
is the parent class, this class actually does provide those events as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this file be renamed to gestureDebugPath.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent of these tests was always to validate GesturePath
, the standard use-case variant. The tests involve assertions that can now only be fulfilled by the "Debug" variant.
If preferred, I could spend time doing a more "proper" split for the tests, but I figured that was comparatively low priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this file be renamed to gestureDebugSource.spec.ts
?
common/web/gesture-recognizer/src/engine/headless/gestureDebugPath.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Marc Durdin <[email protected]>
The default mode for the gesture engine will no longer persistently store all coordinates visited by each touchpoint. All relevant data is accumulated within the
CumulativePathStats
object and can be obtained from there. This change thus should reduce runtime memory overhead, particularly for long-lived continuous gestures. (Intermediate coordinates may now be garbage-collected quite rapidly, even while their touchpoint lives on.)Accordingly, a new configuration flag has been added:
recordingMode
. This flag reactivates persistent coordinate storing, which is useful for some of the existing automated tests and for, well, "recording" input sequences. It might also be of use when debugging gesture-related issues.I'd always intended for this change to occur at some point... I'd just always "put off" finding the time to handle the few related loose ends. The biggest "loose end" was the handling of source "subview" generation as it relates to path inheritance when transitioning between gesture-model states. The other loose ends are all handled within the first commit.
@keymanapp-test-bot skip