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 files on drop if control is pressed #1257

Merged
merged 16 commits into from May 14, 2019

Conversation

Projects
None yet
3 participants
@50Wliu
Copy link
Member

commented Jun 20, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

For consistency with OS behavior, copy files when dropping if control is pressed, rather than moving them.

Alternate Designs

None?

Benefits

Consistency with file managers such as Windows Explorer.

Possible Drawbacks

Unknown

Applicable Issues

Closes #703
Supersedes and closes #800

  • Make it work
    • Add specs
  • Allow files to be copied to the same directory
    • Add specs
  • Regression testing

50Wliu added some commits May 10, 2018

50Wliu added some commits Jun 20, 2018

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2018

Everything seems to be working as expected. I will have a checklist of what I tested soon.

While implementing this feature, I noticed that there was already code present to copy entries in pasteEntries, so I refactored pasteEntries to extract that logic into a standalone function that the drag-and-drop code could use as well.

50Wliu added some commits Jul 12, 2018

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2018

Verification checklist:

  • Copy and pasting a file to the same directory pastes the file with a 0 behind the basename
  • Cut and pasting a file to the same directory pastes the file (you should notice no change)
  • Copy and pasting a directory to the same directory pastes the directory with a 0 appended (recursive copy)
  • Cut and pasting a directory to the same directory pastes the directory (you should notice no change)
  • Copy and pasting a file and directory to the same directory pastes both successfully, with numbers appended as appropriate
  • Cut and pasting a file and directory to the same directory pastes both successfully (you should notice no change)
  • Copy and pasting a file to a different directory pastes the file succesfully
  • Cut and pasting a file to a different directory pastes the file successfully and removes the file from the original location
  • Copy and pasting a directory to a different directory pastes the directory successfully
  • Cut and pasting a directory to a different directory pastes the directory successfully
  • Copy and pasting a file and directory to a different directory pastes both successfully
  • Cut and pasting a file and directory to a different directory pastes both successfully

Now repeat the above with drag-and-drop (replacing "copy" with ctrl-drag, and "cut" with drag).

  • Windows
  • macOS
  • Linux
@UziTech

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

It would be great to get this in the next version of atom along with the other multi-drag improvements 👍

50Wliu added some commits Mar 21, 2019

@nathansobo nathansobo self-assigned this May 10, 2019

@nathansobo
Copy link
Contributor

left a comment

This looks great. Thorough test coverage, and pulling out the copyEntry helper was a good move. I notice a bunch of verification checkboxes are incomplete in one of your comments. Does that reflect the current status of things, or have you verified that everything is working? If so, 🚢 it.

@@ -3831,6 +3830,18 @@ describe "TreeView", ->
[alphaDirPath, alphaFilePath, betaFilePath, etaDirPath, gammaDirPath, deltaFilePath, epsilonFilePath, thetaFilePath] = []

beforeEach ->
# tree-view

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 10, 2019

Contributor

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

Does that reflect the current status of things, or have you verified that everything is working?

@nathansobo I've verified that everything's working on Windows, but not on macOS or Linux.

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@50Wliu It looks like the tests are hanging after merging in master. This seems to be due to a dialog prompt when pasting files that conflict with existing directory entries. Maybe something changed on master causing the prompt to be displayed in a situation where it wasn't previously. We'll kick this back to you, since you have more context. Please feel free to ping me again when you get a green build.

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Ugh, dialog prompts. I'm guessing something to do with #1180.

@50Wliu 50Wliu force-pushed the wl-copy-on-ctrl branch from 5e67579 to 9dce9f2 May 14, 2019

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

@nathansobo All right, specs are 💚 again.

What was the problem?

In this PR, I extracted much of the logic in pasteEntries to the helper methods copyEntry and moveEntry. Simultaneously, in #1180, I updated moveEntry to show a dialog box on conflicting entries. Prior to this PR, moveEntry was only used for drag-and-drop, and so the regular cut/paste specs were still expecting to error on conflicting entries. Now that all the move logic is centralized in one function, those specs began to hang due to the appearance of an unexpected dialog prompt.

The fix

I've essentially copied over all the specs I wrote in #1180 and adjusted them for the cut functions. Yes, that means we now have (almost) duplicate spec coverage for moveEntry, and we should probably rewrite those specs to remove the duplication. However, that would be out of the scope of this PR.


Realistically, I should have realized in #1180 that the changes I were making were only applying to drag-and-drop and not cut/paste...the two should be consistent, and now they are.

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I've tested everything manually on macOS per your checklist. Since this all relies on Node APIs to interface with the file system and we have thorough test coverage, I'm going to skip testing on Linux because I can't see how things could be so different on that platform that this would be broken while still building green on Travis. Perhaps famous last words.

@nathansobo nathansobo merged commit 573bb39 into master May 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the wl-copy-on-ctrl branch May 14, 2019

@nathansobo

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Thanks for your stewardship @50Wliu! ❤️ ⚡️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.