feat(web): allows shifting the recognition zone of an active GestureSource 🐵 #9526

jahorton · 2023-08-30T03:46:28Z

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

…source lifetime

keymanapp-test-bot · 2023-08-30T03:46:32Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

Android
Developer
- Keyman Developer - build failure
- Compiler Regression Tests - build failure
- kmcomp.zip - build failure
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 17.0.181 (0.9526.9253)
Keyboards
- Test Keyboards - build failure
Web
- KeymanWeb Test Home
Windows
- Keyman for Windows - build failure
- FirstVoices Keyboards for Windows - build failure
- Text Editor (32 bit) - build failure
- Text Editor (64 bit) - build failure

mcdurdin

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 😀

mcdurdin · 2023-09-02T03:16:58Z

common/web/gesture-recognizer/src/engine/configuration/gestureRecognizerConfiguration.ts

+  config: GestureRecognizerConfiguration<HoveredItemType>
+): Nonoptional<GestureRecognizerConfiguration<HoveredItemType>> {
+  // Allows configuration pre-processing during this method.
+  let processingConfig: Mutable<Nonoptional<GestureRecognizerConfiguration<HoveredItemType>>> = {...config} as Nonoptional<GestureRecognizerConfiguration<HoveredItemType>>;


That is one heck of a type -- starting to feel downright Java!

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!)

mcdurdin · 2023-09-02T03:18:04Z

common/web/gesture-recognizer/src/engine/configuration/gestureRecognizerConfiguration.ts

+  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);


Some comments on why these are being setup this way would be helpful

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.

mcdurdin · 2023-09-02T03:18:26Z

common/web/gesture-recognizer/src/engine/configuration/gestureRecognizerConfiguration.ts

+    if(typeof paddingArray == 'number') {
+      paddingArray = [ paddingArray ];
+    }
+    paddingArray = paddingArray ?? [3];


Why 3? Magic?

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.

mcdurdin · 2023-09-02T03:19:05Z

common/web/gesture-recognizer/src/engine/configuration/gestureRecognizerConfiguration.ts

+  if(!config.paddedSafeBounds) {
+    let paddingArray = config.safeBoundPadding;


These two member names are super conflatable!

mcdurdin · 2023-09-02T03:19:36Z

common/web/gesture-recognizer/src/engine/gestureRecognizer.ts

@@ -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>(


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

common/web/gesture-recognizer/src/test/resources/simulateMultiSourceInput.ts

jahorton · 2023-09-22T02:01:54Z

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

…ting-and-source-rigor

jahorton added 3 commits August 30, 2023 10:14

feat(web): adds support for pushed alt-configurations during gesture-…

504eb53

…source lifetime

fix(web): patches up existing unit-tests

3aee911

feat(web): adds numerous GestureSource unit tests

be19674

jahorton added this to the A17S20 milestone Aug 30, 2023

jahorton requested a review from mcdurdin as a code owner August 30, 2023 03:46

keymanapp-test-bot bot added the epic-gestures label Aug 30, 2023

github-actions bot added common/ common/web/ feat web/ labels Aug 30, 2023

mcdurdin modified the milestones: A17S20, A17S21 Sep 1, 2023

mcdurdin requested changes Sep 2, 2023

View reviewed changes

mcdurdin modified the milestones: A17S21, A17S22 Sep 15, 2023

github-actions bot added web/ and removed web/ labels Sep 22, 2023

jahorton force-pushed the feat/web/config-shifting-and-source-rigor branch from 4f182d8 to be19674 Compare September 22, 2023 02:01

github-actions bot added web/ and removed web/ labels Sep 22, 2023

jahorton added 3 commits September 26, 2023 12:18

chore(web): Merge branch 'refactor/web/matcher-abstraction' into feat…

cd21f02

…/web/config-shifting-and-source-rigor

chore(web): Merge branch 'refactor/web/matcher-abstraction' into feat…

72af79b

…/web/config-shifting-and-source-rigor

chore(web): applies PR suggestions

2a9aaa5

github-actions bot added web/ and removed web/ labels Sep 26, 2023

chore(web): FIX ME was already fine!?

bed8699

github-actions bot added web/ and removed web/ labels Sep 26, 2023

jahorton requested a review from mcdurdin September 26, 2023 07:58

Base automatically changed from refactor/web/matcher-abstraction to feature-gestures September 27, 2023 01:56

chore(web): Merge branch 'feature-gestures' into feat/web/config-shif…

f9cb118

…ting-and-source-rigor

github-actions bot added web/ and removed web/ labels Sep 27, 2023

mcdurdin approved these changes Sep 27, 2023

View reviewed changes

jahorton merged commit ca6b4f9 into feature-gestures Sep 27, 2023
17 of 19 checks passed

jahorton deleted the feat/web/config-shifting-and-source-rigor branch September 27, 2023 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(web): allows shifting the recognition zone of an active GestureSource 🐵 #9526

feat(web): allows shifting the recognition zone of an active GestureSource 🐵 #9526

jahorton commented Aug 30, 2023

keymanapp-test-bot bot commented Aug 30, 2023 •

edited

Loading

mcdurdin left a comment

mcdurdin Sep 2, 2023

jahorton Sep 26, 2023 •

edited

Loading

mcdurdin Sep 2, 2023

jahorton Sep 26, 2023

mcdurdin Sep 2, 2023

jahorton Sep 26, 2023

mcdurdin Sep 2, 2023

mcdurdin Sep 2, 2023

jahorton commented Sep 22, 2023 •

edited

Loading

	* If not specified, this will default to a padding of 3px inside the standard safeBounds
	* unless `paddedSafeBounds` is defined.

	/**
	* 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(!config.paddedSafeBounds) {
		let paddingArray = config.safeBoundPadding;

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

jahorton commented Aug 30, 2023

keymanapp-test-bot bot commented Aug 30, 2023 • edited Loading

User Test Results

Test Artifacts

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin Sep 2, 2023

Choose a reason for hiding this comment

jahorton Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

mcdurdin Sep 2, 2023

Choose a reason for hiding this comment

jahorton Sep 26, 2023

Choose a reason for hiding this comment

mcdurdin Sep 2, 2023

Choose a reason for hiding this comment

jahorton Sep 26, 2023

Choose a reason for hiding this comment

mcdurdin Sep 2, 2023

Choose a reason for hiding this comment

mcdurdin Sep 2, 2023

Choose a reason for hiding this comment

jahorton commented Sep 22, 2023 • edited Loading

keymanapp-test-bot bot commented Aug 30, 2023 •

edited

Loading

jahorton Sep 26, 2023 •

edited

Loading

jahorton commented Sep 22, 2023 •

edited

Loading