Improve handling of undo/redo events in web demo (#1254)#1255
Improve handling of undo/redo events in web demo (#1254)#1255johnbartholomew merged 1 commit intogoogle:masterfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
|
||
| for (let textarea_id in editor_by_textarea_id) { | ||
| let editor = editor_by_textarea_id[textarea_id]; | ||
| let startedJobId = 0; |
There was a problem hiding this comment.
So if I understand correctly you're using this to serialise processing each of the change events?
I think that's ok. Though I think there's another way to do it without the setTimeout: You can keep a reference to an 'ongoing change' to await, and then chain them. Something like this:
let ongoingChange = Promise.resolve(true);
editor.on('change', function() {
return ongoingChange.then(async () => {
try {
// previous change is complete and future changes are queued on our promise
// do the event processing
} catch (e) {
// log the error; if it unwinds further then it'll break the Promise chain
}
});
});What do you think? (I haven't tried running this to verify it!)
There was a problem hiding this comment.
Apologies, I meant to suggest chaining it like this (updating ongoingChange each time):
let ongoingChange = Promise.resolve(true);
editor.on('change', function() {
ongoingChange = ongoingChange.then(async () => {
try {
// previous change is complete and future changes are queued on our promise
// do the event processing
} catch (e) {
// log the error; if it unwinds further then it'll break the Promise chain
}
});
return ongoingChange;
});
There was a problem hiding this comment.
Nice! That is much cleaner.
|
Thanks for the contribution! It's great to have a fix for this! |
Fixes google#1254. In the demo on https://jsonnet.org, if you hit redo/undo for multiple characters, the demo breaks. This seems to be because the browser generates multiple text change events (Chrome 139, MacOS 14.6).
db03341 to
4dad794
Compare
|
Squashed, merged, and deployed to the website. Thanks for the fix! |
garyg1
left a comment
There was a problem hiding this comment.
Thanks for taking a look at this!
|
|
||
| for (let textarea_id in editor_by_textarea_id) { | ||
| let editor = editor_by_textarea_id[textarea_id]; | ||
| let startedJobId = 0; |
There was a problem hiding this comment.
Nice! That is much cleaner.
This avoids an arbitrary setTimeout and reduces the amount of state. Discussed in review comments on google#1255
Fixes #1254. In the demo on https://jsonnet.org, if you hit redo/undo for multiple characters, the demo breaks. This seems to be because the browser generates multiple text change events (Chrome 139, MacOS 14.6).
Previous Behavior
https://jsonnet.org/
Screen.Recording.2025-09-02.at.8.12.12.PM.mov
New Behavior
Screen.Recording.2025-09-02.at.8.12.43.PM.mov