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

Fixes #4741 allow creation of sub-files and/or sub-folders if name has "/" #4764

Merged
merged 1 commit into from
Apr 2, 2019

Conversation

uniibu
Copy link
Contributor

@uniibu uniibu commented Mar 30, 2019

Fixes #4741. Creation of sub-files and/or sub-folders are already available,however, validation check does not allow / on the input box, making it impossible to create nested sub-folders and/or sub-files.

This PR allows recursive creation of folders or files via / .

  • allow recursive creation for New File and New Folder only.
  • do not allow creation of files and folders that starts with /
  • do not allow creation of files and folders that starts and ends with a whitepsace.
  • do not allow recursive create on Rename.

Example:

  • Creating a file using test/scripts/index.js will create test and test/scripts folder if it still does not exists and lastly it creates the file test/scripts/index.js

Signed-off-by: Uni Sayo unibtc@gmail.com

const childUri = new URI(parent.uri).resolve(name).toString();
this.fileSystem.exists(childUri).then(exists => {
if (exists && dialog) {
dialog.error(`A file or folder <strong>${fileName}</strong> already exists at this location.`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should change a type of validation function to MaybPromise<DialogError> to allow async validation. Inside the dialog we will need to make sure that concurrent validation does not occur, i.e. queue them or cancel.

@akosyakov
Copy link
Member

I wonder can we override CtrlCmd + N browser short cut to create a new file, so annoying to click each time.

@akosyakov
Copy link
Member

It does not work if i create the same path twice from the root, it seems to pick the wrong parent then which is very confusing. Not sure whether it is a new bug or something which was not apparent before. In order to reprdoce with Theia repo:

  • right click on theia folder -> New File -> configs/Untitled.txt and create a file
  • do the same again without doing anything else

I would expect an error, but there are non and the second file is created at the bogus location: theia/configs/configs/Untitled.txt

@uniibu
Copy link
Contributor Author

uniibu commented Apr 2, 2019

It does not work if i create the same path twice from the root, it seems to pick the wrong parent then which is very confusing. Not sure whether it is a new bug or something which was not apparent before. In order to reprdoce with Theia repo:

  • right click on theia folder -> New File -> configs/Untitled.txt and create a file
  • do the same again without doing anything else

I would expect an error, but there are non and the second file is created at the bogus location: theia/configs/configs/Untitled.txt

This is because of inconsistent selection bug when creating a new file. Sometimes the new file gets selected on creation (which is what it is supposed to do) and sometimes it doesn't.
This is due to this line:
https://github.com/theia-ide/theia/blob/636533952fd4b42cfc834e346ec3402f0c5db343/packages/filesystem/src/browser/file-resource.ts#L119
which needs to be if (!await this.fileSystem.exists(this.uriString)) {

I've patched this on the latest commit.

@uniibu uniibu force-pushed the recursive-create branch from 1450be2 to 3ba4c30 Compare April 2, 2019 04:11
@@ -116,7 +116,7 @@ export class FileResource implements Resource {
}

protected async getFileStat(): Promise<FileStat | undefined> {
if (!this.fileSystem.exists(this.uriString)) {
if (!await this.fileSystem.exists(this.uriString)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how it used to work! ❤️ for fixing it!

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works nicely now, but there are some code clean up necessary to avoid breaking code org and race conditions.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works nicely, thank you

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @uniibu I didn't expect someone will implement a fix for this issue very soon

This allow creation of files and folders recursive path.
improve validation check, add cancellationtoken

separate validation for absolutpath and leading/trailing spaces

Signed-off-by: Uni Sayo <unibtc@gmail.com>
@uniibu uniibu force-pushed the recursive-create branch from ff5aaef to e49adaf Compare April 2, 2019 10:20
@uniibu
Copy link
Contributor Author

uniibu commented Apr 2, 2019

i've separated the validation checks for absolute paths and leading and trailing spaces as per suggested. Squashed commits.

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks 👍 @uniibu

@benoitf benoitf merged commit b0a7da6 into eclipse-theia:master Apr 2, 2019
@uniibu uniibu deleted the recursive-create branch April 2, 2019 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants