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

API gaps to support Liveshare in Notebook #102503

Closed
rebornix opened this issue Jul 14, 2020 · 27 comments
Closed

API gaps to support Liveshare in Notebook #102503

rebornix opened this issue Jul 14, 2020 · 27 comments
Assignees
Labels
feature-request Request for new features or functionality notebook-api
Milestone

Comments

@rebornix
Copy link
Member

Currently the editing API for Notebook is slim and may not support all features of Live Share.

@rebornix rebornix added feature-request Request for new features or functionality notebook labels Jul 14, 2020
@rebornix rebornix added this to the July 2020 milestone Jul 14, 2020
@rebornix rebornix self-assigned this Jul 14, 2020
@rebornix
Copy link
Member Author

rebornix commented Jul 30, 2020

Below is a draft list of missing API/features to support co-editing in notebook after analyzing how LiveShare uses text editor APIs.

  • Editor
    • 🏃 Visible notebook editors & change event(together with visible text editors) (vscode.window.visibleTextEditors, vscode.window.onDidChangeVisibleTextEditors)
    • Notebook editor selection & text editor selection in cell editor (vscode.window.onDidChangeTextEditorSelection). Track the selection changes in a document (E.g. highlights, cursor position)
    • Active notebook editor change (vscode.window.onDidChangeActiveTextEditor). Handle changes in currently active editor and sync with other participants.
    • Editor visible range change (vscode.window.onDidChangeTextEditorVisibleRanges). Update layout info if other participants are following current user.
      • visible range of viewport
      • reveal range
  • Document
    • 🏃 Notebook document changes event (vscode.workspace.onDidChangeTextDocument). Collect & process actual edits to a document
      • Need to figure out what's the best practice for listening to both notebook changes and cell document changes
    • Notebook document, open, close event (vscode.workspace.onDidCloseTextDocument, vscode.workspace.onDidOpenTextDocument). Clean up states.
    • Notebook document save event (vscode.workspace.onDidSaveTextDocument)
      • Saves need to be propagated through the session, so other clients can also save when indicated (guests aren't really saving, for example)
  • Navigation
    • Open/jump to an editor (vscode.window.showTextDocument).
    • Open/jump to a diff editor (vscode.diff)
    • Unfold recursively to top level on a position
  • Decorations
    • Show user card/info around the editor/cell selection
  • Undo/Redo
    • Notebook document change events + cell document change events
    • 🏃 Hook up with editor undo/redo command (undo, redo)
  • MISC
    • Untitled notebook document (TextDocument.isUntitled)

cc @lostintangent

@rebornix rebornix pinned this issue Jul 30, 2020
@lostintangent
Copy link
Member

lostintangent commented Jul 31, 2020

@rebornix Would there be work left for Live Share to do after you make the above changes? Or would notebooks magically "look" like a series of documents to Live Share? 😄 This might be a silly question, I just wanted to check, so we could plan ahead 👍

@rebornix
Copy link
Member Author

@lostintangent Since the notebook editor/documents are new concepts, it won't work out of the box after we make above changes. Live Share at least needs to

  • Co-editing. It may need to leverage fluid to handle the conflict resolution of notebooks as the notebook documents are structured objects other than plain text.
  • Navigation. It needs to understand the difference between TextEditor/TextDocument & NotebookEditor/Document.

@rebornix
Copy link
Member Author

rebornix commented Aug 28, 2020

Examples of how the notebook APIs can be used together with Text Editor APIs for Live Share

Active Editor change

vscode.window.onDidChangeActiveTextEditor((e) => {})
vscode.notebook.onDidChangeActiveNotebookEditor((e) => {})

Document lifecycle

vscode.workspace.onDidCloseTextDocument
vscode.workspace.onDidOpenTextDocument
vscode.workspace.onDidSaveTextDocument

vscode.notebook.onDidOpenNotebookDocument
vscode.notebook.onDidCloseNotebookDocument
vscode.notebook.onDidSaveNotebookDocument

Content Changes of a notebook document

vscode.workspace.onDidChangeTextDocument((e) => {
  const document = e.document;
  const notebookDocument = document.notebook;
});

vscode.notebook.onDidChangeNotebookCells(e => {
  const notebookDocument = e.document;
});
vscode.notebook.onDidChangeCellOutputs
vscode.notebook.onDidChangeCellLanguage
vscode.notebook.onDidChangeCellMetadata

Participant selection change

vscode.window.onDidChangeTextEditorSelection(e => {
  const selections = e.selections;
});

vscode.notebook.onDidChangeNotebookEditorSelection(e => {
  const selection = e.selection; // a notebook cell
});

Unfold recursively from an index in the notebook document to the top

vscode.commands.executeCommand('notebook.unfold',  {
      "index": 1,
      "direction": "up",
      "levels": 65535
});

Cell Editing

Decorations

  • Decorations for showing participant user name
  • Decorations for showing participant selection

Viewport information

// events
vscode.notebook.onDidChangeNotebookEditorVisibleRanges
// visible ranges info
NotebookEditor.visibleRanges

// reveal a range
notebookEditor.revealRange({ start: 10, end: 10}, vscode.NotebookRevealType.InCenter);

jrieken added a commit that referenced this issue Aug 28, 2020
rebornix added a commit that referenced this issue Aug 28, 2020
@rebornix rebornix modified the milestones: August 2020, September 2020 Aug 31, 2020
@lostintangent
Copy link
Member

@rebornix This is looking awesome! Tagging @daytonellwanger as an FYI, since we'll be looking into adding this Live Share support soon 👍

@rebornix
Copy link
Member Author

rebornix commented Sep 16, 2020

Had offline discussion with @daytonellwanger and found more API limitations:

  • Mock content provider on guest side
    • Currently to register a content provider, the extension needs to register static contribution point inpackage.json first, which will specify file extensions it supports. However when a notebook document is opened in the host side, it's possible that there is no such notebook content provider in the guest side. Also Live Share extension can't register static contribution in advance as the file name patterns are not known yet
    • Figure out how to register a content provider only at runtime
    • Figure out how to register an exclusive content provider

Code snippets will be tracked in https://github.com/microsoft/notebook-extension-samples/tree/master/notebook-vsls

@rebornix
Copy link
Member Author

rebornix commented Sep 17, 2020

While attempting to support exclusive content provider registration at runtime, I found more open questions than listed above. Thus we took one step back and re-evaluate what's the best strategy to support notebook editing in Live Share environment:

  • Notebook content providers work as Live Share companion extensions
    • Content provider extensions should be installed on both host and guest sides, otherwise Live Share won't work.
    • When a notebook is opened on host, LS will open the same document on guest with the same view type if it exists.
    • LS needs to contribute a mock kernel, which redirects the execution/cancel requests to the host
      • When there are kernel providers for the notebook, the mock kernel might need to be exclusive.
      • In some scenarios, guests may still want to use their local kernel. For example, in GitHub Issue Notebook, users want to run queries with their identity, otherwise @me is wrong.
  • Notebook content providers are mirrored on guest side
    • Content provider extensions are only required on host side.
    • When a notebook is opened on host, LS will
      • register a mirror content provider on guest
        • the content provider needs to be registered dynamically (no static contribution)
        • LS needs to know the viewType, displayName, selector and transientOptions of the content provider
        • LS needs to register mirror content providers if there are opened mirror notebook documents in the workspace (for example, when reloading the workspace on guest side)
        • Again, if users switch the editor to another view type, Live Share won't work, unless we disable view type switching
      • open the document with new mirror view type
    • 💚 LS will register a kernel for the mirror content provider on the guest side, and then redirect the execution requests to the host

@lostintangent
Copy link
Member

@rebornix 🙌 If possible, it would be awesome to enable option #2 above. Since VS Code already provides the UI/core functionality of notebooks, being able to leverage kernels from the host would enable the setup-free guest experience we already have with language services, task providers, etc.

Where are you and @daytonellwanger leaning?

@rebornix
Copy link
Member Author

@lostintangent thanks for your feedback, we are leaning towards option 2 as well (majorly for the same reason you mentioned above). option 1 is listed there to ensure us not miss any interesting scenarios and unveil all possible challenges.

@rebornix
Copy link
Member Author

rebornix commented Nov 17, 2020

@daytonellwanger already pushed the initial notebook support in Live Share and while doing that, we figured out the LS extension may need more info from VS Code

  • VS Code API equivalent of vscode.workspace.openTextDocument for notebook documents to avoid our own implementation
    • We added vscode.notebook.openNotebookDocument.
  • VS Code API equivalent of vscode.window.showTextDocument for notebook cells
  • 🐛 Decorations being cut off for notebook cells Overflowing decorations #110769
  • API for getting notebook contents on host without opening notebook on host

@daytonellwanger
Copy link

what's the expected behavior of the equivalent API?

It should focus the notebook in the specified column and ideally scroll to the position of the specified cell.

can vscode.notebook.openNotebookDocument solve this problem?

vscode.notebook.openNotebookDocument will actually open the notebook, right? This is an ask for getting the contents without actually opening the notebook, although it's lower priority because I don't think this is possible with regular text documents today either.

@rebornix
Copy link
Member Author

vscode.notebook.openNotebookDocument will actually open the notebook, right? This is an ask for getting the contents without actually opening the notebook, although it's lower priority because I don't think this is possible with regular text documents today either.

@daytonellwanger vscode.notebook.openNotebookDocument behaves like vscode.notebook,openTextDocument, which will open the document in the background (no editor attached) and then you can access cell and metadata info on the notebook document.

@daytonellwanger
Copy link

@rebornix perfect! I'll give it a try.

@rebornix
Copy link
Member Author

VS Code API equivalent of vscode.window.showTextDocument for notebook cells

@daytonellwanger I just added vscode.window.showNotebookDocument which can open a notebook and reveal/focus a cell position

const editor = await vscode.window.showNotebookDocument(document, { viewColumn: vscode.ViewColumn.Beside, selection: { start: 10, end: 11 } });

@rebornix
Copy link
Member Author

Decorations being cut off for notebook cells #110769

This one is really challenging to fix completely so instead we pushed a workaround, if there is a decoration which has top property set, we will adjust the cell editor's top padding to at least lineHeight+2. So for editors in notebook, please always render the name tag above the cursor instead of showing it below the line when cursor is on the first line.

Once the showNotebookDocument is shipped with Insiders, I think we don't have any blocking API for your next step @daytonellwanger

@lostintangent
Copy link
Member

lostintangent commented Nov 29, 2020

@daytonellwanger Does your list of asks enable us to implement complete "follow" support for notebooks? In addition to being able to open a notebook to a specific scroll position and cell, are we also able to track/synchronize subsequent cell navigations and notebook scrolls? The answer is probably mentioned above, but I just wanted to double-check 👍

@rebornix
Copy link
Member Author

In order to allow the Live Share extension to create mirror kernels on guest side, we introduced two more API commands

CommandsRegistry.registerCommand('_resolveNotebookKernelProviders', async (accessor, args): Promise<{
	extensionId: string;
	description?: string;
	selector: INotebookDocumentFilter;
}[]>

CommandsRegistry.registerCommand('_resolveNotebookKernels', async (accessor, args: {
	viewType: string;
	uri: UriComponents;
}): Promise<{
	id?: string;
	label: string;
	description?: string;
	detail?: string;
	isPreferred?: boolean;
	preloads?: URI[];
}[]>

@daytonellwanger
Copy link

If a file has multiple content providers available, the user can choose which content provider to use. It would be an awkward experience to lock the host and guest into using the same content provider. A couple examples:

  • The host has the notebook open with Content Provider A while the guest isn't following them. The guest later opens the notebook independently and chooses to use Content Provider B. The guest doesn't know the host has the notebook open with Content Provider A and is forced into using Content Provider A.
  • The guest is viewing a document with Content Provider A. The host switches to using Content Provider B. The guest, in the middle of their viewing, is switched to Content Provider B.

I believe the correct UX is for host and guest to be allowed to independently select a content provider (note that if they're using different content providers, coediting will not be supported).

Today the vscode.notebook.openNotebookDocument API will only open the notebook document with the content provider the host has already selected. It will need to be updated to support opening the notebook with a different content provider even if the notebook is already open. Note that we don't necessarily need to support having the notebook open in an editor. We just need the vscode.notebook.openNotebookDocument API to support opening the notebook with multiple content providers.

If different participants are using different content providers, their changes will be synced when the content provider persists them. This presents no difficulties if the file is clean for all other participants - we simply update their contents. However, if Participant A has a file open with Content Provider A, and has made changes but not persisted them, and Participant B has the same file open with Content Provider B and just persisted changes, this should be treated the same as a file being updated on disk when open with dirty changes - when Participant A attempts to persist the file, they will receive "The content of the file is newer. Please compare your version with the file contents or overwrite the content of the file with your changes."

@jrieken
Copy link
Member

jrieken commented Jan 13, 2021

It will need to be updated to support opening the notebook with a different content provider even if the notebook is already open.

That will be VERY complex as internally everything is mapped by its uri and it means we need to invent some uri-format for which combines the actual uri and provider type. We could try to use a query-string for that but I have doubts that this will be easy...

@daytonellwanger
Copy link

@jrieken is there another alternative to avoid the awkward situations described above? I imagine the user will be quite confused if they go to open the notebook and explicitly select a content provider and then we open it with a different one. Or if the content provider is changed out from under them in the middle of a session.

I suppose we could show a notification to the user anytime we encounter one of these to give some explanation, but it still feels like a bad experience.

@jrieken
Copy link
Member

jrieken commented Jan 13, 2021

I don't know if there is an alternative. The challenge is that there is one file on disk (one sequence of bytes) which is interpreted different but always referred to by the same name. So, it is good that this is on the table. It needs some thinking and code analysis to understand the full impact here.

@daytonellwanger
Copy link

Thanks for the info. For Live Share's v1 then, we'll move forward with the constraint that the notebook can only be open by a single content provider at a time. For most scenarios this should be fine, as I imagine it's rare that host and guest would want to use different content providers. However, if it were simple to support multiple content providers, or there were other use cases that required it, that would of course be preferred for the Live Share scenario 😊.

@rebornix
Copy link
Member Author

rebornix commented Jan 14, 2021

Thanks @jrieken for bringing up the potential issues of multiple content providers, I did some initial analysis of how we use the Uri. Since we have the restriction in place, when we designed the Uri for notebook document and notebook cell, they don't take view type into account. It made the Uri simple and compatible with existing workbench features.

The notebook document Uri is the resource Uri, untouched. Users can open a resource in both text editor and notebook editor, since they are different editor types (users can also open the resource in a custom editor), we can always identify them. A typical notebook document uri might be like file:///C:\Users\rebor\code\vscode\.vscode\notebooks\api.github-issues

The notebook cell uri is based on the notebook uri, with one more information cell handle. Thus the notebook cell Uri will carry following info

  • notebookUri#path
  • notebookUri#scheme
  • cell handle

The CellUri will be used in following places to identify which notebook document it belongs to:

  • UndoRedoComparisonKey. notebookUri.toString(), e.g., file:///C:\Users\rebor\code\vscode\src\vs\workbench\contrib\notebook\common\notebookCommon.ts
  • Output renderer, which will be used to find the notebook document in extension host
  • Editor Open, this.editorService.findEditors(notebookUri, group).filter(editor => !(editor instanceof NotebookEditorInput)
  • CellContentProvider
  • Diff Editor check whether a cell is in original or modified editor
  • MarkerListProvider
  • NotebookCellStatusBar

The other significant way of using notebookUri is through ResourceMap (key is notebookUri.toString()), we use them a lot to track notebook documents

  • NotebookService._models
  • NotebookEditorWidgetService._notebookWidgets
  • ExtHostNotebook._documents
  • ExtHostNotebook._documentEventListenersMapping

Last but not least, WorkspaceEdit for notebooks are all talking about notebookUri only

export interface WorkspaceEdit {
	replaceNotebookMetadata(uri: Uri, value: NotebookDocumentMetadata): void;
	replaceNotebookCells(uri: Uri, start: number, end: number, cells: NotebookCellData[], metadata?: WorkspaceEditEntryMetadata): void;
	replaceNotebookCellOutput(uri: Uri, index: number, outputs: (NotebookCellOutput | CellOutput)[], metadata?: WorkspaceEditEntryMetadata): void;
	replaceNotebookCellMetadata(uri: Uri, index: number, cellMetadata: NotebookCellMetadata, metadata?: WorkspaceEditEntryMetadata): void;
}

A short summary of above is to support multi content providers, we can no longer use Uri as the identifier for a notebook resource, instead we need to use a tuple (uri, viewType) everywhere.

Considering it's not just a few hundreds lines of changes, we may want to stick with what we currently have for v1. We can probably improve the error notification, e.g., add a button to save+close old notebook and then open the document in the new view type.

@alyssajotice
Copy link

Before migrating to serializers, Live Share needs a way to get a list of the available serializers, similar to the command vscode.resolveNotebookContentProviders. We also need a way to add viewOptions when registering a serializer so that we can add a fileNamePattern for the serializer.

@rebornix
Copy link
Member Author

rebornix commented Dec 8, 2022

Closing this now as @mjbvz is working with Live Share on the API update.

@rebornix rebornix closed this as completed Dec 8, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality notebook-api
Projects
None yet
Development

No branches or pull requests

6 participants