Skip to content

Conversation

@jpolitz
Copy link
Owner

@jpolitz jpolitz commented Sep 17, 2025

No description provided.

@jpolitz jpolitz marked this pull request as draft September 17, 2025 15:59
@jpolitz
Copy link
Owner Author

jpolitz commented Sep 17, 2025

@blerner it would be awesome to get your eyes on this.

  • fine-grained edits forwarded rather than whole-file edits
  • flag for ignoring external events when we have queued up edits
  • checks for failed fine-grained edits that fall back on making an edit with the full source

This, along with brownplt/code.pyret.org#600, gives an experience I can't break.

In progress because there are a few bad things about this:

  1. The undo ends up being at a finer grain than I'd like – it is basically per character because CodeMirror's batching of edits isn't used for the undo stack; rather we update the TextDocument repeatedly and those updates form the undo stack in VScode.

    It's hard to respect CodeMirror's undo, though, while respecting the model of a CustomTextEditor.

    We could try to merge sequences when we see multiple sequential insertions (basically take from the head of the edit events all adjacent insertions and merge them). That's complicated but seems like it would behave well. I haven't found a way to get CodeMirror to report changes at the same cadence it adds to its history.

  2. This isn't quite done because of an issue with processing VScode's undo – if we do the “update the whole document” workflow, then each time an undo happens from VScode the cursor jumps to the end of the document instead of staying put, because it's a replacement of the whole range. So we need to synthesize more change events when propagating down rather than just using the hammer of setContents. More opportunities for small missed edits, I fear.

Copy link

@blerner blerner left a comment

Choose a reason for hiding this comment

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

Generally lgtm, I'll reread this more carefully later today

// whole document contents to match
if(!ok) {
console.error("applyEdit returned false, updating full contents", edit, source);
enqueueFullEdit(source);
Copy link

Choose a reason for hiding this comment

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

I worry that this line is both here and in the catch block, such that if this line itself causes a problem (somehow), then we immediately try to do it again...

Copy link
Owner Author

Choose a reason for hiding this comment

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

I hear you, but enqueueFullEdit is pure constructor/builder calls and push. I wish there was a TS annotation for “does not throw”!

Copy link

Choose a reason for hiding this comment

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

oh oh, I misread, and thought the close-brace of enqueueFullEdit included this processing step as well (i.e. enqueue, and immediately try to process if there's anything to process). Yeah, pure constructor calls should be simple enough. So then the only fallible line is the applyEdit call? Or is even that infallible, since you'll get the !ok result from the await? I guess I'm not quite sure what the try/catch is accomplishing...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, I have actually never generated an input that causes this catch to trigger. The docs for applyEdit do not say that it will ever throw (https://code.visualstudio.com/api/references/vscode-api#workspace), just return false. So this may be over-cautious of me. I'm just super paranoid about losing fine-grained edits that bork a file.

Copy link
Owner Author

Choose a reason for hiding this comment

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

(And other functions have throws documentation if they can, so I think the assumption is that applyEdit does not throw.)

// A more complete extension should compute minimal edits instead.
// NOTE(joe): we have these on the change events from CodeMirror
// Configure the edit from the CodeMirror change event in e.data.change
const from = e.data.change.from;
Copy link

Choose a reason for hiding this comment

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

Can you clean this to use {from, to, text}=e.data.change?

@jpolitz jpolitz merged commit a3163c7 into main Oct 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants