Lramos15/replace on save #143144
Lramos15/replace on save #143144
Conversation
| 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(); |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hm yeah, I think this would resolve automatically once save / saveAs no longer returns a typed editor input.
| @@ -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); | |||
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can see in debt week whether a cleaner change is possible.
This PR fixes #81832
Now on save if a replace will be completed we utilize the resolver to change the editor input