bulk file edits: make sure to use textFileService when creating files

[isidorn]

This PR fixes #112377

This PR adds an option to pass multiple resources to create in the textFileService.
A bit ugly part is that now only the create method in the textFileService accepts multiple resoruces and undo and cancellation arguments.

I have tested this out and these changes indeed fix the issue.
Note that I am converting a VSBuffer to a string via the toString method for each edit.

@bpasero let me know what you think
@jrieken fyi

[bpasero]

The changes in text file land are fine to me. What I don't really understand is the need for converting a VSBuffer into a string via toString. That imho is dangerous because if the user picked a non UTF-8 encoding to create a file, toString is not safe to use. It needs to be the VSBuffer so that the encoding is preserved.

[isidorn]

@bpasero thanks for the review.
I will change the getEncodedReadable to also work with the VSBuffer then I think we should be good.

Edit: ok not so easy, since the toEncodeReadable is written to work with Readable.
So my question is: is there some better way to transform a VSBuffer to a Readable or a ITextSnapshot.

@jrieken let me know if you have ideas, and if it might be ok to use the toString() in the bulkEditService. I am not sure do we know what encodings are coming in via the edits?

[bpasero]

To clarify: getEncodedReadable will use iconv-lite to return bytes from the string as configured by the encoding. These bytes are then passed to the working copy file service to put to disk.

If we need to preserve the actual string in the bulk edit service, e.g. for undo/redo, then this needs to be the original value before it was passed to getEncodedReadable, otherwise you would need to know the encoding to restore the original value.

Also keep in mind how efficient it is to use string, vs ITextSnapshot. The advantage of ITextSnapshot (as far as I remember) is that we don't have to create a new string from the editor model.

[isidorn]

Discussed with @jrieken, looked a bit more into the issue, and took a step back. Some findings:

• edits do not have to be textual (it can be an image) so it does not make sense to use the textFileService in all cases.
• The proper encoding for new empty files was working before because the Explorer was directly using the textFileService for creating empty content pointer
• This does not work in Insiders because workingCopyFileService is used instead, which directly goes to the fileService thus bypasing the textFileService and the encoding. The textFileService gets the encoding from here. This is true even when an empty file is created, since we create an empty encoded readable which respectes the encoding here

Due to the above I propose the following solution:

• Create folders and non empty file using the workingCopyFileService. Since non empty files do not have to be textual we can not use the textFileService
• Create empty files using the textFileService

Since only for empty files we have to read the actual encoding. Non empty files the bits coming in already have an encoding and we can go directly to the workingCopyFileService.

I have updated the PR to use this strategy. I have verified this fixes the issue.
@bpasero let me know what you think

[bpasero]

Yes, I think this is an elegant solution. If you or Jo feel that the direct use of ITextFileService.createFile is not nice inside bulkEdit, I am also open to make getEncodedReadable public so that you can create the right VSBufferReadable when the file is empty.

[isidorn]

@bpasero thanks for the review. That would also be an option.
I am fine with how things ended up. Let's see if Joh has some comments, if not I will merge this in later in the afternoon.

[jrieken]

@bpasero I am still curious how you store/remember the encoding for an empty file. Is the file not empty in that case but uses some bytes to store the encoding?

[bpasero]

@jrieken this is only required for encodings that use a byte order mark, which means: only UTF8 (with BOM), UTF-16LE and UTF-16BE. Those files, when empty, contain the BOM.

[jrieken]

Ah, thanks for clarifying.

I am also open to make getEncodedReadable public so that you can create the right VSBufferReadable when the file is empty.

I believe that's nice. Ideally, WorkspaceFileEditOptions get's a new, optional property that carries a VSBuffer with the contents of the file. The explorer uses then some mechanics to create that buffer.

[isidorn]

@bpasero let me know if you plan to make getEncodedReadable public. And if you will push that to master. If yes, then I can discard this PR and just adopt that method on master.
Otherwsie I will merge this in.

[bpasero]

Please go ahead and make it public as part of this PR because you also will need to adopt it.

[isidorn]

@bpasero I have updated the PR, check it out and let me know what you think?
I can also make the getEncodedReadable method take less arguments since my use case I only pass in the resource. Though I left it with all the arguments in case somebody wants to use it

I can now change the textFileService back to just supporting 1 argument for create. Let me know what you would prefer.

[bpasero]

👍

[isidorn]

Thanks for the reviews and help. Merging in.