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.

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

Order of edit changes in TextDocumentChangeEvent.contentChanges is reverse the order of the actual edits #11487

Closed
drew-wallace opened this issue Sep 3, 2016 · 8 comments
Assignees
Labels
api *question Issue represents a question, should be posted to StackOverflow (VS Code)

Comments

@drew-wallace
Copy link

  • VSCode Version: 1.4.0
  • OS Version:10.11.6

Steps to Reproduce:

  1. Create an edit with an insert and a remove
  2. In the corresponding onDidChangeTextDocument event, look at the TextDocumentChangeEvent.contentChanges Notice how the order is reverse the order of the actual edit order.
@dbaeumer dbaeumer added the api label Sep 4, 2016
@jrieken jrieken assigned alexdima and unassigned jrieken Sep 5, 2016
@alexdima
Copy link
Member

alexdima commented Sep 5, 2016

@drew-wallace How do you build your edits? Via textEditor.edit?

Note they will all be applied at the "same time" and the order in which you specify them is of no consequence.

@drew-wallace
Copy link
Author

Here's a simple example:

vscode.window.activeTextEditor.edit(function (textEditorEdit) {
    textEditorEdit.insert(start, "first");
    textEditorEdit.insert(start, "second");
});

vscode.workspace.onDidChangeTextDocument(function(TextDocumentChangeEvent) {
    console.log(TextDocumentChangeEvent.contentChanges[0].text); // output: "second"
    console.log(TextDocumentChangeEvent.contentChanges[1].text); // output: "first"
});

@alexdima
Copy link
Member

alexdima commented Sep 6, 2016

Ok, I might have jumped ahead of myself :).

The order in which you specify the edits is of consequence if the edits touch the same location.

Perhaps an explanation of how this works would help:

  • the edits you add to the editBuilder are collected in an array and sent over from the extension host to the main process.
  • the edits are all applied "at once":
    • given the text hello and two edits:
    • first edit to insert a at (0,0)
    • second edit to insert b at (0,1)
    • the result of the edits is ahbello because a is inserted before h and b is inserted after h.
    • the edits are applied "at once" on the original model version. As an implementation detail, the last edit in the file is applied first, etc. I will touch on this point below.
    • if the edits would be applied one after the other, the result would have been abhello because first a would be inserted before h, resulting in ahello and b would be inserted after a.
    • you can think of the edits as a list of things to do on the current text (sort of like a patch where each patch acts on the original contents at the same time with all the other patch in the batch of patches)
  • as you have noticed the two edits proceed to generate a single document change event.
  • this document change event must again describe the "edits" that would allow somebody to maintain a model sync based on these delta events. We use these change events succesfully to maintain a mirror model of the main process model in the extension host (that's why we can make text document read APIs synchronous, because we have an up-to-date version of the main process documents using these change events).
  • your logic should not depend on the order or the correlation of the change events in regards to the edits you send. For example, you have discovered that we apply the edits from the last one in the file to the first one in the file. But we also do other things, which you might have not noticed. For example, if you send more than 1000 edits, for memory allocation purposes we will collapse them all into a single edit and proceed to emit a single content change event consisting of a very large range that contains all the 1000 edits logically. This is the sort of transformation where correctness is maintained, as there are many ways to express the same edit. I would like for us to reserve the right for this to be considered implementation detail.

I hope this makes sense, and perhaps you can explain what you're trying to do, maybe there are better ways to do what you want to do that do not introduce knowledge of the model edits implementation details.

@drew-wallace
Copy link
Author

I was trying to work around these issues: #11418 and #10801. The goal was to make an edit and attach an insert of a very long static string so that I could filter the edits that had it and the edits that don't. I needed this because I'm working on a remote pair programming extension and when a remote user makes an edit, the local user receives the edit, but the local user's listener picks up on it and triggers the same edit on the remote user, which causes an infinite loop.

This workaround didn't end up working anyway, because I was unable to delete that static string from within the listener. I realize it was probably a bad idea to force edits to have the feature I need since I don't entirely understand what kinds of things can happen when making large edits.

@alexdima
Copy link
Member

alexdima commented Sep 6, 2016

Cool use-case.

This is how I'd do it (pseudocode ignoring multiple documents but the principle should hold):

function delta(text1, text2) {
  // ... diff/LCS algorithm here
}

function patch(text, edits) {
  // apply edits produced by above delta to text here
}

// true when I've posted edits and I don't know if they were applied yet.
var waitingForApplyingMyEdits = false;

// Record last seen text and fetch current text,
// determine diff, send edits off to a server
var lastSentText = doc.getText();
function reconcile() {
  if (waitingForApplyingMyEdits) {
    return;
  }
  var currentText = doc.getText();
  var edits = delta(lastSentText, currentText);
  lastSentText = currentText;
  // send edits to server
}


function applyIncomingEdits(edits) {
  var currentText = doc.getText();
  var desiredText = patch(currentText, edits);

  // block any reconciliation until we submit this edit
  waitingForApplyingMyEdits = true;
  editor.edit((builder) => {
  }).then(function(applied) {
    waitingForApplyingMyEdits = false;
    if (!applied) {
       // TODO: try again! user changed the file in the meantime
    } else {
       // force to compute delta between what I expected and what actually happened
       lastSentText = desiredText;
       reconcile();
    }
  });
}

// 500ms after the last edit begin to reconcile
var reconcileTimeout = -1;
workspace.onDidChangeTextDocument((e) => {
  clearTimeout(reconcileTimeout);
  reconcileTimeout = setTimeout(reconcile, 500);
});

@siegebell
Copy link

siegebell commented Sep 6, 2016

  • if the edits would be applied one after the other, the result would have been abhello because first a would be inserted before h, resulting in ahello and b would be inserted after a.
  • you can think of the edits as a list of things to do on the current text (sort of like a patch where each patch acts on the original contents at the same time with all the other patch in the batch of patches)

@alexandrudima that's a subtle detail that needs to be documented.

So as each change in TextEditorEdit is applied to a document, the locations of all remaining changes need to be updated. In practice, you avoid having to update subsequent locations by first sorting the edits by location (descending order).

  • your logic should not depend on the order or the correlation of the change events in regards to the edits you send.

What about edits received -- can extension authors assume that the changes in TextDocumentChangeEvent have already been sorted by location?

@drew-wallace
Copy link
Author

@alexandrudima This could work, but I think I might wait for the #10801. Having the source of the edit would reduce complexity.

@alexdima
Copy link
Member

alexdima commented Sep 6, 2016

@siegebell Good point, the doc comment is lacking in this regard (explanation regarding applying edits at the same time). I remember describing this in great detail somewhere in https://github.com/Microsoft/vscode-docs/ but I think it got lost when we went open source.

That is exactly what I'm trying to prevent -- that you in any way depend on the order or the shape of the content change events. The contract is that if you begin following independently a certain document and consistently apply the content change events (in the order you received them) you will end up with a document in the same state (same text) as our document. No guarantess about the sort order, about the count, etc. I would like to keep the door open if we need to improve our edit applying code.

@alexdima alexdima closed this as completed Mar 2, 2017
@alexdima alexdima added the *question Issue represents a question, should be posted to StackOverflow (VS Code) label Mar 2, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api *question Issue represents a question, should be posted to StackOverflow (VS Code)
Projects
None yet
Development

No branches or pull requests

5 participants