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

Workspace: Rename files/folder is case sensitive #9709

Merged

Conversation

OmarSdt-EC
Copy link
Contributor

@OmarSdt-EC OmarSdt-EC commented Jul 7, 2021

What it does

Fixes #9663
This commit fixes the rename command for windows users by making possible
the case sensitive rename of files and folders. Also, it adds the test file
workspace-commands.spec.ts which verifies that the implementation works as expected.

How to test

If your operating system is Windows, you can test this commit with the following steps in the browser or electron target :

  1. Create a new folder : foo
  2. Create two files in it : a.txt and b.txt
  3. Rename a.txt to A.txt
  4. It should let you do the rename
  5. Rename b.txt to a.txt
  6. It should not let you do the rename and the message : "A file or folder 'a.txt' already exists at this location" should appear

You can also test the commit by running :
npx run build @theia/workspace && npx run test @theia/workspace

Review checklist

Reminder for reviewers

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.

I tested the changes on Windows and Ubuntu. The behavior on Ubuntu didn't change, while it fixed the issue on Windows. I tried:

  • Renaming a file to the same name but with a different case.
  • Renaming a file to create a name collision with an existing file with the same case as the other file.
  • Renaming a file to create a name collision with an existing file with a different case.

LGTM 👍

@OmarSdt-EC OmarSdt-EC force-pushed the os/fix_caseInsensitive branch 15 times, most recently from fb824ec to 28280d1 Compare July 9, 2021 21:05
@tsmaeder tsmaeder requested a review from vinokurig July 14, 2021 14:07
@OmarSdt-EC OmarSdt-EC force-pushed the os/fix_caseInsensitive branch from 28280d1 to 945537d Compare July 19, 2021 13:27
@vinokurig
Copy link
Contributor

The code is OK for me but I can't approve it because I don't have a Windows workstation to test it.

@OmarSdt-EC OmarSdt-EC force-pushed the os/fix_caseInsensitive branch from 945537d to c85774e Compare July 19, 2021 21:58
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.

I tested everything again, as the code changed a lot since the last time I took a look at it. However, everything still performs as expected.

@OmarSdt-EC OmarSdt-EC force-pushed the os/fix_caseInsensitive branch from c85774e to 66a05e1 Compare July 20, 2021 19:15
@msujew
Copy link
Member

msujew commented Aug 4, 2021

@paul-marechal Anything blocking you from approving/retesting?

@OmarSdt-EC OmarSdt-EC force-pushed the os/fix_caseInsensitive branch from 66a05e1 to 33f5fb1 Compare August 4, 2021 17:13
This commit fixes the rename command for windows users by making possible
the case sensitive rename of files and folders. Also, it adds the test file
workspace-commands.spec.ts which verifies that the implementation works as expected.
@OmarSdt-EC OmarSdt-EC force-pushed the os/fix_caseInsensitive branch from 33f5fb1 to 52e4d30 Compare August 4, 2021 17:19
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

Code LGTM, and I confirm that I am now able to rename files by just changing the case on Windows.

@paul-marechal paul-marechal merged commit f776392 into eclipse-theia:master Aug 4, 2021
@vince-fugnitto vince-fugnitto mentioned this pull request Aug 13, 2021
1 task
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.

Rename of files and folder are case insensitive
4 participants