fix(web): infinite model-match replacement looping 🪠 #10838
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
| /* | ||
| * 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. | ||
| */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😁
| /* | ||
| * 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. | ||
| */ |
There was a problem hiding this comment.
Same notes apply to this comment as for ll.454-462
|
Aside from heavily reworking the comments, I made a few extra changes; a 'cancelled' rejection-action was technically allowed on |
…web/infinite-model-replacement-loop
…web/infinite-model-replacement-loop
|
Changes in this pull request will be available for download in Keyman version 17.0.301-beta |
Addresses part of #10592.
Continues from #10836.
While investigating #10592, I discovered that during rapid-typing scenarios, there's often an asynchronously-infinite loop running within the gesture-engine whenever a simple-tap is autocompleted by a new incoming tap! (To be clear - this is neither intended nor desired.) This was occurring because we weren't checking the component paths' initial states during the replacement process - only the validity of the initial item(s). (Model replacement shouldn't proceed if the replacing model would instantly reject - for any cause.)
To be more detailed: in particular, the 'longpress' and 'longpress-roam' gesture models used by KeymanWeb are rejected when a new touch comes in... and it's not resulting in their permanent cancellation (which is what the design would intend). Instead, they're instantly rejecting/resetting again, constantly - each pass is one iteration of the aforementioned 'loop'. This unintended 'loop' auto-terminates whenever a gesture is positively matched or when none are able to match.
On one hand, this is why the oversight fixed by #10836 wasn't an issue before - it so happens that this loop 'sustains' the selection process and prevents early interruption of the model-matching process. On the other hand... a side-effect of the same loop caused a O(N) process per loop iteration, with N as the number of prior iterations - resulting in an O(N^2) effect on something that should be O(0). That's a really, really bad look. The resulting good news: this fix should improve the gesture engine's performance during rapid touch scenarios, helping to mitigate #10592 (and #10646). There are still other co-morbid issues that also need addressing, though.
If this PR is placed before #10836 in commit history, one unit test will fail. The failure would be due to removing the "sustaining" effect mentioned in the second paragraph.
@keymanapp-test-bot skip
As this PR seeks to address a host of related known errors but only fixes a bit of them, specifying a proper user test at this point would be quite tricky. #10843 will host the main user testing suite for this PR chain, as it exists as the final "rung" of this PR "ladder."