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

Fix: Fixed an issue where the conflicts dialog didn't append numbers when pasting files #15267

Merged
merged 21 commits into from
May 5, 2024

Conversation

ashrafmansuri
Copy link
Contributor

@ashrafmansuri ashrafmansuri commented Apr 28, 2024

Resolved / Related Issues

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    1. Open app ...
    2. Copy a file or folder from a similar source and destination the filename has -Copy and file behaves similar to Explorer ...
    3. Now copy a file or folder from a source different than destination the filename has (#) appended to it.

Screenshots (optional)
Add screenshots here.

@ashrafmansuri
Copy link
Contributor Author

I have tried to make changes. Let me know if there are improvements

@0x5bfa

This comment has been minimized.

@gave92
Copy link
Member

gave92 commented Apr 28, 2024

The copypaste test would need to be updated because it checks for a file named "- Copy".

@0x5bfa
Copy link
Member

0x5bfa commented Apr 28, 2024

Please modify here too

TestHelper.InvokeButtonByName("Renamed Folder - Copy");

Copy link
Contributor

@QuaintMako QuaintMako left a 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! :)

@Josh65-2201 Josh65-2201 changed the title Fix numberinsteadcopy - #11697 Fix: Use number instead of - copy when duplicate name exists Apr 28, 2024
@hishitetsu
Copy link
Member

  • Your PR appends the number after the file extension, but it should be appended before the file extension.
  • If you want to match the behavior with Explorer, the hyphen in front of the number is not necessary.
  • This change should only be applied when copying files to a folder different from the source. The behavior when pasting into the same folder should not be changed from the current behavior. (- Copy, - Copy (2), - Copy (3), ...)

@ashrafmansuri
Copy link
Contributor Author

Few feedback on the formatting! :)

Thank you :) working on it as soon as I get home.

@ashrafmansuri
Copy link
Contributor Author

  • Your PR appends the number after the file extension, but it should be appended before the file extension.

  • If you want to match the behavior with Explorer, the hyphen in front of the number is not necessary.

  • This change should only be applied when copying files to a folder different from the source. The behavior when pasting into the same folder should not be changed from the current behavior. (- Copy, - Copy (2), - Copy (3), ...)

Understood! Gonna make the necessary changes :)

@Josh65-2201 Josh65-2201 added changes requested Changes are needed for this pull request and removed needs - code review labels Apr 30, 2024
@ashrafmansuri
Copy link
Contributor Author

ashrafmansuri commented Apr 30, 2024

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.
@ashrafmansuri
Copy link
Contributor Author

The last commit seems to be working as expected. Please review and let me know if there are any suggestions. Thank you :)

Copy link
Contributor Author

@ashrafmansuri ashrafmansuri left a 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 :)

@0x5bfa 0x5bfa added needs - code review and removed changes requested Changes are needed for this pull request labels May 1, 2024
Copy link
Member

@0x5bfa 0x5bfa left a 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)

@ashrafmansuri
Copy link
Contributor Author

Test might be failing because it apparently copies in same directory. So by default it appends - Copy in the end.

@hishitetsu hishitetsu added needs - code review and removed changes requested Changes are needed for this pull request labels May 5, 2024
Copy link
Member

@hishitetsu hishitetsu 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 well! There are a few more things in terms of code quality.

@ashrafmansuri
Copy link
Contributor Author

It works well! There are a few more things in terms of code quality.

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?

@hishitetsu
Copy link
Member

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 Func<int, string> genFilePath? I think it's fine.

@ashrafmansuri
Copy link
Contributor Author

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 Func<int, string> genFilePath? I think it's fine.

Oh no no I was saying about var fileName.

@hishitetsu
Copy link
Member

Oh no no I was saying about var fileName.

There is no problem as it is 👍

@ashrafmansuri
Copy link
Contributor Author

Oh no no I was saying about var fileName.

There is no problem as it is 👍

Alright! Thank you so much! I have committed suggested changes!

Copy link
Member

@hishitetsu hishitetsu left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@hishitetsu
Copy link
Member

@ashrafmansuri there are lines with extra spaces, please remove them if you have time.
image

@ashrafmansuri
Copy link
Contributor Author

@ashrafmansuri there are lines with extra spaces, please remove them if you have time. image

Done! Thanks! 👍

Copy link
Member

@yaira2 yaira2 left a 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.

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels May 5, 2024
@yaira2 yaira2 changed the title Fix: Fixed an issue where the conflicts dialog wasn't appending numbers when pasting files Fix: Fixed an issue where the conflicts dialog didn't append numbers when pasting files May 5, 2024
@yaira2 yaira2 merged commit 994f058 into files-community:main May 5, 2024
6 checks passed
@ashrafmansuri
Copy link
Contributor Author

Nice work! Thank you for contributing to Files.

Thank you! My pleasure! ✌🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Generating new name for conflict is appending "copy" instead of a number "(#)"
7 participants