Search editor: registers as working copy even if saved

[bpasero]

I think this is asking for trouble: when a search editor is saved on disk, it still registers as working copy:

vscode/src/vs/workbench/contrib/search/browser/searchEditorInput.ts

Line 85 in 3a1e245

this.workingCopyService.registerWorkingCopy(workingCopyAdapter);

However, the text file model also registers as working copy, so we end up having 2 even though only 1 is the truth. This can lead to all kinds of weird things. We should only register a working copy to the workbench if we are introducing a new working copy to the application, not when reusing an existing one. In general there is no 1-to-1 mapping between editor input and working copy: many editor inputs can edit the same working copy.

Maybe it would be easier to split the input up into 2: a UntitledSearchEditorInput and a FileSearchEditorInput. The former would be picked if the resource scheme is search-editor and the latter only if the extension is .code-search. Only the untitled one should report as working copy. In fact the other one can simply extend TextEditorInput and benefit from a lot of default implementations for free.

[bpasero]

Upon second thought, I think ideally the search editor would do this:

• its probably fine to just register as one editor input that extends TextEditorInput
• if the search editor is untitled, it should simply use the ITextFileService.untitled helper to create an untitled working copy with a specific URI that we can use to open the custom search editor (e.g. file extension)
• if the search editor is saved, it should go through ITextFileService.files helper in the same way
• alternatively you can simply use ITextModelResolverService#createModelReference() to get a model with either a file or untitled scheme

That would mean, the search editor is really just another view onto files or untitled files but would not introduce a working copy on its own.

//cc @mjbvz I have a feeling that this model would work reasonably well for custom editors from extensions as well. E.g. if my editor works on top of text models, I think I would like to just contribute an editor for it, but not a working copy model. Everything data related should go through the text document.

[JacksonKearl]

I checked IBackupFileService#getBackups() and it seems like the text files do not get double-registered. When I conditionally disable the registration for file-backed search editors I see no entries in the backup service and working copy state is not restored. I think this is because I am not using the standard text file model, instead I create a raw model via ITextFileService#read() piped to IModelService#createModel().

vscode/src/vs/workbench/contrib/search/browser/searchEditorInput.ts

Lines 335 to 345 in a592b87

	// backupService.discardBackup(uri);
	} else if (uri.scheme !== searchEditorScheme) {
	contents = (await textFileService.read(uri)).value;
	} else {
	contents = existingData.text ??
	(existingData.config ? serializeSearchConfiguration(existingData.config) : '');
	}
	return modelService.createModel(contents, modeService.create('search-result'), uri);
	};
	const input = instantiationService.createInstance(SearchEditorInput, uri, getModel, config);

I think moving to ITextFileService.file for file-backed search editors would be a good idea, especially if it gives us things like restore and proper dirty tracking. But ITextFileService.untitled brings with it the "untitled mechanism" of handling dirty files and restore (anything not empty is dirty and restoring is deleting all contents), which is not what we want.

[bpasero]

@JacksonKearl I am not sure I understand. Instead, I have implemented a simple custom text editor that follows the model I am thinking about: https://github.com/microsoft/vscode/tree/ben/custom-text-editor

There are 2 commands:

• Open Custom Text Editor
• Open Untitled Custom Text Editor

They open a custom editor (simple text area) but delegate everything to the related text models without registering a working copy on their own.

I think this works reasonably well, only a few issues I notice:

• quite a bit of duplication needed in the editor input, I will try to move more things into TextInput
• once you start typing, we open the original file in the background because we think it is not opened. this is something I can look into

[bpasero]

@JacksonKearl I updated https://github.com/microsoft/vscode/compare/ben/custom-text-editor to require a lot less changes for the custom editor implementation by extending existing file and untitled inputs rather than duplicating all the code.

Things look pretty good in that world, editors no longer open duplicate when becoming dirty.

I would have to clean up a small hack for untitled editors but that is all doable. Let me know if you could adopt a search editor on top of this model.

[bpasero]

Again I updated https://github.com/microsoft/vscode/compare/ben/custom-text-editor to reflect latest changes in untitled land. I consider this done from the workbench point of view. I added a third command "Open custom untitled editor with initial contents" to showcase how one can open an untitled editor with initial content that is not showing up as dirty. Note that in this case reloading the window will restore the editor to be empty because backup only works for dirty editors.

[bpasero]

@JacksonKearl we can probably move this to March as I think the changes have a bigger impact and should get a full iteration of testing.

I have a wip solution in https://github.com/microsoft/vscode/tree/ben/custom-search-editor (diff) that you could maybe then pick up (it requires some of the changes from my other ben/custom-text-editor branch which we can easily put to master as needed).

One thing I did not look into was your separation into 2 models: it seems there is a header model and a content model that the work happens in. I am not sure why that is needed, I would keep it to 1 model all in all and let the UI pieces reflect this accordingly if possible. If that cannot be done, then you need to forward all changes of these 2 models into the model of the text or untitled file.

There is probably also some room for cleaning up the duplication between UntitledSearchEditorInput and FileEditorInput. Unfortunately they cannot extend the same base class :-/

[JacksonKearl]

there is a header model and a content model that the work happens in. I am not sure why that is needed

I found the model that the actual CodeEditorWidget gets works best if it contains just the results. I tried setting hidden ranges for the header region but that gave rise to other issues, in particular lots of bugs that pop up when a CodeEditorWidget contains only hidden ranges.

[JacksonKearl]

@bpasero I'm going to migrate this over to your new model for this debt week, can the branch be merged in now?

[bpasero]

@JacksonKearl you mean these two changes only right?

• https://github.com/microsoft/vscode/compare/ben/custom-text-editor#diff-9a74d47c1b8834834c055fd14a08e442
• https://github.com/microsoft/vscode/compare/ben/custom-text-editor#diff-dd979ba46af58bcc72d293f673c39e8b

I think the rest was just me playing around and not meant to be merged.

[JacksonKearl]

Yes, don't need the dummy implementation

[bpasero]

@JacksonKearl yeah those are fine, maybe put me on a PR review once you wrap it up so I can do a final check.

[JacksonKearl]

Closed with 379e732. Tried going with the inheriting from Base....Input approach at first, but ended up finding the resultant code wasn't much simpler and produced more problems to fix (https://github.com/microsoft/vscode/compare/jackson/searchEditorRefactor).

Instead simplified the existing code pretty decently by removing the ability for an SearchEditorInput to be Untitled or Saved, instead those that have an on-disk representation maintain a backingUri reference that is used when they need to save and besides that all the code paths are the same. This means we only deal with (and therefore register working copies for) search-editor schemes, never file schemes.

Verify by code inspection I suppose:

vscode/src/vs/workbench/contrib/searchEditor/browser/searchEditorInput.ts

Lines 97 to 99 in 379e732

	if (this.modelUri.scheme !== SearchEditorScheme) {
	throw Error('SearchEditorInput must be invoked with a SearchEditorScheme uri');
	}

Also, with regard to the issue that prompted this refactoring, #93745, verify that it is now possible to open a saved search editor and the diff for that search editor.

[bpasero]

As long as you maintain your own URI scheme, things should be OK. I gave it a brief test and it seems to work, but maybe put a verification needed on it to verify in endgame with some steps.