Skip to content
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.

Sign up for GitHub

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

fix(web): infinite model-match replacement looping 🪠 #10838

Merged
merged 5 commits into from
Apr 3, 2024
Merged

fix(web): infinite model-match replacement looping 🪠 #10838

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
028ff13
fix(web): infinite model-match replacement looping
jahorton Feb 26, 2024
e8fb4a1
change(web): more explicit 'cancelled'-rejection handling, documentation
jahorton Feb 27, 2024
03a329a
docs(web): minor comment tweak
jahorton Feb 27, 2024
f6fdca2
chore(web): Merge branch 'fix/web/early-matchGesture-abort' into fix/…
jahorton Mar 8, 2024
a6144c0
chore(web): Merge branch 'fix/web/early-matchGesture-abort' into fix/…
jahorton Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions common/web/gesture-recognizer/src/engine/headless/gestures/matchers/gestureMatcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,7 @@ export class GestureMatcher<Type, StateToken = any> implements PredecessorMatch<
}
}

// Check that initial "item" and "state" properties are legal for this type of gesture.
if(contactSpec.model.allowsInitialState) {
const initialStateCheck = contactSpec.model.allowsInitialState(
simpleSource.currentSample,
Expand All @@ -450,13 +451,40 @@ export class GestureMatcher<Type, StateToken = any> implements PredecessorMatch<
// pathMatcher for a source that failed to meet initial conditions.
this.pathMatchers.pop();

this.finalize(false, 'path');
/*
* To prevent any further retries for the model (via rejectionActions), we list the
* cause as 'cancelled'. The rejection-action mechanism already blocks paths that
* are rejected synchronously by this method; this adds an extra layer of protection
* and is likely also more clear.
*
* Alternatively, 'item' _should_ be fine - and corresponds best with a
* rejection based on the initial item.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand the second part of this comment. It's somewhat vague, using words like "likely" and "should" but it doesn't really clarify. Likely more clear than what? What part of the code uses the 'cancelled' cause and how does its behaviour differ? What is 'this method' -- is that 'this.finalize' or something else?

Saying that alternatively 'item' should be fine suggests that this change is wrong... particularly given the second half of that sentence.

Code comments should not be describing the change that you've made (from 'path' to 'cancelled') -- that belongs in a commit comment. The code comment needs to stand on its own explaining what is happening and why.

Can we use consistent comment formatting? This is a nit but elsewhere you are using //. The * on the front of each line format should be reserved for JSDoc style comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use consistent comment formatting? This is a nit but elsewhere you are using //. The * on the front of each line format should be reserved for JSDoc style comments.

I thought JSDoc style comments use a /** opening - two asterisks - rather than just /*? Plain /* is just a plain multi-line comment, isn't it? (VS Code certainly ignores /* when it comes to JSDoc - it only looks at the /** comments.)

Yeah, in the past (and sometimes even currently) I've often used lots of // for larger multiline comments, but I've been trying to work on that in favor of using a plain multiline comment style for sizably multiline comments like this one. Would it be better if I nixed the intermediate * line prefixes but kept it otherwise intact?

Part of why I've been trying to move away from // for larger comments like this is that they're a bit of a pain to maintain as line-width shifts; I find it less tedious to maintain that aspect when using a multi-line comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, JSDoc needs the /** but then the subsequent lines look like JSDoc but aren't. But it's not a big deal, just a bit of a mental shift when moving between different comment blocks.

For formatting multi-line comments, I use the VSCode extension Rewrap -- select the comment, press Alt+Q and it will re-wrap it (or Alt+Shift+Q to unwrap). Also works wonders with Markdown, commit messages, etc etc. Saves me so much angst 😁

this.finalize(false, 'cancelled');
}
}

contactModel.update();
// Now that we've done the initial-state check, we can check for instantly-matching path models.
// Now that we've done the initial-state check, we can check for instantly-matching and
// instantly-rejecting path models... particularly from from causes other than initial-item
// and state, such as rejection due to an extra touch.
//
// KMW example: longpresses cancel when a new touch comes in during the longpress timer;
// they should never become valid again for that base touch.
const result = contactModel.update();
if(result?.type == 'reject') {
/*
* To prevent any further retries for the model (via rejectionActions), we list the
* cause as 'cancelled'. The rejection-action mechanism already blocks paths that
* are rejected synchronously by this method; this adds an extra layer of protection
* and is likely also more clear.
*
* Alternatively, 'path' _should_ be fine - and corresponds best with a
* rejected contact-path-model.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same notes apply to this comment as for ll.454-462

this.finalize(false, 'cancelled');
}

// Standard path: trigger either resolution or rejection when the contact model signals either.
contactModel.promise.then((resolution) => {
this.finalize(resolution.type == 'resolve', resolution.cause);
});
Expand Down
Loading