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

Multiple text document changes when setting the entire content #289

Closed
wants to merge 1 commit into from
Closed

Conversation

arxanas
Copy link
Contributor

@arxanas arxanas commented Sep 1, 2017

It's unclear how a language server should react when multiple changes to the text document are sent, and one or more set the content of the entire document.

The convention so far among the language servers I've seen has been to apply the changes from the bottom of the document to the top. However, there is no obvious ordering for the whole-document changes. Should we apply the changes in the order that they are received (so that earlier whole-document changes are overridden by later whole-document changes), except that within consecutive subsequences of range-changes we apply them in bottom-to-top order?

This is a non-trivial amount of complexity for a situation that probably never arises in practice. Most likely the editor will either set the entire text of the document, or set ranges for the changes to the document (where, in some cases, the range may be the whole document). This commit clarifies this by forbidding whole-document changes in conjunction with other changes. It's also in line with the idea of forbidding overlapping edits as per #279.

It's unclear how a language server should react when multiple changes to the text document are sent, and one or more set the content of the entire document.

The convention so far among the language servers I've seen has been to apply the changes from the bottom of the document to the top. However, there is no obvious ordering for the whole-document changes. Should we apply the changes in the order that they are received (so that earlier whole-document changes are overridden by later whole-document changes), except that within consecutive subsequences of range-changes we apply them in bottom-to-top order?

This is a non-trivial amount of complexity for a situation that probably never arises in practice. Most likely the editor will either set the entire text of the document, or set ranges for the changes to the document (where, in some cases, the range may be the whole document). This commit clarifies this by forbidding whole-document changes in conjunction with other changes. It's also in line with the idea of forbidding overlapping edits as per #279.
@msftclas
Copy link

msftclas commented Sep 1, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@dbaeumer
Copy link
Member

dbaeumer commented Sep 7, 2017

@arxanas forbidding this is not necessary since the content changes describe single state moves. So if you have two content changes c1 and c2 for a document in state S10 then c1 move the document to S11 and c2 to S12.

So the array of content changes should simply be applied from 0 to contentChanges.length - 1.

I updated the documentation accordingly.

@dbaeumer dbaeumer closed this Sep 7, 2017
@arxanas
Copy link
Contributor Author

arxanas commented Sep 7, 2017

@dbaeumer: Is there discussion of that anywhere else? I fear that this is incompatible with how most editors are currently treating this: as a vehicle to support features like multiple cursors. In those cases, one can make changes to multiple places in the document simultaneously during the course of normal editing.

What is the point of supporting multiple changes in a single TextDocumentContentChangeEvent, when the editor could simply send multiple TextDocumentContentChangeEvent and have the same semantics?

I assumed that the point of having multiple changes in a TextDocumentContentChangeEvent was to support this use-case, which is most easily implemented in the editor by simply stating all the ranges before any edits were made and then stating the new contents of the each of those ranges. The consequence is that language servers have to apply the edits backward. I think we would have to change both Nuclide and all of our language servers to produce and consume the edits in the new fashion if that's not how the spec was intended to be interpreted.

Does VS Code have any multiple-cursor feature, and what does it do in that case?

cc @ljw1004: the corresponding commit can be found at fcb32f9

@dbaeumer
Copy link
Member

dbaeumer commented Sep 8, 2017

What is the point of supporting multiple changes in a single TextDocumentContentChangeEvent, when the editor could simply send multiple TextDocumentContentChangeEvent and have the same semantics?

The only reason here is performance / batching.

Regarding multi cursor: the assumption is that the server doesn't need to know anything about multi cursor. For example if there are two cursor and the user types a we generate a change with two content changes but treat them as individual states.

IMO there is no difference whether this would be one or two state changes. The client could even simulate one state change by setting the right version number in the VersionedTextDocumentIdentifier which is part of a text document change.

The important thing is that the content changes must be applied in order without the need for adjusting any positions which IMO is the easiest model for a server.

@alanz
Copy link

alanz commented Sep 8, 2017

The important thing is that the content changes must be applied in order without the need for adjusting any positions which IMO is the easiest model for a server.

In the order given in the message, or from the end of the file to the front?

@kdvolder
Copy link

kdvolder commented Sep 8, 2017

In the order given in the message, or from the end of the file to the front?

Actually, it doesn't really matter. Since the edits do not overlap, the order in which you apply them shouldn't matter.... however...

The caveat is that you have to think of all position references in the edits in terms of the coordinates of the original document to which all these edits are being applied. This means that if you have changes in the beginning of the document then, if you apply them first, then you have to adjust all the positions of the other edits, as the coordinates have been changed by the earlier edits. Now... if you sort the edits based on position and then apply them starting from the end of the document, then you don't have to worry about adjusting the other edit positions because they are unaffected. So, basically, you can think of the sorting as only a nice / easy / efficient way to implement applying a Multi-change.

@arxanas
Copy link
Contributor Author

arxanas commented Sep 8, 2017

This means that if you have changes in the beginning of the document then, if you apply them first, then you have to adjust all the positions of the other edits,

@kdvolder: I think that's not what @dbaeumer is saying, although this was my initial assumption. I believe the clarification of the spec is that you simply apply them in the order that they're received, without adjusting any positions, and you get the right results.

@kdvolder
Copy link

kdvolder commented Sep 8, 2017

@arxanas I think you are right. For a document change event, the changes in the array are to be applied in order, and if position shift around because of that, I'd assume the client doesn't have worry about that because the next change is meant to be applied to the result of the previous change. But... the order matters. I would also assume that changes can be overlapping in this case.

What I was talking about was this:

https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#new-textdocumentedit

The similarity between document edits and document change events had gotten me all confused.

Change events are sequential and have a definite ordering to them. Whereas DocumentEdits on the other hand are non-overlapping and their order shouldn't affect the semantics.

@alanz
Copy link

alanz commented Sep 8, 2017

Well, if the "adapted in order" interpretation is correct than I can revert emacs-lsp/lsp-mode@3d230b7 and go back to batching up changes for emacs.

@rcjsuen
Copy link
Contributor

rcjsuen commented Sep 8, 2017

I renamed a variable in VS Code and I received the following two changes. Applying them in order is certainly fine given that the first modification is on line 10 and the second is on line 9 so no offsets have to be recalculated here upon application of the first change.

Seems like I revert my language server's sorting code and open bugs against LSP clients that aren't sending changes in this order.

[
  {
    "range": {
      "start": {
        "line": 10,
        "character": 1
      },
      "end": {
        "line": 10,
        "character": 4
      }
    },
    "rangeLength": 3,
    "text": "var2"
  },
  {
    "range": {
      "start": {
        "line": 9,
        "character": 1
      },
      "end": {
        "line": 9,
        "character": 4
      }
    },
    "rangeLength": 3,
    "text": "var2"
  }
]

@dbaeumer
Copy link
Member

@kdvolder your final conclusion is correct. If you need further clarification please let me know.

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.

6 participants