-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
WebviewContentProvider API investigation #45994
Comments
@jrieken Is |
A few more ideas after discussions during and after our standup: 1) have explicit state and a factory
export interface WillCloseWebviewEvent {
storeState(state: any): void;
}
export Webview {
onWillClose: Event<WillCodeWebviewEvent>
}
export interface WebviewFactory {
export function createWebview(id:string, state:any, token: CancellationToken): Webview;
}
export namespace window {
export function createWebview(id:string, options): Webview;
export function registerWebviewFactory(id:string, ...): Disposable;
} The markdown flow would be
2) have a webview serialiser We can make the (web)view-state something that is first-class and ask extensions to (optionally) provide a state serialiser/deserialiser for restoring webviews and ideally tree views export interface WebviewSerializer {
captureWebview(webview: Webview):any;
restoreWebview(state:any): Webview;
}
export namespace window {
export function registerWebviewSerializer(id: string, serializer: WebviewSerializer): Disposable;
export function createWebview(id:string, options...): Webview;
} Again, the markdown flow
|
Ok, I like option 2 but am concerned about needing to call out to extensions on shutdown to get the state. My proposal is that we make the interface Webview {
...,
state: any;
}
// TODO: better name
interface WebviewRehydrator {
restoreWebview(webview: Webview): void
}
export namespace window {
export function registerWebviewRehydrator(id: string, rehydrator: WebviewRehydrator): Disposable;
} That way, on shutdown we can just write out the view state without having to call into extensions @jrieken Any concerns with this approach? |
Yeah, we discussed the continuous state writing approach and I initially liked it but I now think that reality is more complex, esp. since most state is managed inside the webview-process and not the extension. Asking extensions to continuously update means heavy ipc-traffic and it's also hard to enforce, e.g some extensions might continuously update their state and some might never do it. That's why I preferred the explicit pull model in the end.
Sharing but also not sharing the concern. For once we already call into extensions on shutdown have a little of a grace/waiting period. Then, the API doesn't talk about shutdown and the workbench is free to call the |
Ok, I'm just struggling to fit pull serialization into our editor model. Leveraging Realistically the extension host or the extension itself could throttle the state updates. These state objects should also be small. The markdown preview's state for example would be something like: {
"resource": "/path/to/file.md",
"locked": false,
"topMostVisibleRow": 0
} Sorry for bringing it up again, but a few tweaks on the
|
I understand but that shouldn't have an influence in how we design the API. @bpasero might be able to help out and explore how editor input persistence can be made async |
It all starts from here for the editor input factory to persist. We do allow long running operations from the shutdown, so we could make this async. But as usual, doing long running things in the shutdown phase is something we should stay away from. In the worst case it can mean that the OS flags VS Code as preventing a shutdown if it takes too long to quit. |
Thanks @bpasero! That seems like exactly what we need here. I'll work to get a prototype of this API for this iteration. There are a few other things to investigate as part of restoration that will be pushed off for this iteration:
|
@mjbvz I believe we already have code in place that gracefully deactivates extensions on shutdown by giving them a little bit of time (not sure where that is implemented and if the time is just hardcoded). We would need to add this logic close to that place to not have a race condition between deactivating extensions and persisting the editor view state.
I think that will happen automatically if the call to |
From #45994 Adds an experimental API that allows extensions to persist webviews and restore them even after vscode restarts. Uses this api to restore markdown previews. This is done by: - Adding a new `state` field on webviews. This is a json serializable blob of data - Adds a new `WebviewReviver` interface (name will probably change). This binds a specific viewType to a provider that can restore a webview's contents from its `state` - In VS Code core, persist webview editors. When we restart and need to show a webview, activate all extensions that may have reviviers for it using the `onView:viewType` activation event. Current implementation is sort of a mess. Will try to clean things up
Fixed by #46380. Will open new issues to track the individual issues identified |
From #43713
Problem
The currently proposed webview API is a push model (extensions call
createWebview
to create and push a webview to vscode). This model prevents us from implementing restoration of webviews when vscode is reloaded. It also does not feel consistent with the other APIsProposal
Based on a discussion with @jrieken and @kieferrm, move webview to more of a pull model similar to the
TreeDataProviderProposal
(vscode asks an extension to create a webview). This would also be very similar to the existingTextDocumentContentProvider
api.Here's a mock up:
The markdown extension for example would register a provider for the
markdown-preview:
scheme. API usage:Create a new markdown preview
vscode.open('markdown-preview:preview1')
Restoring a markdown preview
'markdown-preview:preview1'
uri is open'markdown-preview:preview1'
document. This inits any extensions that activate onmarkdown-preview:
and then invokes markdown's content providerClose a markdown preview
Either:
.dispose()
on the webview/cc @sandy081
The text was updated successfully, but these errors were encountered: