Support index based url for notebook cells

[rebornix]

Provide a url for cell locations like https://github.com/owner/repo/blob/master/notebook.ipynb#C2:L6-10

[greazer]

cc: @rchiodo

[rebornix]

Dig into this a bit and find that to make this happen, it would require following changes:

• GitHub Repositories: support url fragment like C2:L12-L23 in addition to L12-L23 and convert them into path in syntax :2_12-23
• RemoteHub: parse the file path :2_12-23 and convert it to { cellIndex, start, end }
• VS Code supports selecting a cell with editaor selection. This means we would need to update showNotebookDocument options.

[rebornix]

Discussed offline with @joyceerhl , with above items we can make the cell index url work in VS Code desktop with remote hub. To support the url in github/vscode.dev:

• the embedders need to support this syntax
• the embed api IWorkbenchConstructionOptions.defaultLayout should support notebook cell index

[rebornix]

Above diagrams show what changes we might need to make to support this style of link

• vscode/github.dev needs to support parsing the link
  • web.api for embedders now only support selection text editor options, we want to extend it to support others
• github/azurerepo extensions need to support the fragment in their link/repo opener
  • remotehub supports parsing it too (it's using a different but generic syntax)
  • vscode.open command only supports text editor options. we either extend it, or remotehub uses showNotebookDocument api with cell index + cell editor selection support explicitly
• editorService.openEditor to support UntypedEditorInput to support generic editor options, other than just TextEditorOptions.

[rebornix]

Had offline discussions with @lramos15 and @sbatten on proposed changes to have the notebook cell index support in Web scenarios (vscode/github.dev). Here is the gist of our discussions and some of our proposals.

vscode/github.dev are constructing the workbench based on the info from the url (parse the path and fragments) and providing the default views and editors to open to VS Code. The workbench construction info is defined as

interface IWorkbenchConstructionOptions {
	readonly defaultLayout?: IDefaultLayout;
...
}
interface IDefaultLayout {
	readonly views?: IDefaultView[];
	readonly editors?: IDefaultEditor[];
...
}
interface IDefaultEditor {
	readonly uri: UriComponents;
	readonly selection?: IRange;
	readonly openOnlyIfExists?: boolean;
	readonly openWith?: string;
}

IDefaultEditor is a combination of resource uri, selection and openWith. Internally we will construct ITextEditorOptions with selection and openWith (converted to overrideId). Challenge here is ITextEditorOptions is not compatible with Notebook Editors. We talked about 3 options of extending the IDefaultEditor to support notebook editor options

First one is extending openWith. openWith is used to allow resources to be opened in a custom/notebook editor. It already has the knowledge of custom* editor so this proposal extends it to carry editor options for the custom editor

export interface IDefaultEditor {
	readonly uri: UriComponents;
	readonly selection?: IRange;
	readonly openOnlyIfExists?: boolean;
	readonly openWith?: string | { id: string; options?: Record<string, any> };
}

Second option is replacing selection with a real IEditorOptions, instead of constructing editor options from selection later on. For text editor, the options can be as simple as { selection, pin } and for notebooks it would be { cellIndex, selection }.

export interface IDefaultEditor {
	readonly uri: UriComponents;
	readonly options?: IEditorOptions;
	readonly openOnlyIfExists?: boolean;
	readonly openWith?: string;
}

Exposing the whole IEditorOptions might be overwhelming so we can also limit properties on the options

export interface IDefaultEditor<T extends Pick<IEditorOptions, 'override'> {
	readonly options?: T;
}

Last options is from @logan. The idea behind this is passing the original fragment and query parameters of the browser url to the custom editor opening the resource and the custom editor itself can decide how it wants to parse the fragment/params.

export interface IDefaultEditor {
	readonly uri: UriComponents;
	readonly selection?: IRange;
	readonly openOnlyIfExists?: boolean;
	readonly openWith?: string;
	readonly params?: Record<string, string>;
}

---

I'd love to have feedbacks from @joaomoreno and @bpasero, especially on what approach we take to extend the editor options support in web embedder api.

[bpasero]

I like the idea that we make it the editor resolver's responsibility to parse a UriComponents into a EditorInput and IEditorOptions. I think we should not have a top level selection in our default editor as a consequence, but rather make the text editor resolver fit to extract that line/col info and apply it to the options.

The idea from core workbench editor service was always that IEditorOptions is opaque: you can pass in any editor specific options such as notebook options and the options will get transported through. So an alternative would be to expose IEditorOptions in the default editor, I think that is fine as well.

ITextEditorOptions being so prominent in the core workbench API is a bit of a left over from before we had the editor resolver. I would argue that today it should move into respective text editor resolver and not be so prominent...

[bpasero]

I think we already picked a solution for the opener to convert the URI into URI + options via:

vscode/src/vs/platform/opener/common/opener.ts

Line 161 in b3ec818

export function extractSelection(uri: URI): { selection: ITextEditorSelection | undefined; uri: URI } {

and

vscode/src/vs/platform/opener/common/opener.ts

Line 147 in b3ec818

export function withSelection(uri: URI, selection: ITextEditorSelection): URI {

[rebornix]

I like the idea that we make it the editor resolver's responsibility to parse a UriComponents into a EditorInput and IEditorOptions. I think we should not have a top level selection in our default editor as a consequence, but rather make the text editor resolver fit to extract that line/col info and apply it to the options.

I like this idea. It would be natural if the editor resolver parse the resource Uri but it seems now that we have quite some syntax for file fragment. The EditorOpener, the QuickAccess, the CLI and lastly github.dev all have different styles of line/col syntax. It will be quite some effort to unify them and have a standard of way constructing/parsing the UriComponents to EditorOptions in the same resolver.

Also @joyceerhl mentioned there is probably a reason why we didn't put the fragment in the IDefaultLayout.uri, would love to hear more about the reasoning behind this.

So an alternative would be to expose IEditorOptions in the default editor, I think that is fine as well.

Since the reality now is each editorService user has their own logic of handling Uri#fragment. I actually prefer this a bit more. The workbench editor service get an T extends IEditorOption object and pass it to custom* editor without worrying how its shape (as long as it matches IEditorOptions). The changes are quite minimal and straight-forward (PR #147816) and I tested that the editor options are passed to the notebook editor seamlessly.

[bpasero]

There is always a risk that encoding these kind of options in the Uri ends up introducing the Uri into the system in that form because someone forgets to rewrite the Uri to Uri + Options. So a model where resource and options are clearly separated is actually nicer, now that I think about it. From the editor point of view (and in many cases due to our utils), a resource that is different only in query or fragment is still considered to not being equal to the same resource with different query or fragment.

For the opener we probably did not have much choice because the protocol service in the OS only allows to pass in a path to the application and not additional information, so we end up encoding everything into that path. I think a similar challenge exists for drag and drop where the transfer object is most often also just a resource.

[connor4312]

I tried testing this by running Github Issues: Copy Github Permalink in github.dev, but that resulted in a URL containing only lines and not the cell index. How should this be verified?

[jrieken]

Doesn't seem to work for me, open the wrong notebook for when giving it https://insiders.vscode.dev/github/microsoft/vscode/.vscode/notebooks/my-endgame.github-issues#C13

Screen.Recording.2022-04-27.at.12.06.20.mov

[joyceerhl]

@jrieken that URL seems malformed, should it be https://insiders.vscode.dev/github/microsoft/vscode/blob/main/.vscode/notebooks/my-endgame.github-issues#C13

[jrieken]

Oh, you are right. Closing as verified but I really need an action to generate these links for me