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

Implement large file confirmation for file working copy editors #172129

Closed
jrieken opened this issue Jan 24, 2023 · 8 comments
Closed

Implement large file confirmation for file working copy editors #172129

jrieken opened this issue Jan 24, 2023 · 8 comments
Assignees
Labels
debt Code quality issues notebook workbench-editors Managing of editor widgets in workbench window

Comments

@jrieken
Copy link
Member

jrieken commented Jan 24, 2023

Testing #171743

  • vscode desktop
  • have a 2BG file
  • in the explorer, right+click and open in HEX editor
  • 🐛 no warning
@bpasero
Copy link
Member

bpasero commented Jan 24, 2023

Yeah it will only apply to text editors, I will update the test plan item and maybe the setting should change to workbench.textEditorLargeFileConfirmation.

@jrieken
Copy link
Member Author

jrieken commented Jan 24, 2023

same is true for notebooks (and I guess for any custom editor)

@bpasero
Copy link
Member

bpasero commented Jan 24, 2023

Yeah, it is currently only implemented for text editors, though the underlying logic for file size limits is available to anyone from the file service via the limits option:

readonly limits?: IFileReadLimits;

We can probably implement it for notebooks, by adding the limit here:

const content = await this.fileService.readFileStream(this.resource, { etag });

But for custom editors that are not text based, the loading of data is not under our control...

@jrieken
Copy link
Member Author

jrieken commented Jan 24, 2023

+1 for doing this for editors that we ship out of the box /cc @roblourens @rebornix

@bpasero bpasero changed the title Large file warning doesn't work when opening with HEX editor Implement large file confirmation for notebooks Jan 24, 2023
@bpasero bpasero added the workbench-editors Managing of editor widgets in workbench window label Jan 24, 2023
@bpasero
Copy link
Member

bpasero commented Jan 24, 2023

I can see to add this for notebooks, but custom editors that do file loading on their own will probably not be so easy to get working with that setting.

@bpasero bpasero changed the title Implement large file confirmation for notebooks Implement large file confirmation for file working copy editors Jan 25, 2023
@rebornix
Copy link
Member

rebornix commented Feb 2, 2023

I can see to add this for notebooks

I was expecting this to auto opt-in feature for working copy but it seems it requires us to handle it in editor input or editor level?

@bpasero
Copy link
Member

bpasero commented Feb 2, 2023

I have not started the investigation yet, but yeah it might be on the editor level and not generic.

Update: this needs to be implemented in editor inputs and editors currently if we want to avoid throwing the error when any working copy is resolved. My intention was to limit this new error to editor resolving only because there we have a UI for the user to resolve it. If we were to have this size limit in a generic way, anyone resolving a model would potentially have to deal with this error.

For text files, the limit is installed here:

await this.textFileService.files.resolve(this.resource, {
languageId: this.preferredLanguageId,
encoding: this.preferredEncoding,
contents: typeof preferredContents === 'string' ? createTextBufferFactory(preferredContents) : undefined,
reload: { async: true }, // trigger a reload of the model if it exists already but do not wait to show the model
allowBinary: this.forceOpenAs === ForceOpenAs.Text,
reason: TextFileResolveReason.EDITOR,
limits: this.ensureLimits(options)
});

And the editor handling part is here:

// Handle case where a file is too large to open without confirmation
if ((<FileOperationError>error).fileOperationResult === FileOperationResult.FILE_TOO_LARGE && this.group) {
let message: string;
if (error instanceof TooLargeFileOperationError) {
message = localize('fileTooLargeForHeapErrorWithSize', "The file is not displayed in the text editor because it is very large ({0}).", ByteSize.formatSize(error.size));
} else {
message = localize('fileTooLargeForHeapErrorWithoutSize', "The file is not displayed in the text editor because it is very large.");
}
throw createTooLargeFileError(this.group, input, options, message, this.preferencesService);
}

@bpasero
Copy link
Member

bpasero commented Dec 5, 2023

Notebook support is coming:

image

@bpasero bpasero added this to the December 2023 milestone Dec 5, 2023
bpasero added a commit that referenced this issue Dec 5, 2023
* working copy - enable file limits when resolving (#172129)

* .
@bpasero bpasero closed this as completed Dec 5, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jan 21, 2024
@aiday-mar aiday-mar added this to the December / January 2024 milestone Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues notebook workbench-editors Managing of editor widgets in workbench window
Projects
None yet
Development

No branches or pull requests

4 participants