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
WorkspaceFileEditOptions add maxSize #111042
Conversation
} catch (err) { | ||
this._logService.critical(err); | ||
} | ||
} | ||
|
||
const useTrash = this._fileService.hasCapability(this.oldUri, FileSystemProviderCapabilities.Trash) && this._configurationService.getValue<boolean>('files.enableTrash'); | ||
await this._workingCopyFileService.delete([this.oldUri], { useTrash, recursive: this.options.recursive, ...this.undoRedoInfo }); | ||
return this._instaService.createInstance(CreateOperation, this.oldUri, this.options, { isUndoing: true }, contents); | ||
|
||
if (typeof this.options.maxSize === 'number' && fileContent && (fileContent?.size > this.options.maxSize)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: move to two new private functions so it is reusable and easier to read e.g.
if (this->hasMaxFileSize() && this._doesSizeExceedMax(fileContent)) { return new Noop(); }
} catch (err) { | ||
this._logService.critical(err); | ||
} | ||
} | ||
|
||
const useTrash = this._fileService.hasCapability(this.oldUri, FileSystemProviderCapabilities.Trash) && this._configurationService.getValue<boolean>('files.enableTrash'); | ||
await this._workingCopyFileService.delete([this.oldUri], { useTrash, recursive: this.options.recursive, ...this.undoRedoInfo }); | ||
return this._instaService.createInstance(CreateOperation, this.oldUri, this.options, { isUndoing: true }, contents); | ||
|
||
if (typeof this.options.maxSize === 'number' && fileContent && (fileContent?.size > this.options.maxSize)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit pick: Braces around last condition not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestoin. This way it is more readable for me.
if (!this._undoesCreateOperation && !this.options.folder) { | ||
try { | ||
contents = (await this._fileService.readFile(this.oldUri)).value; | ||
fileContent = (await this._fileService.readFile(this.oldUri)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit pick: braces around await not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I will change that. Copy / paste error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look OK to me tho, this doesn't really solve the problem because the accumulation of things (10 little files etc). To circumvent the "restore from trash" problem we could try to move/copy the file that's to be deleted to some tmp-folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT
This PR introduces a
maxSize
option to theWorkspaceFileEditOptions
.We need to limit how large a file we keep in memory, otherwise this can grow unbounded.
The idea is that Explorer would use this API to limit file sizes that we keep in memory to 5mb.
Corner case: what if the user deletes ten 9mb files. Thea each file is below the limit but will still take up space all together.
I think we can live with this limitation for now, and potentially the
UndoRedo
service could limit the number of items to 20 always.fyi @alexdima who also came up with the idea of this approach