-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix: Fixed an issue where the conflicts dialog didn't append numbers when pasting files #15267
Fix: Fixed an issue where the conflicts dialog didn't append numbers when pasting files #15267
Conversation
I have tried to make changes. Let me know if there are improvements |
This comment has been minimized.
This comment has been minimized.
The copypaste test would need to be updated because it checks for a file named "- Copy". |
Please modify here too
|
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.
Few feedback on the formatting! :)
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
- copy
when duplicate name exists
|
Thank you :) working on it as soon as I get home. |
Understood! Gonna make the necessary changes :) |
I have made a commit. Looks fine for a file but not a directory. working on it mean while let me know if there are any suggestions. |
Fixed filename before extension. Suggested changes.
The last commit seems to be working as expected. Please review and let me know if there are any suggestions. Thank you :) |
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.
Thank you for the suggestions :)
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.
Some suggestions for codebase quality.
Here's too:
#15267 (comment)
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
…uri/Files into fix_numberinsteadcopy
Test might be failing because it apparently copies in same directory. So by default it appends - Copy in the end. |
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.
It works well! There are a few more things in terms of code quality.
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
I was thinking whether I should declare a variable or directly call the function? My only deciding factor was declaring variable in this case makes it more easy to read. What do you suggest? |
Are you talking about |
Oh no no I was saying about var fileName. |
There is no problem as it is 👍 |
…uri/Files into fix_numberinsteadcopy
Alright! Thank you so much! I have committed suggested changes! |
src/Files.App/Utils/Storage/Operations/FileOperationsHelpers.cs
Outdated
Show resolved
Hide resolved
… op.QueueCopyOperation
…uri/Files into fix_numberinsteadcopy
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.
Thank you! LGTM
@ashrafmansuri there are lines with extra spaces, please remove them if you have time. |
Done! Thanks! 👍 |
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.
Nice work! Thank you for contributing to Files.
Thank you! My pleasure! ✌🏻 |
Resolved / Related Issues
Close Bug: Generating new name for conflict is appending "copy" instead of a number "(#)" #11697
Validation
How did you test these changes?
Screenshots (optional)
Add screenshots here.