Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Drag multiple items in tree-view #793

Closed
wants to merge 10 commits into from

Conversation

anderoonies
Copy link
Contributor

@anderoonies anderoonies commented Apr 1, 2016

this is working toward #232, just one bug that needs to be worked out
it functions correctly, as can be seen:
tree-multi

@anderoonies
Copy link
Contributor Author

so it turns out the bug is a result of the directory moving before the files within move.

for initialPath in initialPaths.split(',')
  @moveEntry(initialPath, newDirectoryPath)

is called on each of the entries to move, and so if a dir top/mid/bottom containing 1.txt and 2.txt is moved to top/bottom (same level as mid), the subsequent calls to @moveEntry will reference top/mid/bottom/1.txt and top/mid/bottom/2.txt, an invalid path.

this can be fixed by iterating backward over the paths (for initialPath in initialPaths.split(',') by -1), as they will always be in order of directory -> elements because the querySelectorAll does a DFS on the DOM. thoughts on this fix?

@anderoonies
Copy link
Contributor Author

anderoonies commented May 26, 2016

i just found a bug with dragging files and folders at the same time, i'll likely need to rewrite logic for dragging folders.

@anderoonies
Copy link
Contributor Author

added a fix for the above comment, would appreciate someone else reviewing the code

@Ben3eeE
Copy link
Contributor

Ben3eeE commented May 27, 2016

I'm a bit interested in this so I can do some testing sometime next week earliest. If I can figure out how, last time I tried with another PR tree view failed to activate. But can't do a code review as I don't have any understanding of anything.

@anderoonies
Copy link
Contributor Author

@Ben3eeE are you in the atom slack team? if you send me a direct message there i can help out

@Ben3eeE
Copy link
Contributor

Ben3eeE commented May 30, 2016

@anderoonies Thanks for the help getting it up and running. I have had time to test it out and found a few things.

  • It is not possible to ctrl-click on an already selected item to deselect it. This is possible in Atom 1.7.4 without your PR.
  • If you select a folder and a file inside the folder and start the drag from the file the folder collapses and you are unable to drag and move, see gif.
    move with folder collapses
  • Same as above but dragging from the folder works but shows the selected file in the drag shadow, which is weird in the case more files gets moved since the folder can contain more files.
    drag shadow with file

I don't like that the folder collapses when it gets dragged, it feels unnatural and I don't think other programs do this? Maybe it is just opinion based.

As a side note: The move command (Press m while tree view has focus) still moves a single file, I found #619 for this. I think merging one without the other should be avoided if possible.

@anderoonies
Copy link
Contributor Author

thanks for taking a look at this—I'll rebase which should fix the first issue. collapsing was in an effort to simplify the drag image in a way which does not work, so I'll make a fix to drag image stuff overall. should have updates to this in a day or so

@anderoonies
Copy link
Contributor Author

anderoonies commented May 30, 2016

here are some gifs showing the fixes in the latest commit, @Ben3eeE — solves all that you brought up afaik

meta clicking to deselect
meta

dragging a selected file in a selected folder
folder-file

dragging a selected folder with a contained file selected
folder-folder

@Ben3eeE
Copy link
Contributor

Ben3eeE commented May 31, 2016

@anderoonies looking good. I can give it another round of testing later, let me know when it's ready.

@Ben3eeE
Copy link
Contributor

Ben3eeE commented Jun 22, 2016

I'll rebase which should fix the first issue.

Nope I am still unable to ctrl click to deselect selected items, it does work without this PR.

All other things are i mentioned are fixed.

I am not sure about the drag image. Looking at how other programs work (on Windows) I don't see them listing every file. For example visual studio just shows a small square(Atom has this too) with an icon added to indicate if you copy or move based on modifiers held down. The file explorer shows the file count with the file icon and text indicating if you move, copy or make a shortcut based on modifiers held down.
Maybe all this could be part of a separate enhancement? So there can be some discussions about the exact design. Maybe after or when ctrl-drag to copy is implemented?

@anderoonies
Copy link
Contributor Author

@Ben3eeE i think the meta-clicking is a system issue, then? it's working on darwin. i unfortunately have no way to test it on a windows machine.

@Ben3eeE
Copy link
Contributor

Ben3eeE commented Jun 24, 2016

Deselecting now works with latest commit.

When a directory and its children are being dragged, only drag the directory, not the individual children.
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.
anderoonies and others added 2 commits July 6, 2016 13:24
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.
@0xdevalias
Copy link

Any updates on this?

@0xdevalias 0xdevalias mentioned this pull request Dec 21, 2016
@winstliu
Copy link
Contributor

This needs a rebase in order to tell whether the failing Travis spec is genuine or not.

@mattpilott
Copy link

Should this be fixed now? I am only able to drag a single file into/out of a directory in the tree view. I can select multiple but only one gets moved after i drag and drop.

Even using 1.19.0-beta 5

@UziTech
Copy link
Contributor

UziTech commented Aug 31, 2017

I rebased and fixed this branch at UziTech:multi-drag

I submitted a pull request to anderoonies:multi-drag but I don't know if he is alive.

Should I create a new pull request to atom:master instead?

@UziTech
Copy link
Contributor

UziTech commented Sep 19, 2017

This pull request can be closed.

I added an updated pull request at #1179

@ungb ungb closed this in #1179 Feb 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants