-
Notifications
You must be signed in to change notification settings - Fork 28.6k
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
New notebook from the command line issues #127757
Conversation
src/vs/workbench/contrib/notebook/browser/notebookServiceImpl.ts
Outdated
Show resolved
Hide resolved
Idk - It's the first time I hear about I think this needs further clarification. Let's reserve some 1-1 time tomorrow or next week |
@jrieken the factory was added only recently (last week?) to distinguish normal editors from untitled editors (and diff editors have their own factory too), but it doesn't really matter and is just there for convenience. The underlying contract is that you get a lightweight "untyped" JSON editor input and have to return a heavy "typed" editor input that is then opened in an editor. There are only a few untyped editors:
The URI of the editor input does not have to be known up front, in fact it is even optional for editor inputs so notebooks are free to do whatever they want, even if that means to resolve the URI only in the 1on1 sounds good, we can do next week since this probably is not making it for June anyway. |
Btw I like the idea of having this logic somewhere here: vscode/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts Line 155 in 9bb711d
It would however require that for the vscode/src/vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl.ts Line 183 in 9bb711d
We can pass on additional context to pipe through the untitled with associated file path case. Could be just a boolean too. |
Continues in #130236, I can walk you through when you are back. |
This is for #125028 and not the full fix yet but I want to get some feedback if the direction makes sense.
The challenge is that we seem to have some logic how to create a untitled notebook URI in
mainThreadNotebookDocuments.ts
but we also need it for the untitled editor factory where we have to create a notebook editor:vscode/src/vs/workbench/contrib/notebook/browser/notebookServiceImpl.ts
Line 171 in d3613d0
This method is responsible for converting a simple JSON object ("untyped editor input") to a real untitled notebook editor input. With this PR, the behaviour of both are aligned with regards to:
possibleFileEnding
usage (unless a resource is associated)query: notebookProviderInfo.id
)One missing aspect is how to forward the information down to where the working copy is being created that we have an untitled resource with associated file path. Basically this code:
vscode/src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts
Line 519 in ca4bf8a
Needs to change in a way that it continues to call
resolve({ untitledResource: this.resource })
for normal untitled editors andresolve({ associatedResource: this.resource })
for untitled with associated path. This distinction makes it possible to get the right behaviour which is:One idea I had was to also encode this info into the URI as query param, but I am not sure where this resource is travelling and if that is a safe thing to do.
Thoughts?