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.

Sign up for GitHub

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

Jump to bottom

Lramos15/replace on save #143144

Merged
merged 2 commits into from Feb 15, 2022
Merged

Lramos15/replace on save #143144

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24bbcca
Replace editor upon saving
lramos15 Feb 15, 2022
2932c50
Merge branch 'main' into lramos15/replaceOnSave
lramos15 Feb 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/vs/workbench/services/editor/browser/editorService.ts
View file
Open in desktop
Expand Up @@ -910,8 +910,16 @@ export class EditorService extends Disposable implements EditorServiceImpl {
if (!result.matches(editor)) {
const targetGroups = editor.hasCapability(EditorInputCapabilities.Untitled) ? this.editorGroupService.groups.map(group => group.id) /* untitled replaces across all groups */ : [groupId];
for (const targetGroup of targetGroups) {
const resolvedEditor = await this.editorResolverService.resolveEditor({ resource: result.resource, }, targetGroup);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lramos15 I think this solution is a bit of a bandaid: ideally editor.saveAs and editor.save would return an untyped editor input and then replaceEditors should not be called with a typed editor but with an untyped one. This is somewhat a missing adoption of untyped editors in general (I was aware of it but forgot about it).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I was just a bit nervous to go with that level refactor given I'm not really sure how sequential vs non sequential save work and all that plays a role. This change while bandaid seemed safe and allows the testing of this flow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see in debt week whether a cleaner change is possible.

const group = this.editorGroupService.getGroup(targetGroup);
await group?.replaceEditors([{ editor, replacement: result, options: editorOptions }]);
if (resolvedEditor !== ResolvedStatus.ABORT && isEditorInputWithOptionsAndGroup(resolvedEditor)) {
// dispose the previous result before replacing with the resolved editor
saveResults.pop()?.dispose();
Copy link
Member

@bpasero bpasero Feb 16, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lramos15 I am not so sure about this dispose call on the editor input. We typically never call dispose on editor inputs because the lifecycle may be different depending on the kind of input. For example, all file editor inputs are shared across all groups.

Why is this dispose call needed? It will happen as part of the replaceEditors call via a close call I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well normally we replace editor with result but here we are replacing editor with resolvedEditor meaning the editor input returned from save (result) isn't being used. So I dispose of it and add the resolved editor as the result instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah, I think this would resolve automatically once save / saveAs no longer returns a typed editor input.

saveResults.push(resolvedEditor.editor);
await group?.replaceEditors([{ editor, replacement: resolvedEditor.editor, options: editorOptions }]);
} else {
await group?.replaceEditors([{ editor, replacement: result, options: editorOptions }]);
}
}
}
}
Expand Down