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.

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

Editor service should not create untitled editors before overrides #106012

Closed
abrasaxdeity opened this issue Sep 3, 2020 · 13 comments
Closed

Editor service should not create untitled editors before overrides #106012

abrasaxdeity opened this issue Sep 3, 2020 · 13 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workbench-editor-resolver Issues resolving the editor inputs workbench-untitled-editors Managing of untitled editors in workbench window
Milestone

Comments

@abrasaxdeity
Copy link

Add a command to the existing custom editor sample extension

{ "command": "catCustoms.catScratch.new", "title": "Create new Cat Scratch Document", "category": "Cat Scratch" }

register command in extension.ts

vscode.commands.registerCommand('catCustoms.catScratch.new', () => { vscode.workspace.openTextDocument({ language: 'catScratch', content: '' }).then(doc => { vscode.commands.executeCommand("vscode.openWith", doc.uri, "catCustoms.catScratch"); }); });

  1. Open command palette and select Create new Cat Scratch Document - Untitled-1 tab opens
  2. Close opened editor
  3. Repeat 1. - Untitled-2 opens

Bug is in step 3, since Untitled-1 was previously closed name should be free for the new untitled file

@bpasero bpasero assigned mjbvz and unassigned bpasero and jrieken Sep 3, 2020
@bpasero bpasero added bug Issue identified by VS Code Team member as probable bug custom-editors Custom editor API (webview based editors) labels Sep 3, 2020
@bpasero bpasero changed the title openTextDocument Untitled scheme with vscode.openWith and custom text editor naming issue Custom text editors do not free up untitled models Sep 3, 2020
@bpasero
Copy link
Member

bpasero commented Sep 3, 2020

I saw this recently and forgot to follow up: there is an easier repro for this and I think somewhere when custom text editors were added, we forgot to deal with untitled models properly. I think they are not getting freed up.

Steps:

  • revert my change in 7686b52
  • run out of sources
  • select File > New File
  • you should get a picker to pick an editor
  • click ESC to close picker
  • repeat

=> 🐛 you see Untitled-2 and this continues

recording (2)

@mjbvz mjbvz added this to the Backlog milestone Sep 17, 2020
@bpasero
Copy link
Member

bpasero commented Mar 19, 2021

@lramos15 I think this will be part of your work: with the new override handling moving out of the editor group I think this should be fixed as well because we will no longer create an untitled editor input before the override has happened 👍

@lramos15
Copy link
Member

Optimistically closing. I wasn't able to get the repro steps above to work, but when I create new untitled files with the hex editor the number no longer increments.

@bpasero bpasero modified the milestones: Backlog, April 2021 Apr 1, 2021
@roblourens roblourens added the verified Verification succeeded label Apr 29, 2021
@roblourens
Copy link
Member

I repro this with the original steps

@roblourens roblourens reopened this Apr 29, 2021
@roblourens roblourens added verification-found Issue verification failed and removed verified Verification succeeded labels Apr 29, 2021
@lramos15 lramos15 modified the milestones: April 2021, May 2021 Apr 29, 2021
@lramos15 lramos15 added workbench-editor-resolver Issues resolving the editor inputs and removed custom-editors Custom editor API (webview based editors) verification-found Issue verification failed labels Apr 29, 2021
@lramos15
Copy link
Member

lramos15 commented May 6, 2021

@bpasero or @mjbvz I think I need some help with this one, I'm not sure where these models are getting created / why they're sticking around. I don't really see this else where just in this particular open flow.

@bpasero
Copy link
Member

bpasero commented May 6, 2021

Untitled text models typically get created through the IUntitledTextEditorModelManager.create method, but in theory you can also ask the IEditorService#createEditorInput method to create a untitled editor input with associated model for you when you pass in no resource and give contents in the arguments.

I am not sure where in custom editor land untitled files are created.

@lramos15
Copy link
Member

lramos15 commented May 6, 2021

It seems like openTextDocument creates a model, and then openWith creates a second one. I fixed properly disposing the openWith model here 9f32d37 (At least I think, I'm not sure if it's expected that the editor input would be disposed in this service). However, I have no way to handle the openTextDocument model disposing as that isn't handled by the editor service flow. Seems like this feature request might cleanup this flow #93441

@bpasero
Copy link
Member

bpasero commented May 7, 2021

I would argue that it should be the IEditorService that creates EditorInput from the untyped ones and that one should properly be disposed if it gets overridden by the editor override service. We are probably not there yet because we still allow someone to pass in an EditorInput to the IEditorService and then have questionable lifecycle issues.

As soon as everyone only uses untyped editor inputs, the lifecycle is a lot clearer and owned only by editor service and editor override service.

@lramos15 lramos15 modified the milestones: May 2021, On Deck May 13, 2021
@bpasero bpasero changed the title Custom text editors do not free up untitled models Editor service should not create untitled editors before overrides May 21, 2021
@bpasero
Copy link
Member

bpasero commented May 21, 2021

I have renamed the issue. I think #124352 would probably fix this. The gist is that we should stay away from creating UntitledTextEditorInput in the editor service before the overrides because it is possible that an override kicks in and then the untitled text editor was created for no good reason and possibly leaks.

@bpasero bpasero added the workbench-untitled-editors Managing of untitled editors in workbench window label May 21, 2021
@bpasero
Copy link
Member

bpasero commented Jul 7, 2021

I think this can be closed?

@lramos15 lramos15 modified the milestones: On Deck, June 2021 Jul 7, 2021
@lramos15 lramos15 closed this as completed Jul 7, 2021
@rzhao271
Copy link
Contributor

rzhao271 commented Jul 7, 2021

I can still reproduce the issue with the original steps.

Edit: I've created a repro here https://github.com/microsoft/vscode-extension-samples/tree/rzhao271/106012-repro/custom-editor-sample.

@rzhao271 rzhao271 reopened this Jul 7, 2021
@rzhao271 rzhao271 modified the milestones: June 2021, Backlog Jul 7, 2021
@lramos15
Copy link
Member

lramos15 commented Jul 7, 2021

Ah the steps aren't the same since custom editors don't properly implement the untitled factory yet. I'm not sure the best way to verify this @bpasero but I know it no longer creates the untitled input.

@bpasero
Copy link
Member

bpasero commented Jul 8, 2021

This should be the steps:

  • install GH issue extension notebook
  • from command line start vscode on a *.github-issues file that does not exist
  • it should open a issue notebook editor
  • close that
  • File > New File
  • it should be "Untitled-1"

Previously it would be "Untitled-2", indicating that we leaked a typed untitled editor.

@rzhao271 rzhao271 closed this as completed Jul 8, 2021
@rzhao271 rzhao271 modified the milestones: Backlog, June 2021 Jul 8, 2021
@rzhao271 rzhao271 added the verified Verification succeeded label Jul 8, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workbench-editor-resolver Issues resolving the editor inputs workbench-untitled-editors Managing of untitled editors in workbench window
Projects
None yet
Development

No branches or pull requests

8 participants
@roblourens @bpasero @jrieken @lramos15 @rzhao271 @mjbvz @abrasaxdeity and others