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

Drag multiple items in tree-view #1179

Merged
merged 37 commits into from Feb 10, 2018

Conversation

Projects
None yet
10 participants
@UziTech
Contributor

UziTech commented Sep 10, 2017

Description of the Change

  • Allow multiple items to be dragged to a folder. This is a rebase of #793

multi-drag screenshot

  • Allow multi-select to work like it is supposed to

shift + ctrl + click screenshot

Benefits

Multi-select will work like other file explorers

Possible Drawbacks

none

Applicable Issues

Fixes #232
Supersedes and closes #793

@50Wliu 50Wliu added the needs-review label Sep 10, 2017

@50Wliu 50Wliu self-assigned this Sep 10, 2017

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 10, 2017

Member

@UziTech what is the purpose of 0f7a4b7?

Member

50Wliu commented Sep 10, 2017

@UziTech what is the purpose of 0f7a4b7?

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Sep 10, 2017

Contributor

@50Wliu

When trying to drag multiple files you click and hold on one of the selected files and drag to a folder.

When selecting only a single selected file when multiple are selected you just click on the selected file.

We need to make sure the user is not dragging before deselecting all files except for the one that is clicked on. In order to do that we need to do the deselect on mouseup.

We still want most mouse operations to occur on mousedown. Only when we need to make sure there is no dragging do we do the operation on mouseup.

Contributor

UziTech commented Sep 10, 2017

@50Wliu

When trying to drag multiple files you click and hold on one of the selected files and drag to a folder.

When selecting only a single selected file when multiple are selected you just click on the selected file.

We need to make sure the user is not dragging before deselecting all files except for the one that is clicked on. In order to do that we need to do the deselect on mouseup.

We still want most mouse operations to occur on mousedown. Only when we need to make sure there is no dragging do we do the operation on mouseup.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Sep 16, 2017

Member

I'm signing up for testing here since I tested the original PR.

Member

Ben3eeE commented Sep 16, 2017

I'm signing up for testing here since I tested the original PR.

@andyfleming

This comment has been minimized.

Show comment
Hide comment
@andyfleming

andyfleming Sep 23, 2017

Great work! Let's get this done!

I can't believe we can't drag multiple files in atom yet 😆 .

andyfleming commented Sep 23, 2017

Great work! Let's get this done!

I can't believe we can't drag multiple files in atom yet 😆 .

@UziTech UziTech referenced this pull request Sep 25, 2017

Closed

rebase Multi drag #1

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Oct 6, 2017

Contributor

@Ben3eeE anything holding this up?

Contributor

UziTech commented Oct 6, 2017

@Ben3eeE anything holding this up?

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 6, 2017

Member

@UziTech Sorry been swamped with other things. I will take a look as soon as I get a chance to, hopefully I will have some time this weekend ⚡️

Member

Ben3eeE commented Oct 6, 2017

@UziTech Sorry been swamped with other things. I will take a look as soon as I get a chance to, hopefully I will have some time this weekend ⚡️

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Oct 19, 2017

Contributor

I rebased again and fixed the merge conflicts in the tests

Contributor

UziTech commented Oct 19, 2017

I rebased again and fixed the merge conflicts in the tests

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Oct 27, 2017

Contributor

@Ben3eeE Any way we can get this out the door so I don't have to keep rebasing?

Contributor

UziTech commented Oct 27, 2017

@Ben3eeE Any way we can get this out the door so I don't have to keep rebasing?

@arthurlacoste

This comment has been minimized.

Show comment
Hide comment
@arthurlacoste

arthurlacoste Nov 8, 2017

Please, merge this feature !

arthurlacoste commented Nov 8, 2017

Please, merge this feature !

anderoonies and others added some commits Mar 31, 2016

Don't drag dir entries on multi-drag
When a directory and its children are being dragged, only drag the directory, not the individual children.
spec dragging a directory and files to a new directory
this test guarantees that dragging a directory d and additional files f into a new directory will move the entire directory d discreetly along with any additional files f.
refactor drag image, clicking logic
re-enable a metaclick to allow an item to be selected from the multi-selection. change the behavior of drag image building so that there's no collapsing and properly deselects the drag image.
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Jan 29, 2018

Member

One other case I came across that doesn't work but probably should is dropping a selection on a file. I would expect that the selection would be moved into the same folder as the file that is being dropped on, but right now the move operation is just canceled when dropping on a file.

Tested this quick and it appears to also be an issue on master so it's not going to block this PR from moving forward. Do you want to create an issue for this?

Member

Ben3eeE commented Jan 29, 2018

One other case I came across that doesn't work but probably should is dropping a selection on a file. I would expect that the selection would be moved into the same folder as the file that is being dropped on, but right now the move operation is just canceled when dropping on a file.

Tested this quick and it appears to also be an issue on master so it's not going to block this PR from moving forward. Do you want to create an issue for this?

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Jan 29, 2018

Contributor

I just added it to this pr

Contributor

UziTech commented Jan 29, 2018

I just added it to this pr

Show outdated Hide outdated lib/tree-view.coffee
Show outdated Hide outdated lib/tree-view.coffee
Show outdated Hide outdated lib/tree-view.coffee
Show outdated Hide outdated lib/tree-view.coffee
Show outdated Hide outdated lib/tree-view.coffee
Show outdated Hide outdated lib/tree-view.coffee
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jan 29, 2018

Member

Going back to your comment here: #1179 (comment)

Perhaps click could be used to combine the two cases?

Member

50Wliu commented Jan 29, 2018

Going back to your comment here: #1179 (comment)

Perhaps click could be used to combine the two cases?

@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Jan 29, 2018

Contributor

I could use click but I would still need a variable letting the onClick function know if it should execute otherwise it would deselect a file after selecting it on mousedown. And we do want to use the mousedown function for selecting because someone might want to ctrl+click then just start dragging.

screenshot

Contributor

UziTech commented Jan 29, 2018

I could use click but I would still need a variable letting the onClick function know if it should execute otherwise it would deselect a file after selecting it on mousedown. And we do want to use the mousedown function for selecting because someone might want to ctrl+click then just start dragging.

screenshot

@UziTech UziTech referenced this pull request Feb 2, 2018

Merged

use path.join #1221

@adamreisnz

This comment has been minimized.

Show comment
Hide comment
@adamreisnz

adamreisnz Feb 3, 2018

Would be great to have this 👍

adamreisnz commented Feb 3, 2018

Would be great to have this 👍

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Feb 3, 2018

Member

Code changes look good to me. @Ben3eeE if you're good with this as well I think it can 🚢?

Member

50Wliu commented Feb 3, 2018

Code changes look good to me. @Ben3eeE if you're good with this as well I think it can 🚢?

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Feb 6, 2018

Member

@UziTech All good from me when testing on Windows 👍 @ungb is going to go over the test plan on macOS and Linux and merge if all is good.

Member

Ben3eeE commented Feb 6, 2018

@UziTech All good from me when testing on Windows 👍 @ungb is going to go over the test plan on macOS and Linux and merge if all is good.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Feb 6, 2018

Contributor

I'm looking into this now. going off test plan in #1179 (comment). I'll merge this once I'm done

Contributor

ungb commented Feb 6, 2018

I'm looking into this now. going off test plan in #1179 (comment). I'll merge this once I'm done

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Feb 7, 2018

Contributor

I ran into the following while dragging between two atom windows.

See gif for repro.

error

Repro steps:
Run the following command to create the folders and file

mkdir test1 test2
cd test1 && touch a b c && cd ../test2 && touch a b c && cd ..
atom test1 && atom test2
  1. Drag file a from folder test1 to folder test2 in the other atom window

Atom: 1.23.3 x64
Electron: 1.6.15
OS: Mac OS X 10.12.6
Thrown From: tree-view package 0.222.0

Stack Trace

Uncaught TypeError: Cannot read property 'collapse' of null

At /Users/ungb/github/packages/tree-view/lib/tree-view.coffee:1126

TypeError: Cannot read property 'collapse' of null
    at TreeView.module.exports.TreeView.onDrop (/packages/tree-view/lib/tree-view.coffee:1126:9)
    at /packages/tree-view/lib/tree-view.coffee:206:47)
Contributor

ungb commented Feb 7, 2018

I ran into the following while dragging between two atom windows.

See gif for repro.

error

Repro steps:
Run the following command to create the folders and file

mkdir test1 test2
cd test1 && touch a b c && cd ../test2 && touch a b c && cd ..
atom test1 && atom test2
  1. Drag file a from folder test1 to folder test2 in the other atom window

Atom: 1.23.3 x64
Electron: 1.6.15
OS: Mac OS X 10.12.6
Thrown From: tree-view package 0.222.0

Stack Trace

Uncaught TypeError: Cannot read property 'collapse' of null

At /Users/ungb/github/packages/tree-view/lib/tree-view.coffee:1126

TypeError: Cannot read property 'collapse' of null
    at TreeView.module.exports.TreeView.onDrop (/packages/tree-view/lib/tree-view.coffee:1126:9)
    at /packages/tree-view/lib/tree-view.coffee:206:47)
@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Feb 7, 2018

Contributor

Nice catch

Contributor

UziTech commented Feb 7, 2018

Nice catch

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Feb 7, 2018

Contributor

Ctrl+click doesn't work on linux.. it seems to just open up the context menu. Shift works fine.
image

@Ben3eeE does this work on windows? I can spend some time testing windows today as well.

After the latest change, everything works great on mac!

Contributor

ungb commented Feb 7, 2018

Ctrl+click doesn't work on linux.. it seems to just open up the context menu. Shift works fine.
image

@Ben3eeE does this work on windows? I can spend some time testing windows today as well.

After the latest change, everything works great on mac!

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Feb 7, 2018

Member

@Ben3eeE does this work on windows?

@ungb It did when I tested. I have not tested on the most recent commit however.

Member

Ben3eeE commented Feb 7, 2018

@Ben3eeE does this work on windows?

@ungb It did when I tested. I have not tested on the most recent commit however.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Feb 9, 2018

Contributor

@Ben3eeE I think my issues is because I'm running on a VM on mac and it's doing something weird. I'll take a look when I get home on my windows machine and then try to see if I spin up a VM on there for linux. So far everything looks good, but every time I hit ctrl+click it opens up the context menu. I'm noticing this on my VM and happens on the desktop on windows too.

Contributor

ungb commented Feb 9, 2018

@Ben3eeE I think my issues is because I'm running on a VM on mac and it's doing something weird. I'll take a look when I get home on my windows machine and then try to see if I spin up a VM on there for linux. So far everything looks good, but every time I hit ctrl+click it opens up the context menu. I'm noticing this on my VM and happens on the desktop on windows too.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Feb 9, 2018

Member

@ungb I have Ubuntu installed so I can test this on Linux if you run into more issues because of testing in a VM.

Member

Ben3eeE commented Feb 9, 2018

@ungb I have Ubuntu installed so I can test this on Linux if you run into more issues because of testing in a VM.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Feb 10, 2018

Contributor

I've tested this and everything looks good to me. I'm going to merge this. I'm going to follow up with the team later on the appveyor failure so we can get this into master.

Contributor

ungb commented Feb 10, 2018

I've tested this and everything looks good to me. I'm going to merge this. I'm going to follow up with the team later on the appveyor failure so we can get this into master.

@ungb ungb merged commit 3424331 into atom:master Feb 10, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@UziTech

This comment has been minimized.

Show comment
Hide comment
@UziTech

UziTech Feb 10, 2018

Contributor

hallelujah-gif-15

Contributor

UziTech commented Feb 10, 2018

hallelujah-gif-15

@jonathan-dejong

This comment has been minimized.

Show comment
Hide comment
@jonathan-dejong

jonathan-dejong May 8, 2018

Any news on this?
From what I can tell with Atom 1.26.1 it is not merged yet.

jonathan-dejong commented May 8, 2018

Any news on this?
From what I can tell with Atom 1.26.1 it is not merged yet.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE May 8, 2018

Member

@jonathan-dejong This is merged but it's not released yet because the tests are failing on atom/atom when upgrading the tree-view. You may want to subscribe to atom/atom#17088 for updates on when it will be available in a released version of Atom.

Member

Ben3eeE commented May 8, 2018

@jonathan-dejong This is merged but it's not released yet because the tests are failing on atom/atom when upgrading the tree-view. You may want to subscribe to atom/atom#17088 for updates on when it will be available in a released version of Atom.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment