-
-
Notifications
You must be signed in to change notification settings - Fork 112
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
Changes from 3 commits
504eb53
3aee911
be19674
cd21f02
72af79b
2a9aaa5
bed8699
f9cb118
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,6 +3,9 @@ | |||||||||||||||||||||||||||||
// after configuration. | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import { InputSample } from "../headless/inputSample.js"; | ||||||||||||||||||||||||||||||
import { Mutable } from "../mutable.js"; | ||||||||||||||||||||||||||||||
import { Nonoptional } from "../nonoptional.js"; | ||||||||||||||||||||||||||||||
import { PaddedZoneSource } from "./paddedZoneSource.js"; | ||||||||||||||||||||||||||||||
import { RecognitionZoneSource } from "./recognitionZoneSource.js"; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// For example, customization of a longpress timer's length need not be readonly. | ||||||||||||||||||||||||||||||
|
@@ -81,4 +84,34 @@ export interface GestureRecognizerConfiguration<HoveredItemType> { | |||||||||||||||||||||||||||||
* @returns | ||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||
readonly itemIdentifier?: (coord: Omit<InputSample<any>, 'item'>, target: EventTarget) => HoveredItemType; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
export function preprocessRecognizerConfig<HoveredItemType>( | ||||||||||||||||||||||||||||||
config: GestureRecognizerConfiguration<HoveredItemType> | ||||||||||||||||||||||||||||||
): Nonoptional<GestureRecognizerConfiguration<HoveredItemType>> { | ||||||||||||||||||||||||||||||
// Allows configuration pre-processing during this method. | ||||||||||||||||||||||||||||||
let processingConfig: Mutable<Nonoptional<GestureRecognizerConfiguration<HoveredItemType>>> = {...config} as Nonoptional<GestureRecognizerConfiguration<HoveredItemType>>; | ||||||||||||||||||||||||||||||
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); | ||||||||||||||||||||||||||||||
Comment on lines
+94
to
+101
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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:
Ah. Got it, the concern would make more sense if the code were appearing without any context. |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
if(!config.paddedSafeBounds) { | ||||||||||||||||||||||||||||||
let paddingArray = config.safeBoundPadding; | ||||||||||||||||||||||||||||||
Comment on lines
+103
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two member names are super conflatable! |
||||||||||||||||||||||||||||||
if(typeof paddingArray == 'number') { | ||||||||||||||||||||||||||||||
paddingArray = [ paddingArray ]; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
paddingArray = paddingArray ?? [3]; | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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
From: keyman/common/web/gesture-recognizer/src/engine/configuration/gestureRecognizerConfiguration.ts Lines 52 to 63 in be19674
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. |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
processingConfig.paddedSafeBounds = new PaddedZoneSource(processingConfig.safeBounds, paddingArray); | ||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||
// processingConfig.paddedSafeBounds is already set via the spread operator above. | ||||||||||||||||||||||||||||||
delete processingConfig.safeBoundPadding; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
return processingConfig; | ||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,6 @@ | ||
import { GestureRecognizerConfiguration } from "./configuration/gestureRecognizerConfiguration.js"; | ||
import { GestureRecognizerConfiguration, preprocessRecognizerConfig } from "./configuration/gestureRecognizerConfiguration.js"; | ||
import { MouseEventEngine } from "./mouseEventEngine.js"; | ||
import { Mutable } from "./mutable.js"; | ||
import { Nonoptional } from "./nonoptional.js"; | ||
import { PaddedZoneSource } from "./configuration/paddedZoneSource.js"; | ||
import { TouchEventEngine } from "./touchEventEngine.js"; | ||
import { TouchpointCoordinator } from "./headless/touchpointCoordinator.js"; | ||
|
||
|
@@ -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 commentThe 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 😁 |
||
config: GestureRecognizerConfiguration<HoveredItemType> | ||
): Nonoptional<GestureRecognizerConfiguration<HoveredItemType>> { | ||
// Allows configuration pre-processing during this method. | ||
let processingConfig: Mutable<Nonoptional<GestureRecognizerConfiguration<HoveredItemType>>> = {...config} as Nonoptional<GestureRecognizerConfiguration<HoveredItemType>>; | ||
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); | ||
|
||
if(!config.paddedSafeBounds) { | ||
let paddingArray = config.safeBoundPadding; | ||
if(typeof paddingArray == 'number') { | ||
paddingArray = [ paddingArray ]; | ||
} | ||
paddingArray = paddingArray ?? [3]; | ||
|
||
processingConfig.paddedSafeBounds = new PaddedZoneSource(processingConfig.safeBounds, paddingArray); | ||
} else { | ||
// processingConfig.paddedSafeBounds is already set via the spread operator above. | ||
delete processingConfig.safeBoundPadding; | ||
} | ||
|
||
return processingConfig; | ||
} | ||
|
||
public constructor(config: GestureRecognizerConfiguration<HoveredItemType>) { | ||
super(); | ||
this.config = GestureRecognizer.preprocessConfig(config); | ||
this.config = preprocessRecognizerConfig(config); | ||
|
||
this.mouseEngine = new MouseEventEngine<HoveredItemType>(this.config); | ||
this.touchEngine = new TouchEventEngine<HoveredItemType>(this.config); | ||
|
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!)