Preserve open editors in Cloud Changes #179507
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,7 @@ export class BreadcrumbsService implements IBreadcrumbsService { | |
|
|
||
| register(group: number, widget: BreadcrumbsWidget): IDisposable { | ||
| if (this._map.has(group)) { | ||
| console.error(`group (${group}) has already a widget`); | ||
|
There was a problem hiding this comment. What is the purpose of this change? There was a problem hiding this comment. Same as #179507 (comment) There was a problem hiding this comment. I think this would not be needed if we can avoid the hack for restoring the editors. |
||
| } | ||
| this._map.set(group, widget); | ||
| return { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,9 @@ import { DeferredPromise, Promises } from 'vs/base/common/async'; | |
| import { findGroup } from 'vs/workbench/services/editor/common/editorGroupFinder'; | ||
| import { SIDE_GROUP } from 'vs/workbench/services/editor/common/editorService'; | ||
| import { IBoundarySashes } from 'vs/base/browser/ui/sash/sash'; | ||
| import { EditSessionRegistry } from 'vs/platform/workspace/browser/editSessionsStorageService'; | ||
| import { ICommandService } from 'vs/platform/commands/common/commands'; | ||
| import { URI } from 'vs/base/common/uri'; | ||
|
|
||
| interface IEditorPartUIState { | ||
| serializedGrid: ISerializedGrid; | ||
|
|
@@ -149,6 +152,55 @@ export class EditorPart extends Part implements IEditorGroupsService, IEditorGro | |
| super(Parts.EDITOR_PART, { hasTitle: false }, themeService, storageService, layoutService); | ||
|
|
||
| this.registerListeners(); | ||
|
|
||
| this._register(EditSessionRegistry.registerEditSessionsContribution('workbenchEditorLayout', this)); | ||
| } | ||
|
|
||
| private static readonly EditSessionContributionSchemaVersion = 1; | ||
|
|
||
| getStateToStore() { | ||
| return { | ||
| version: EditorPart.EditSessionContributionSchemaVersion, | ||
| serializedGrid: this.gridWidget.serialize(), | ||
| activeGroup: this._activeGroup.id, | ||
| mostRecentActiveGroups: this.mostRecentActiveGroups | ||
| }; | ||
| } | ||
|
|
||
| resumeState(state: unknown, uriResolver: (uri: URI) => URI) { | ||
|
There was a problem hiding this comment. This looks to me like a big hack to resume state, in fact you even execute vscode/src/vs/workbench/browser/layout.ts Line 756 in cca4052 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. 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 There was a problem hiding this comment. 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. |
||
| if (typeof state === 'object' && state !== null && 'serializedGrid' in state && 'activeGroup' in state && 'mostRecentActiveGroups' in state && 'version' in state) { | ||
| if (state.version === EditorPart.EditSessionContributionSchemaVersion) { | ||
| this.mostRecentActiveGroups = (state as IEditorPartUIState).mostRecentActiveGroups; | ||
| this.instantiationService.invokeFunction(async (accessor) => { | ||
| const restoreFocus = this.shouldRestoreFocus(this.container); | ||
|
|
||
| await accessor.get(ICommandService).executeCommand('workbench.action.closeAllEditors').catch(() => { }); | ||
|
|
||
| const serializedGrid = (state as IEditorPartUIState).serializedGrid; | ||
|
|
||
| this.doCreateGridControlWithState(serializedGrid, (state as IEditorPartUIState).activeGroup, undefined, uriResolver); | ||
|
|
||
| // Layout | ||
| this.doLayout(this._contentDimension); | ||
|
|
||
| // Update container | ||
| this.updateContainer(); | ||
|
|
||
| // Events for groups that got added | ||
| for (const groupView of this.getGroups(GroupsOrder.GRID_APPEARANCE)) { | ||
| this._onDidAddGroup.fire(groupView); | ||
| } | ||
|
|
||
| // Notify group index change given layout has changed | ||
| this.notifyGroupIndexChange(); | ||
|
|
||
| // Restore focus as needed | ||
| if (restoreFocus) { | ||
| this._activeGroup.focus(); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private registerListeners(): void { | ||
|
|
@@ -557,14 +609,14 @@ export class EditorPart extends Part implements IEditorGroupsService, IEditorGro | |
| return this._partOptions.splitSizing === 'split' ? Sizing.Split : Sizing.Distribute; | ||
| } | ||
|
|
||
| private doCreateGroupView(from?: IEditorGroupView | ISerializedEditorGroupModel | null, uriResolver?: (uri: URI) => URI): IEditorGroupView { | ||
|
|
||
| // Create group view | ||
| let groupView: IEditorGroupView; | ||
| if (from instanceof EditorGroupView) { | ||
| groupView = EditorGroupView.createCopy(from, this, this.count, this.instantiationService); | ||
| } else if (isSerializedEditorGroupModel(from)) { | ||
| groupView = EditorGroupView.createFromSerialized(from, this, this.count, this.instantiationService, uriResolver); | ||
| } else { | ||
| groupView = EditorGroupView.createNew(this, this.count, this.instantiationService); | ||
| } | ||
|
|
@@ -1064,7 +1116,7 @@ export class EditorPart extends Part implements IEditorGroupsService, IEditorGro | |
| return true; // success | ||
| } | ||
|
|
||
| private doCreateGridControlWithState(serializedGrid: ISerializedGrid, activeGroupId: GroupIdentifier, editorGroupViewsToReuse?: IEditorGroupView[], uriResolver?: (uri: URI) => URI): void { | ||
|
|
||
| // Determine group views to reuse if any | ||
| let reuseGroupViews: IEditorGroupView[]; | ||
|
|
@@ -1082,7 +1134,7 @@ export class EditorPart extends Part implements IEditorGroupsService, IEditorGro | |
| if (reuseGroupViews.length > 0) { | ||
| groupView = reuseGroupViews.shift()!; | ||
| } else { | ||
| groupView = this.doCreateGroupView(serializedEditorGroup, uriResolver); | ||
| } | ||
|
|
||
| groupViews.push(groupView); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,17 +47,18 @@ export class FileEditorInputSerializer implements IEditorSerializer { | |
| return JSON.stringify(serializedFileEditorInput); | ||
| } | ||
|
|
||
| deserialize(instantiationService: IInstantiationService, serializedEditorInput: string, uriHandler: ((uri: URI) => URI) | undefined): FileEditorInput { | ||
|
There was a problem hiding this comment. 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.
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:
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
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.
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. |
||
| return instantiationService.invokeFunction(accessor => { | ||
| const serializedFileEditorInput: ISerializedFileEditorInput = JSON.parse(serializedEditorInput); | ||
| const resource = URI.revive(serializedFileEditorInput.resourceJSON); | ||
| const resolvedResource = uriHandler ? uriHandler(resource) : resource; | ||
| const preferredResource = URI.revive(serializedFileEditorInput.preferredResourceJSON); | ||
| const name = serializedFileEditorInput.name; | ||
| const description = serializedFileEditorInput.description; | ||
| const encoding = serializedFileEditorInput.encoding; | ||
| const languageId = serializedFileEditorInput.modeId; | ||
|
|
||
| const fileEditorInput = accessor.get(ITextEditorService).createTextEditor({ resource: resolvedResource, label: name, description, encoding, languageId, forceFile: true }) as FileEditorInput; | ||
| if (preferredResource) { | ||
| fileEditorInput.setPreferredResource(preferredResource); | ||
| } | ||
|
|
||
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
IViewinterface 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.