Notebook diff not compatible with readonly files from TextDocumentContentProvider - only checks IFileService for a IFileSystemProvider

[rehmsen]

Does this issue occur when all extensions are disabled?: Yes/No

• VS Code Version: vscode.dev
• OS Version: macOS Sonoma 14.4.1 (23E224)

Steps to Reproduce:

1. Have an extension that registers a TextDocumentContentProvider with a different "scheme", like "my-scheme://".
2. Try to open a notebook diff where the left side is served via "my-scheme://".

Expected:

The notebook diff can be opened with a left hand side provided via a TextDocumentContentProvider.

Actual:

It fails in this line, because it requires the fileService to have aIFileSystemProvider for it:

vscode/src/vs/workbench/contrib/notebook/browser/services/notebookServiceImpl.ts

Line 176 in a1d4ad1

canSupportResource: (resource: URI) => resource.scheme === Schemas.untitled || resource.scheme === Schemas.vscodeNotebookCell || this._fileService.hasProvider(resource)

Preliminary investigation and assumptions:

My (possibly incorrect) understanding of the provided abstractions and concepts is:

• TextDocumentContentProvider is for readonly documents (such as some historic version in a VCS):

  vscode/src/vscode-dts/vscode.d.ts

  Lines 1798 to 1806 in a1d4ad1

  	* A text document content provider allows to add readonly documents
  	* to the editor, such as source from a dll or generated html from md.
  	*
  	* Content providers are {@link workspace.registerTextDocumentContentProvider registered}
  	* for a {@link Uri.scheme uri-scheme}. When a uri with that scheme is to
  	* be {@link workspace.openTextDocument loaded} the content provider is
  	* asked.
  	*/
  	export interface TextDocumentContentProvider {

• FileSystemProvider is for file systems that allow reading and writing:

  vscode/src/vscode-dts/vscode.d.ts

  Lines 8856 to 8867 in a1d4ad1

  	* The filesystem provider defines what the editor needs to read, write, discover,
  	* and to manage files and folders. It allows extensions to serve files from remote places,
  	* like ftp-servers, and to seamlessly integrate those into the editor.
  	*
  	* * *Note 1:* The filesystem provider API works with {@link Uri uris} and assumes hierarchical
  	* paths, e.g. `foo:/my/path` is a child of `foo:/my/` and a parent of `foo:/my/path/deeper`.
  	* * *Note 2:* There is an activation event `onFileSystem:<scheme>` that fires when a file
  	* or folder is being accessed.
  	* * *Note 3:* The word 'file' is often used to denote all {@link FileType kinds} of files, e.g.
  	* folders, symbolic links, and regular files.
  	*/
  	export interface FileSystemProvider {

• The left hand side of a diff cannot be edited - this should be true both for regular and notebook diffs.

The TextDocumentContentProviders seem to be registered with the ITextModelService - I think Notebook diff should possibly check that instead of the FileService to see if the "my-scheme://" is supported? This check seems to have been added here: #122627 (comment)
#122658

Maybe @jrieken can advise on how to get the notebook diff viewer to work with readonly files from TextDocumentContentProvider schemes?

[rehmsen]

I tried to dig a bit to better understand how to make the notebook diff work with a TextDocumentContentProvider instead of the FileService, but there are a lot of abstractions I do not understand. In regular diff, non-file scheme URIs seem to be loaded using a TextResourceEditorInput as opposed to a FileEditorInput. For notebook diffs, the callgraph is roughly
NotebookEditorInput -> NotebookModelResolverServiceImpl -> NotebookModelReferenceCollection -> SimpleNotebookEditorModel -> StoredFileWorkingCopyManager -> StroedFileWorkingCopy -> FileService

Somewhere between those one would need to add support for TextDocumentContentProvider provided text, but as I don't understand what all these abstractions are for, it is difficult for me to pinpoint where exactly. Just from the names, I would say that the left hand side of a diff is not necessarily a "working copy" (it may be read only original version), so I am guessing that SimpleNotebookEditorModel should maybe have a way to branch out to the TextDocumentContentProvider provided document, but that is a wild guess.

Can someone point me in the right direction? I am happy to make a contribution, but might need some help figuring out how these things are meant to work together.

[a-stewart]

Hi,

I've been investigating this a bit and the interesting part is the FileWorkingCopyManager. As best as I could tell it is something that is only in use by Notebooks and provides an alternate approach for how to read files.

There is some context in #117873 and the initial implementation is in #119233.

As far as I understand, the desire was to have a basic underlying implementation to abstract file reading so that multiple different types of editor wouldn't need to deal with the low level logic. However, at the moment only Notebooks make use of this.

The problem is with the TextDocumentContentProvider API. The FileWorkingCopyManager handles "stored files" and "untitles files", with stored files being read write files on disk, and with untitled files being scratch buffers and files that are yet to be saved.

As far as I can see there are two things that need to happen in order for files provided by TextDocumentContentProvider to be supported in notebooks.

1. FileWorkingCopyManager needs to be aware of registered TextDocumentContentProviders.
2. FileWorkingCopyManager needs to have a third type of file that it can consider, which are readonly files with no "stored" backing.

The approach I am looking into for the second part is to to just create a sibling of IStoredFileWorkingCopyManager and IUntitledFileWorkingCopyManager to handle files provided by TextDocumentContentProviders. (Would appreciate suggestions with regards to what this should be called.)

For the first part there are two options, one would be to load in the ITextModelService and use the file contents are provided by that class. This feels a little bit odd to be wiring it into here, but this is currently the source of truth for documents provided by TextDocumentContentProvider.

Another option, would be to duplicate the calls to register a TextDocumentContentProvider, and to register with both ITextModelService (used by TextEditors) and FileWorkingCopyManager (used by Notebooks), but having 2 sources of truth for this feels wrong.

A third option would be to extract the handling of TextDocumentContentProvider into another class, and have that wiring into both the classes. This would probably be the cleanest approach, but would be a fairly large refactor, and would make the PR larger than either of the other two options.

I'm happy to work on trying to resolve this setup, and would be interested to know what your preferred approach would be, and if you would be willing to accept a PR for this.

[a-stewart]

I've had a bit of a play with this and I have a working proof of concept which adds support for the TextDocumentConentProvider API in Notebooks: #212697.

I'd be interested in discussing this, and seeing what you think of this approach so that we can work towards creating a PR to resolve this.

[a-stewart]

@jrieken - quick ping on this.

We'd like to work on creating a solution for this, and were wondering if you could take a look at the approach in the POC in #212697 and let us know if that seems like the best approach.

Happy to look into other approaches if you think that another way would be better.

[jrieken]

Sorry, this isn't really my domain but from what I know it is wrong to use the "TextDocument"ContentProvider for anything notebook related. The latter works with serializers and isn't text-based. This makes the marriage of both worlds sound wrong. I understand the appeal in using the simple text document content provider for this but having a simple, readonly-only file system provider is also fairly simple and that would power notebooks the way it is intened

[a-stewart]

This makes sense from a holistic view point, but the assumption is that the extension using TextDocumentContentProvider is the same extension as the one handling the notebooks.

In our case, the TextDocumentContentProvider is a source control extension, providing a view of the file at an older revision, which is used for creating diffs of a file.

We also have a notebook extension which is completely unrelated and developed by a different team.

It seems to be mixing these concerns? Why should the Notebook extension stop working based on how a completely unrelated source control extension offers file contents?

Or why should the source control extension have to go for a more complex approach to serving files on the chance that a user may also have an extension installed that uses notebooks.

And if we have this limitation with the TextDocumentContentProvider - should we document this telling authors of source control extensions or other extensions that using it will prevent the content from rendering in a notebook?

[jrieken]

Or why should the source control extension have to go for a more complex approach to serving files on the chance that a user may also have an extension installed that uses notebooks.

Yes, it should do that in order to support other, non text document file types like notebooks or images

And if we have this limitation with the TextDocumentContentProvider - should we document this telling authors of source control extensions or other extensions that using it will prevent the content from rendering in a notebook?

Yeah, it seems we should improve the doc. I was under the assumption that its name and its signature makes this clear but I open for suggestions on how to underline this stronger