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

Add normalizing notebook documents service #1743

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

msujew
Copy link
Member

@msujew msujew commented Nov 6, 2024

Closes #1496.

This is a continuation of #1660. Since the NotebookDocuments class exported by vscode-languageserver is quite specific in what it accepts as a TextDocuments instance (only instances of the class exported by vscode-languageserver), adopters of Langium, can no longer plug Langium's TextDocuments service in there.

However, the NotebookDocuments implementation of vscode-languageserver had the same URI encoding issues as TextDocuments anyway, and so we need to roll our own. This new NormalizingNotebookDocuments is now a default service of the shared LSP services and can be accessed by all Langium adopters.

@msujew msujew added polish Some feature needs improvement LSP Language Server Protocol integration labels Nov 6, 2024
@cdietrich
Copy link
Contributor

@msujew please let me know when we have an alpha for this. would like to test.

@msujew msujew force-pushed the msujew/notebook-documents branch from 3838304 to 1409e5d Compare December 11, 2024 13:58
@msujew
Copy link
Member Author

msujew commented Dec 11, 2024

@cdietrich I got around to publish 3.4.0-next.1409e5d which contains this change.

@cdietrich
Copy link
Contributor

thx will give it a test

@cdietrich
Copy link
Contributor

hmmm its not working. need to debug ....

@cdietrich
Copy link
Contributor

for some reason the notebook documents dont end up in text documents

@cdietrich
Copy link
Contributor

we do this in our service registry
const textDocument = this.sharedServices.workspace.TextDocuments.get(uri.toString())

but

this.sharedServices.workspace.TextDocuments.all()

is empty

@msujew
Copy link
Member Author

msujew commented Dec 11, 2024

@cdietrich Thanks for looking into it. I'll take another look to see what's wrong. I probably missed something when adapting the Microsoft implementation.

@cdietrich
Copy link
Contributor

Then I ask for cell and document by cell I get one step more
But then services crash with document already present

need to figure out why

@cdietrich
Copy link
Contributor

Maybe there is still to much double escaping somewhere

@cdietrich
Copy link
Contributor

hmmm no. when i use

  let languageId: string | undefined = undefined
      const cell = this.sharedServices.workspace.NotebookDocuments.getNotebookCell(uri.toString())
      if (cell !== undefined) {
        languageId = this.sharedServices.workspace.NotebookDocuments.getCellTextDocument(cell)?.languageId
      } else {
        console.log("nooooo ceeeellll")
      }

in the service registry the cell map is still empty in the early stage (guard in fireDocumentUpdate)

@cdietrich
Copy link
Contributor

cdietrich commented Dec 11, 2024

hmmm somehow not all docs end up in
_syncedDocuments

this is cause vscode sends only 4 of 9 cells. need to digg why

update: found the reason for it. now if still end up with failing services / no registered service found.
need to check ..

@cdietrich
Copy link
Contributor

upon further debugging
the this.sharedServices.workspace.TextDocuments
indeed does not get populated upon open

@cdietrich
Copy link
Contributor

looks like i somehow end up with multiple NormalizedTextDocs. need to figure out why.

@cdietrich
Copy link
Contributor

cdietrich commented Dec 11, 2024

hmmmm
NotebookDocuments: () => new NormalizedNotebookDocuments(TextDocument)

should this be

NotebookDocuments: (services) => new NormalizedNotebookDocuments(services.workspace.TextDocuments)

@cdietrich
Copy link
Contributor

cdietrich commented Dec 11, 2024

looks better now. we will do some thorough testing tomorrow

@msujew msujew force-pushed the msujew/notebook-documents branch from 325db9e to 52183ea Compare December 12, 2024 00:19
@cdietrich
Copy link
Contributor

All tests from our side look good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LSP Language Server Protocol integration polish Some feature needs improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support notebook document messages
2 participants