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

The LSP client may send document content with non-LF line endings, normalize before comparing with NB document content. #6690

Merged

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Nov 13, 2023

When a file is opened, the didOpen called in the LSP server is called. The LSP client (tested with Visual Studio Code) sends the file text/content with their real line endings, which may be \r\n. Internally, the NB server only uses \n. In the didOpen callback, when the server compares the text from the client and the internal document, the contents will be different, and the server will re-set the Document's content to the received file content.

Such Document is internally marked as modified, which may cause problems or accentuate other problems (see for example oracle/javavscode#51 or oracle/javavscode#26).

In this patch, I've tried to fix this, by normalizing the line endings before the comparison. I've tried to make this in a way that files with '\n' don't pay additional cost for line endings normalization in the common cases (i.e. when the file content as sent from the client, and the content of the document match).

…rmalize before comparing with NB document content.
@lahodaj lahodaj added the LSP [ci] enable Language Server Protocol tests label Nov 13, 2023
@lahodaj lahodaj added this to the NB21 milestone Nov 13, 2023
@lahodaj lahodaj requested a review from sdedic November 13, 2023 11:12
@lahodaj
Copy link
Contributor Author

lahodaj commented Nov 16, 2023

Thanks for the review!

@lahodaj lahodaj merged commit a92b79d into apache:master Nov 16, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LSP [ci] enable Language Server Protocol tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants