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

Lookup insertion results sends 'rangeLength' ? #341

Open
payne911 opened this issue Oct 18, 2023 · 3 comments
Open

Lookup insertion results sends 'rangeLength' ? #341

payne911 opened this issue Oct 18, 2023 · 3 comments
Labels

Comments

@payne911
Copy link
Contributor

Typing System. and selecting out as the option to insert from the code completion lookup that appears in the IDE results in this being sent:

Notification {
  method: "textDocument/didChange",
  params: Object {
    "contentChanges": Array [
      Object {
        "range": Object {
          "end": Object {
            "character": Number(11),
            "line": Number(6)
          },
          "start": Object {
            "character": Number(11),
            "line": Number(6)
          }
        },
        "rangeLength": Number(3),
        "text": String("out")
      }
    ]
  }
}

It seems like rangeLength should not be specified given this is an insertion rather than a replacement (since range.start == range.end).

@nixel2007
Copy link
Contributor

rangeLength is deprecated and can be ommited at all

@payne911
Copy link
Contributor Author

payne911 commented Oct 20, 2023

In any case, I think something is wrong (source):

/**
 * An event describing a change to a text document. If only a text is provided
 * it is considered to be the full content of the document.
 */
export type [TextDocumentContentChangeEvent](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocumentContentChangeEvent) = {
	/**
	 * The range of the document that changed.
	 */
	range: [Range](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#range);

	/**
	 * The optional length of the range that got replaced.
	 *
	 * @deprecated use range instead.
	 */
	rangeLength?: [uinteger](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#uinteger);

	/**
	 * The new text for the provided range.
	 */
	text: string;
} | {
	/**
	 * The new text of the whole document.
	 */
	text: string;
};

Unless I'm misunderstanding what rangeLength is for, it seems like it is currently being used by lsp4intellij to designate the text.length, but the documentation says:

The optional length of the range that got replaced.

Which given it was an insertion (rather than a replacement), sounds like the value should have been 0 instead of 3 (if we look into the example I provided in the original message).

@payne911
Copy link
Contributor Author

Moreover, are we certain that we shouldn't use documentChanged only for FullSync, and that beforeDocumentChanged is the one that should be used for IncrementalSync (since I believe the values should be calculated based on the "old" document version's offset values) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants