-
-
Notifications
You must be signed in to change notification settings - Fork 109
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): allows shifting the recognition zone of an active GestureSource 🐵 #9526
feat(web): allows shifting the recognition zone of an active GestureSource 🐵 #9526
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
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.
Looking good, I think. I am struggling to keep the abstractions in my head and relate them to how gestures work though!
I see that the build failed, but here's some review feedback anyway 😀
config: GestureRecognizerConfiguration<HoveredItemType> | ||
): Nonoptional<GestureRecognizerConfiguration<HoveredItemType>> { | ||
// Allows configuration pre-processing during this method. | ||
let processingConfig: Mutable<Nonoptional<GestureRecognizerConfiguration<HoveredItemType>>> = {...config} as Nonoptional<GestureRecognizerConfiguration<HoveredItemType>>; |
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.
That is one heck of a type -- starting to feel downright Java!
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 always have loved generics a little too much... and yeah, I felt like this was brushing up against "too much". Fits well, though.
FWIW, it can get much worse - it's been noted that TypeScript's type system is Turing-complete. That'd clearly be way too far.
Okay, maybe I don't love them that much - some people clearly had fun writing their analyses in that issue. (Like the guy who wrote a MUD with them!)
processingConfig.mouseEventRoot = processingConfig.mouseEventRoot ?? processingConfig.targetRoot; | ||
processingConfig.touchEventRoot = processingConfig.touchEventRoot ?? processingConfig.targetRoot; | ||
|
||
processingConfig.inputStartBounds = processingConfig.inputStartBounds ?? processingConfig.targetRoot; | ||
processingConfig.maxRoamingBounds = processingConfig.maxRoamingBounds ?? processingConfig.targetRoot; | ||
processingConfig.safeBounds = processingConfig.safeBounds ?? new PaddedZoneSource([2]); | ||
|
||
processingConfig.itemIdentifier = processingConfig.itemIdentifier ?? (() => null); |
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.
Some comments on why these are being setup this way would be helpful
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.
Note that the documentation for the fields and their defaults has already been established in JSDoc-style comments on each of them. It shows up via IntelliSense within VS Code.
Granted, why the defaults are set as they are... that is a different question. Is that your concern?
Edit:
And ... now I see[...]
Ah. Got it, the concern would make more sense if the code were appearing without any context.
if(typeof paddingArray == 'number') { | ||
paddingArray = [ paddingArray ]; | ||
} | ||
paddingArray = paddingArray ?? [3]; |
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.
Why 3? Magic?
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.
keyman/common/web/gesture-recognizer/src/engine/configuration/gestureRecognizerConfiguration.ts
Lines 58 to 59 in be19674
* If not specified, this will default to a padding of 3px inside the standard safeBounds | |
* unless `paddedSafeBounds` is defined. |
From:
keyman/common/web/gesture-recognizer/src/engine/configuration/gestureRecognizerConfiguration.ts
Lines 52 to 63 in be19674
/** | |
* Used to define a "boundary" slightly more constrained than `safeBounds`. Events that | |
* start within this pixel range from a safe bound will disable that bound for the duration | |
* of its corresponding input sequence. May be a number or an array of 1, 2, or 4 numbers, | |
* as with CSS styling. | |
* | |
* If not specified, this will default to a padding of 3px inside the standard safeBounds | |
* unless `paddedSafeBounds` is defined. | |
* | |
* If `paddedSafeBounds` was specified initially, this will be set to `undefined`. | |
*/ | |
readonly safeBoundPadding?: number | number[]; |
If memory serves, this default is derived from the corresponding value KMW is already using for the OSK. I've just abstracted its purpose and given it a name and documentation.
if(!config.paddedSafeBounds) { | ||
let paddingArray = config.safeBoundPadding; |
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.
These two member names are super conflatable!
@@ -12,39 +10,9 @@ export class GestureRecognizer<HoveredItemType> extends TouchpointCoordinator<Ho | |||
private readonly mouseEngine: MouseEventEngine<HoveredItemType>; | |||
private readonly touchEngine: TouchEventEngine<HoveredItemType>; | |||
|
|||
protected static preprocessConfig<HoveredItemType>( |
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.
And ... now I see that my comments relate to pre-existing code. I still think they are good comments though 😁
common/web/gesture-recognizer/src/engine/headless/gestureSource.ts
Outdated
Show resolved
Hide resolved
common/web/gesture-recognizer/src/engine/headless/gestureSource.ts
Outdated
Show resolved
Hide resolved
common/web/gesture-recognizer/src/test/resources/simulateMultiSourceInput.ts
Outdated
Show resolved
Hide resolved
4f182d8
to
be19674
Compare
Accidentally pushed a new commit to this PR instead of the one after it in the chain; the force-push simply removed that one commit. Didn't want to revert, as I'd have to revert the reversion in the child PR, and that sounds like a recipe for a headache. |
…/web/config-shifting-and-source-rigor
…/web/config-shifting-and-source-rigor
…ting-and-source-rigor
This PR allows "pushing" an alternate configuration for the engine's "recognition zone" for an ongoing gesture. This is particularly designed to model the shift from a longpress to subkey-selection, which is generally within a smaller and more restrictive area.
The need for this was discovered as part of #9525; the changes are easily significant enough to merit spinning it off in this manner, as it'll ease that future review.
It also includes a number of new unit tests for
GestureSource
and adds new "guards" to certain methods, prohibiting a subview from performing update commands either on or separate from its underlying, base Source.@keymanapp-test-bot skip