-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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
Preserve open editors in Cloud Changes #179507
Conversation
…oyceerhl/giant-otter
@@ -644,7 +644,7 @@ export class Grid<T extends IView = IView> extends Disposable { | |||
} | |||
|
|||
private getViewLocation(view: T): GridLocation { | |||
const element = this.views.get(view); | |||
const element = this.views.get(view) ?? [...this.views.values()][0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are hacks and should not be shipped as is. I encountered issues with the lifecycle of the view attached to the active GridWidget. It seems that when we close all editors or dispose this.gridwidget, we still keep around one grid widget (and therefore one view, breadcrumb control etc.) which never gets disposed. This presents issues when trying to restore serialized state, because when deserializing editor view nodes, the previous view and breadcrumb control are still lying around, so getting the active view location and setting the breadcrumb control will throw. I am not familiar enough with the gridview to understand why this is desirable behavior and would appreciate a pointer on how to avoid this hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would need to discuss with Grid owner @joaomoreno
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bpasero The grid's IView
interface isn't disposable. The grid doesn't own each view's lifecycle. It won't dispose them, otherwise it would also have to conceptually create them. Since it's up to the grid user to create the views (which from the grid's viewpoint aren't "alive"), it's also up to the grid user to dispose them, if needed.
The grid is only disposable because it listens to DOM events, eg. mouse click on the sashes.
@@ -35,7 +35,7 @@ export class BreadcrumbsService implements IBreadcrumbsService { | |||
|
|||
register(group: number, widget: BreadcrumbsWidget): IDisposable { | |||
if (this._map.has(group)) { | |||
throw new Error(`group (${group}) has already a widget`); | |||
console.error(`group (${group}) has already a widget`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as #179507 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would not be needed if we can avoid the hack for restoring the editors.
}; | ||
} | ||
|
||
resumeState(state: unknown, uriResolver: (uri: URI) => URI) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to me like a big hack to resume state, in fact you even execute workbench.action.closeAllEditors
which will result in flicker and would not even close dirty editors. We have a dedicated location on startup where we determine which editors to open and I think we need to integrate session resume there:
vscode/src/vs/workbench/browser/layout.ts
Line 756 in cca4052
const editors = await this.state.initialization.editor.editorsToOpen; |
Having the editors to open there ensures that there will be no flicker and it will also not require to drop the current editor state and recreate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue here is that at startup, the edit session payload is not guaranteed to be available (unless we move towards blocking resuming editor state on a network call to the storage server to retrieve the payload). Moreover an edit session payload today may be applied even after editor startup via the Resume Latest Changes From Cloud
command, which together with open editors would support scenarios like https://github.com/microsoft/vscode/issues/35307
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we cannot just deserialize some state over existing state because you may have dirty editors opened that you cannot just close. So if we want to restore editors, it has to go through a different model that does not drop the grid and creates a new grid.
@@ -47,17 +47,18 @@ export class FileEditorInputSerializer implements IEditorSerializer { | |||
return JSON.stringify(serializedFileEditorInput); | |||
} | |||
|
|||
deserialize(instantiationService: IInstantiationService, serializedEditorInput: string): FileEditorInput { | |||
deserialize(instantiationService: IInstantiationService, serializedEditorInput: string, uriHandler: ((uri: URI) => URI) | undefined): FileEditorInput { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a big fan of passing on the URI resolver to factories, shouldn't the factory when serializing and deserializing take care of using a format that can roam to other locations?
Besides, there are many factories for many editors (for example notebooks, custom editors), so this change will only work for text files and we would need an adoption in all factories.
See also #179507 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the factory when serializing and deserializing take care of using a format that can roam to other locations
This is the first approach that I took, which I ultimately walked back because I found that it would lead to information duplication--to determine whether a URI is relevant to the current workspace, we must know four things:
- The workspace folder that contained the URI, if it was part of a workspace folder before (the 'base uri')
- The relative path from the workspace folder
- The additional metadata ('edit session identity') which associates the original workspace folder with one of the folders from the current workspace
- The actual current workspace folder that the relative path should be reunited with
If we try to make each factory 'self sufficient' by storing all of the above information, this leads to us duplicating and leaking knowledge of the edit session identity into the state of every workbench contribution which contributes to the payload, since now every workbench contribution must do the work of calculating and matching identities, workspace folders, and constructing URIs. IMHO this would raise the cost of adopting edit sessions and would lead to duplicated work across all contributors to edit sessions.
To simplify adoption, the next approach I took (in this PR) was to abstract all of this knowledge via the uriResolver
function, which
- checks whether an absolute URI from another filesystem is relevant to a workspace folder, and if so which one
- resolves the absolute URI from another filesystem to one which should work in the current workspace folder
My intention is that putting this knowledge into a resolver rather than each contrib's state would make it easier to adopt across other workbench contribs, e.g. SCM (commit input), comments (draft comments), debug (breakpoints) and so on.
Besides, there are many factories for many editors (for example notebooks, custom editors), so this change will only work for text files and we would need an adoption in all factories.
You're absolutely right, this PR just shows a proof of concept for text editors, and once we have settled on a good approach I'd love to help adopt it for all editor types as well as other workbench contributions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets talk this through today.
I think a big challenge is that today editors are serialised with their full |
Superseded by #183449 |
For #179898
This PR introduces the ability to roam open editors as part of Cloud Changes. This allows the following scenarios:
Cloud Changes: Store Working Changes in Cloud
or enabling"workbench.experimental.cloudChanges.autoStore": "onShutdown"
Here's a demo of preserving open editors going from vscode.dev -> RemoteHub on desktop:
continue-on-restore-editors.mp4
The way this works is:
editorPart.ts
register anIEditSessionWorkbenchStateContribution
with a unique identifier:uriResolver
handler knows how to convert fully-qualified URIs which may have originated on a different filesystem to URIs that are applicable to the current workspace in VS Code. Under the hood it calls the registeredEditSessionIdentityProvider
to match the edit session payload to the current workspace. If there is no match,uriResolver
simply returns the original URI that was passed in. This abstracts away knowledge of how to convert URIs from each registered contribution, so thateditorPart.ts
can continue storing the serialized state that it already stores today with no additional properties.getStateToStore
. I'd suggest to version the opaque object that gets stored for future proofing (and have done so in this prototype implementation).workbenchEditorLayout
in the above code snippet. Contributions can then run theuriResolver
arg to convert any stored URIs in thestate
object, resulting in state that can be applied to the current workspace.The intent is that
IEditSessionWorkbenchStateContribution
becomes a way for other parts of the workbench to transfer state--it should be straightforward to adopt it to transfer active editor selections, breakpoints, SCM input history, etc. This then becomes a secondary series of lifecycle restore (with potential UX implications for when we consider the workbench 'ready' that might require UI to communicate that state from edit sessions are yet to be restored).@bpasero I'm not familiar with the
editorPart.ts
, but I encountered issues with the lifecycle of the view attached to the activeGridWidget
. It seems that when we close all editors or disposethis.gridwidget
, we still keep around one grid widget (and therefore one view, breadcrumb control etc.) which never gets disposed. This presents issues when trying to restore serialized state, because when deserializing editor view nodes, the previous view and breadcrumb control are still lying around, so getting the active view location and setting the breadcrumb control will throw. I put in two hacks for that in this prototype here and here but I would like to fix those issues with your guidance before merging.cc @rebornix as this changes the payload structure and the signature for the
IEditSessionWorkbenchStateContribution
, let me know if this still fits the scenario for roaming notebook controllers.