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

Copy/paste not working within folder in Explorer #10767

Merged
merged 1 commit into from
Mar 16, 2022

Conversation

AlexandraBuzila
Copy link
Contributor

What it does

Fixes copy/pasting of a file or folder within the same parent folder in the Explorer, by renaming the target file if the source and target URIs are the same.
Also changes the naming scheme to use spaces instead of underscores when creating a new file, to align with VS Code and user expectations (e.g. on macOS).

Fixes #9923

How to test

Testing copy/paste in the same parent folder:
  1. Copy a file/folder in the Explorer view
  2. Paste in the same folder
  3. A copy of the file/folder should be created (its name should be in the form originalName copy.ext or
    originalName copy index.ext)
Testing the name generation of new files:

Several commands were affected by the change in naming for new files/folders. New files/folders should now have spaces instead of underscores in the generated name portion:

  1. the New File / New Folder commands: from the File menu, select New File: the name suggestion in the dialog that opens should use spaces, e.g. Untitled 2.txt
  2. the Duplicate command: select a file in the Explorer and select Duplicate from its context menu: a copy of the file is created and its name has copy <index> appended to the original file name
  3. copy a file in the Explorer view and paste in into a folder that already contains a file with the same name: a copy of the file is created and its name has copy <index> appended to the original file name

Review checklist

Reminder for reviewers

@msujew msujew self-requested a review February 17, 2022 13:24
@vince-fugnitto vince-fugnitto added the filesystem issues related to the filesystem label Feb 17, 2022
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looking mostly good already. However, I noticed that copying a folder leads to some weird behavior. Instead of creating <original-name> copy, the copied folder is called copy<original-name> instead.

@AlexandraBuzila
Copy link
Contributor Author

AlexandraBuzila commented Feb 18, 2022

However, I noticed that copying a folder leads to some weird behavior. Instead of creating <original-name> copy, the copied folder is called copy<original-name> instead.

I could reproduce the behavior you mention for hidden folders and files, whose names begin with a .. The problem occurs because the uri Path for the resource is returning an empty name and treats the name as an extension. For example for the .theia folder, the name is '' and the ext is .theia. I'm not sure if hidden files are a special use case that we should consider when renaming or if this is rather a bug in Path.

@msujew
Copy link
Member

msujew commented Feb 18, 2022

@AlexandraBuzila I see, I simply used the .theia folder in the repo to test this. It isn't hidden, but that doesn't matter in that case anyway.

I wouldn't say it's in error in the path implementation, files like .gitignore are correct in having an empty name property and an ext value of .gitignore. I would try to align this VSCode in that regard:

  1. Files are copied as <name> copy.<ext>
  2. Files with an empty name are copied as .<ext> copy
  3. Folders are always copied as <basename> copy

@AlexandraBuzila
Copy link
Contributor Author

I rebased and updated the code to handle files and folders that start with a .

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looking better, but copying a copied file results in <filename> copy copy.<ext>. In VSCode copying <filename> copy.<ext> leads to creating <filename> copy 2.<ext> instead. I believe we should align to VSCode's behavior.

@AlexandraBuzila
Copy link
Contributor Author

copying a copied file results in <filename> copy copy.<ext>. In VSCode copying <filename> copy.<ext> leads to creating <filename> copy 2.<ext> instead. I believe we should align to VSCode's behavior.

Should be fixed now

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Almost working correctly now. I believe this point hasn't been resolved correctly:

Folders are always copied as <basename> copy

Because naming a folder test.theia and copying it, leads to the new file name test copy.theia. In VSCode this will be test.theia copy instead.

@AlexandraBuzila
Copy link
Contributor Author

I believe this point hasn't been resolved correctly:

Folders are always copied as <basename> copy

You're right, I missed that. It should be fixed now. I also added some test cases.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looking good now!

  • Copying files (with and without names) works as in VSCode
  • Copying folders works as in VSCode

Please address my documentation related suggestions and I'll merge before the release!

packages/filesystem/src/common/filesystem-utils.ts Outdated Show resolved Hide resolved
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Oh well, I didn't notice the test failure on Windows. Some test in navigator.spec.js is failing on Windows. Do you mind addressing it?

If you don't have access to a Windows system to reproduce the issue, I will look into it next week.

@AlexandraBuzila
Copy link
Contributor Author

AlexandraBuzila commented Feb 25, 2022

Oh well, I didn't notice the test failure on Windows. Some test in navigator.spec.js is failing on Windows. Do you mind addressing it?

I can have a look at it at the beginning of next week.

@AlexandraBuzila
Copy link
Contributor Author

Some test in navigator.spec.js is failing on Windows. Do you mind addressing it?

The targetUri computed in the copy method from file-tree-model.ts was not correct, it should be fixed now. There was another issue that I previously missed: pasting a folder on itself was not behaving correctly. It should be fixed now as well.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thanks, looking well now! The behavior is perfectly aligned to VSCode now:

  • Copying a normal file suffixes copy (<number>) to its filename.
  • Copying a file without a filename, suffixes copy (<number>) to its fullname.
  • Copying a folder suffixes the fullname as well.
  • Copying a folder into itself will result in a copied sibling folder.

@msujew
Copy link
Member

msujew commented Mar 15, 2022

@AlexandraBuzila There seem to be some conflicts (although GitHub isn't willing to show me what it is - I suspect the changelog). Do you mind resolving them? I'll merge this afterwards.

* treat paste on parent directory same as paste on copied file
* fix paste of folder on self
* use spaces instead of underscores as separators inside file names for
newly created files/folders
* fix unique name generation for dotfiles/folders

Signed-off-by: Johannes Faltermeier <[email protected]>
Signed-off-by: Alexandra Buzila <[email protected]>
@AlexandraBuzila
Copy link
Contributor Author

@AlexandraBuzila There seem to be some conflicts (although GitHub isn't willing to show me what it is - I suspect the changelog). Do you mind resolving them? I'll merge this afterwards.

I rebased the changes and fixed the conflict (it was indeed the changelog).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filesystem issues related to the filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy/paste not working within folder in Explorer
4 participants