Allow reordering project folders via drag and drop #525

Merged
merged 29 commits into from Oct 26, 2016

Conversation

Projects
None yet
@forivall
Contributor

forivall commented Jul 18, 2015

Fixes #691

This uses the same logic that moving tabs uses to reposition project folders within the tree view.

move-project-folders

  • implement
  • update to interoperate with file movement dragging
  • fix placeholder width
  • add tests

Future additions:

  • reduce amount of duplicated code - needs coordination with tabs-view and drag-and-drop file moving
  • allow dragging in folders from the filesystem - needs electron support first, unfortunately. Or it's just broken in kwin.

Bugfixes

  • When dragging project folders, don't allow movement of the folder itself
  • Allow movement below last project folder (#525 (comment)) - should also cover when dragging to an empty tree view
  • Make sure placeholder (blue bar) is visible when dragging above first project (#525 (comment))

@izuzak izuzak referenced this pull request in atom/atom Aug 27, 2015

Closed

Manually change order of Project folders #8538

@alex88

This comment has been minimized.

Show comment
Hide comment
@alex88

alex88 Sep 4, 2015

+1 this would be great, specially with a lot of folders not alphabetically ordered

alex88 commented Sep 4, 2015

+1 this would be great, specially with a lot of folders not alphabetically ordered

@50Wliu 50Wliu added the needs-review label Sep 4, 2015

@forivall

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Sep 4, 2015

Contributor

I've updated this to work with the drag and drop changes in master. The semantics are that project folder dragging will occur if the drop occurs over the top half of a project folder entry, or over a file entry. The placeholder will appear if and only if project folder movement will occur, otherwise, selection will appear.

Alternate semantics that I've thought about are that file movement should only occur if the user is holding down the shift key, similar to many file managers.

Contributor

forivall commented Sep 4, 2015

I've updated this to work with the drag and drop changes in master. The semantics are that project folder dragging will occur if the drop occurs over the top half of a project folder entry, or over a file entry. The placeholder will appear if and only if project folder movement will occur, otherwise, selection will appear.

Alternate semantics that I've thought about are that file movement should only occur if the user is holding down the shift key, similar to many file managers.

forivall referenced this pull request in danielbrodin/atom-project-manager Sep 20, 2015

@brunogaspar

This comment has been minimized.

Show comment
Hide comment

+1

lib/helpers.coffee
@@ -15,3 +15,11 @@ module.exports =
camelizedAttr = property.replace /\-([a-z])/g, (a, b) -> b.toUpperCase()
styleObject[camelizedAttr] = value
styleObject
+
+ ensureOpaqueBackground: (styleObject) ->

This comment has been minimized.

@50Wliu

50Wliu Nov 17, 2015

Member

What is this for?

@50Wliu

50Wliu Nov 17, 2015

Member

What is this for?

This comment has been minimized.

@forivall

forivall Nov 17, 2015

Contributor

It's from this PR: #565 ; I accidentally merged it in at some point. I can rebase it out if needed.

@forivall

forivall Nov 17, 2015

Contributor

It's from this PR: #565 ; I accidentally merged it in at some point. I can rebase it out if needed.

lib/project-folder-dnd-handler.coffee
+ event.originalEvent.dataTransfer.setData 'project-root-index', projectRoot.index()
+
+ rootIndex = -1
+ _.find(@treeView.roots, (root, index) -> root.directory is directory and ((rootIndex = index) or true))

This comment has been minimized.

@50Wliu

50Wliu Nov 17, 2015

Member

(rootIndex = index) or true) I might be missing something, but won't that always evaluate to true? Is it just to assign rootIndex?

@50Wliu

50Wliu Nov 17, 2015

Member

(rootIndex = index) or true) I might be missing something, but won't that always evaluate to true? Is it just to assign rootIndex?

This comment has been minimized.

@forivall

forivall Nov 17, 2015

Contributor

It's an overly clever way to do _.first, while also returning the index. I should replace it with a for loop, since that would make it clearer.

@forivall

forivall Nov 17, 2015

Contributor

It's an overly clever way to do _.first, while also returning the index. I should replace it with a for loop, since that would make it clearer.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 17, 2015

Member

Can you please rebase on top of master so that only the changes specific to this PR get displayed in the diff? It gets confusing reviewing when there's changes from other, unrelated PRs.

Member

50Wliu commented Nov 17, 2015

Can you please rebase on top of master so that only the changes specific to this PR get displayed in the diff? It gets confusing reviewing when there's changes from other, unrelated PRs.

@forivall

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Nov 17, 2015

Contributor

@50Wliu Rebased. The other PR isn't 100% unrelated, but the fix is disjoint. 😉

Contributor

forivall commented Nov 17, 2015

@50Wliu Rebased. The other PR isn't 100% unrelated, but the fix is disjoint. 😉

@marjakm

This comment has been minimized.

Show comment
Hide comment

marjakm commented Jan 22, 2016

+1

@forivall

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Feb 3, 2016

Contributor

Rebased and updated. Tests all work.

@50Wliu ping!

Contributor

forivall commented Feb 3, 2016

Rebased and updated. Tests all work.

@50Wliu ping!

@50Wliu

This comment has been minimized.

Show comment
Hide comment
Member

50Wliu commented Feb 3, 2016

/cc @atom/feedback

@mxstbr

This comment has been minimized.

Show comment
Hide comment
@mxstbr

mxstbr Apr 14, 2016

Is there a particular reason this wasn't merged yet? I'd love to have this feature.

mxstbr commented Apr 14, 2016

Is there a particular reason this wasn't merged yet? I'd love to have this feature.

@krzysztof-sikorski

This comment has been minimized.

Show comment
Hide comment
@krzysztof-sikorski

krzysztof-sikorski Apr 18, 2016

Please make this behavior optional (toggled by a setting). Too many times I had accidentally dragged an entry instead of clicking on it.

Please make this behavior optional (toggled by a setting). Too many times I had accidentally dragged an entry instead of clicking on it.

@anothernode

This comment has been minimized.

Show comment
Hide comment
@anothernode

anothernode Apr 18, 2016

@mxstbr I guess someone with the proper authorization needs to review and approve the code (there's a "needs-review" label attached).

@mxstbr I guess someone with the proper authorization needs to review and approve the code (there's a "needs-review" label attached).

@thedaniel

This comment has been minimized.

Show comment
Hide comment
@thedaniel

thedaniel May 2, 2016

Contributor

Sorry about the delay. I linked this locally to test and found this issue right away: if you open the editor with a single project, and drag that project and drop it onto one of its own subfolders, an error is displayed. We need to not only do nothing on drop in this case, we shouldn't be highlighting / selecting tree view entries as the project is dragged over them.

screen shot 2016-05-02 at 5 08 07 pm

Contributor

thedaniel commented May 2, 2016

Sorry about the delay. I linked this locally to test and found this issue right away: if you open the editor with a single project, and drag that project and drop it onto one of its own subfolders, an error is displayed. We need to not only do nothing on drop in this case, we shouldn't be highlighting / selecting tree view entries as the project is dragged over them.

screen shot 2016-05-02 at 5 08 07 pm

@mxstbr

This comment has been minimized.

Show comment
Hide comment
@mxstbr

mxstbr May 19, 2016

@forivall any update to the above bug report?

mxstbr commented May 19, 2016

@forivall any update to the above bug report?

@forivall

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall May 19, 2016

Contributor

@mxstbr haven't had time yet, was focused on #582

Contributor

forivall commented May 19, 2016

@mxstbr haven't had time yet, was focused on #582

@heruan

This comment has been minimized.

Show comment
Hide comment
@heruan

heruan Aug 3, 2016

Is this still under review? I'd love to have it on the next release!

heruan commented Aug 3, 2016

Is this still under review? I'd love to have it on the next release!

@50Wliu 50Wliu added requires-changes and removed needs-review labels Aug 3, 2016

@flcoder

This comment has been minimized.

Show comment
Hide comment
@flcoder

flcoder Oct 3, 2016

I really want this... I haven't looked at the code. Should I bother attempting to work on this or is resolution coming soon?

flcoder commented Oct 3, 2016

I really want this... I haven't looked at the code. Should I bother attempting to work on this or is resolution coming soon?

@forivall

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Oct 4, 2016

Contributor

@flcoder My bad, I'll update this within a week. Hopefully tonight.

Contributor

forivall commented Oct 4, 2016

@flcoder My bad, I'll update this within a week. Hopefully tonight.

@forivall

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Oct 6, 2016

Contributor

@thedaniel that's actually an issue inherited from the drag-drop used to move files/folders. I'll create a separate PR to fix that, since I understand the drag-drop code, but it's unrelated to the issue of rearranging project folders.

There is the question of: should project folders be movable into other project folders or not? Let me know if y'all need a more in depth explanation.

Contributor

forivall commented Oct 6, 2016

@thedaniel that's actually an issue inherited from the drag-drop used to move files/folders. I'll create a separate PR to fix that, since I understand the drag-drop code, but it's unrelated to the issue of rearranging project folders.

There is the question of: should project folders be movable into other project folders or not? Let me know if y'all need a more in depth explanation.

@garretwilson

This comment has been minimized.

Show comment
Hide comment
@garretwilson

garretwilson Oct 6, 2016

should project folders be movable into other project folders or not?

Maybe they should. Maybe they shouldn't. But let's not complicate things. We all agree we should be able to rearrange the "projects" in Atom. I say put the rest in another issue. If you're not careful, there will be a big back-and-forth about the "extra" features, and another year will pass before we get even the part that everybody is in agreement on.

should project folders be movable into other project folders or not?

Maybe they should. Maybe they shouldn't. But let's not complicate things. We all agree we should be able to rearrange the "projects" in Atom. I say put the rest in another issue. If you're not careful, there will be a big back-and-forth about the "extra" features, and another year will pass before we get even the part that everybody is in agreement on.

@50Wliu 50Wliu dismissed their stale review Oct 21, 2016

Electron require issues fixed

@forivall

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Oct 24, 2016

Contributor

@BinaryMuse I updated the css to be equivalent to the tabs css, and changed the events so that the placeholder isn't removed/re-added when it shouldn't be removed. This appeared to bring down the style recalculation to similar levels as the tabs extension.

Dragging tree-view roots:
tree-view-root-dragging

Dragging tabs:
tabs-dragging

I also call the cleanup function in deactivate (which i had previously written but not hooked up 😝.

Let me know if it still has issues for you. Any further improvement would be experimentation hackery with transforms and/or the web animation api. 😎

Also, I noticed that tabs doesn't use the atom space-pen views anymore, and it looks like y'all have deprecated space-pen. Are there plans to refactor tree-view to remove jquery use, or is that pending switch to a different framework? Cuz I would be open to writing a PR to do so, but don't want to waste effort.

Contributor

forivall commented Oct 24, 2016

@BinaryMuse I updated the css to be equivalent to the tabs css, and changed the events so that the placeholder isn't removed/re-added when it shouldn't be removed. This appeared to bring down the style recalculation to similar levels as the tabs extension.

Dragging tree-view roots:
tree-view-root-dragging

Dragging tabs:
tabs-dragging

I also call the cleanup function in deactivate (which i had previously written but not hooked up 😝.

Let me know if it still has issues for you. Any further improvement would be experimentation hackery with transforms and/or the web animation api. 😎

Also, I noticed that tabs doesn't use the atom space-pen views anymore, and it looks like y'all have deprecated space-pen. Are there plans to refactor tree-view to remove jquery use, or is that pending switch to a different framework? Cuz I would be open to writing a PR to do so, but don't want to waste effort.

@peterschmidler

This comment has been minimized.

Show comment
Hide comment
@peterschmidler

peterschmidler Oct 24, 2016

any progress on this? missing it badly ;)

any progress on this? missing it badly ;)

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 24, 2016

Member

All project folders collapsed and it seems fine(Did not profile just manual testing). Expand one and 💥
drag root 1-2

Member

Ben3eeE commented Oct 24, 2016

All project folders collapsed and it seems fine(Did not profile just manual testing). Expand one and 💥
drag root 1-2

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Oct 24, 2016

Member

@forivall @Ben3eeE This seems to be because we're calling @getPlaceholder().insertBefore/After(element) over and over again from RootDragAndDropHandler#onDragOver even if the drop target index hasn't changed. I recommend the following modification to cache the previous result and bail early if it's the same:

diff --git a/lib/root-drag-and-drop.coffee b/lib/root-drag-and-drop.coffee
index af68e01..829fd15 100644
--- a/lib/root-drag-and-drop.coffee
+++ b/lib/root-drag-and-drop.coffee
@@ -24,6 +24,7 @@ class RootDragAndDropHandler
     @treeView.on 'drop', '.tree-view', @onDrop

   onDragStart: (e) =>
+    @lastDropTargetIndex = null
     e.originalEvent.dataTransfer.setData 'atom-event', 'true'
     projectRoot = $(e.target).closest('.project-root')
     directory = projectRoot[0].directory
@@ -75,6 +76,8 @@ class RootDragAndDropHandler

     newDropTargetIndex = @getDropTargetIndex(e)
     return unless newDropTargetIndex?
+    return if @lastDropTargetIndex is newDropTargetIndex
+    @lastDropTargetIndex = newDropTargetIndex

     @removeDropTargetClasses()

This resolves the jank on my machine.

Finally, I wonder if it makes more sense to put the placeholder above the hovered project if you're on the upper half of the entire entry, not just the header.

dnd-root-half

In that case, we'd additionally need

diff --git a/lib/root-drag-and-drop.coffee b/lib/root-drag-and-drop.coffee
index 829fd15..62edee5 100644
--- a/lib/root-drag-and-drop.coffee
+++ b/lib/root-drag-and-drop.coffee
@@ -157,9 +158,7 @@ class RootDragAndDropHandler

     return 0 unless projectRoot.length

-    element = projectRoot.find('.project-root-header')
-
-    elementCenter = element.offset().top + element.height() / 2
+    elementCenter = projectRoot.offset().top + projectRoot.height() / 2

     if e.originalEvent.pageY < elementCenter
       projectRoots.index(projectRoot)
Member

BinaryMuse commented Oct 24, 2016

@forivall @Ben3eeE This seems to be because we're calling @getPlaceholder().insertBefore/After(element) over and over again from RootDragAndDropHandler#onDragOver even if the drop target index hasn't changed. I recommend the following modification to cache the previous result and bail early if it's the same:

diff --git a/lib/root-drag-and-drop.coffee b/lib/root-drag-and-drop.coffee
index af68e01..829fd15 100644
--- a/lib/root-drag-and-drop.coffee
+++ b/lib/root-drag-and-drop.coffee
@@ -24,6 +24,7 @@ class RootDragAndDropHandler
     @treeView.on 'drop', '.tree-view', @onDrop

   onDragStart: (e) =>
+    @lastDropTargetIndex = null
     e.originalEvent.dataTransfer.setData 'atom-event', 'true'
     projectRoot = $(e.target).closest('.project-root')
     directory = projectRoot[0].directory
@@ -75,6 +76,8 @@ class RootDragAndDropHandler

     newDropTargetIndex = @getDropTargetIndex(e)
     return unless newDropTargetIndex?
+    return if @lastDropTargetIndex is newDropTargetIndex
+    @lastDropTargetIndex = newDropTargetIndex

     @removeDropTargetClasses()

This resolves the jank on my machine.

Finally, I wonder if it makes more sense to put the placeholder above the hovered project if you're on the upper half of the entire entry, not just the header.

dnd-root-half

In that case, we'd additionally need

diff --git a/lib/root-drag-and-drop.coffee b/lib/root-drag-and-drop.coffee
index 829fd15..62edee5 100644
--- a/lib/root-drag-and-drop.coffee
+++ b/lib/root-drag-and-drop.coffee
@@ -157,9 +158,7 @@ class RootDragAndDropHandler

     return 0 unless projectRoot.length

-    element = projectRoot.find('.project-root-header')
-
-    elementCenter = element.offset().top + element.height() / 2
+    elementCenter = projectRoot.offset().top + projectRoot.height() / 2

     if e.originalEvent.pageY < elementCenter
       projectRoots.index(projectRoot)
@forivall

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Oct 24, 2016

Contributor

@BinaryMuse Hmm, I didn't think that reinserting the same element would cause the issue, and I wasn't seeing jank on my machines (OSX or Linux, stable atom). But cool, that's a simpler solution than what I briefly tried over my lunch break (debouncing).

Also, I originally did have the placeholder position based on the center of the entire entry, but switched it to the header during the phase where on-disk dragging was allowed. 👍 to reverting back to that placeholder positioning.

Contributor

forivall commented Oct 24, 2016

@BinaryMuse Hmm, I didn't think that reinserting the same element would cause the issue, and I wasn't seeing jank on my machines (OSX or Linux, stable atom). But cool, that's a simpler solution than what I briefly tried over my lunch break (debouncing).

Also, I originally did have the placeholder position based on the center of the entire entry, but switched it to the header during the phase where on-disk dragging was allowed. 👍 to reverting back to that placeholder positioning.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 25, 2016

Member

@BinaryMuse Nice 👍

I will test this again later tonight to verify functionality and responsiveness on my machine.

Member

Ben3eeE commented Oct 25, 2016

@BinaryMuse Nice 👍

I will test this again later tonight to verify functionality and responsiveness on my machine.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 25, 2016

Member

Accidentally did this I don't know what I expected to happen 😆
525 bug

Similarly in reverse:
525 bug 2

Member

Ben3eeE commented Oct 25, 2016

Accidentally did this I don't know what I expected to happen 😆
525 bug

Similarly in reverse:
525 bug 2

@forivall

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Oct 25, 2016

Contributor

@Ben3eeE Fixed by 7dbae50 😆

Contributor

forivall commented Oct 25, 2016

@Ben3eeE Fixed by 7dbae50 😆

@forivall

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Oct 26, 2016

Contributor

@BinaryMuse I also added the change for

Finally, I wonder if it makes more sense to put the placeholder above the hovered project if you're on the upper half of the entire entry, not just the header.

in edf9e5b. Try it out!

Contributor

forivall commented Oct 26, 2016

@BinaryMuse I also added the change for

Finally, I wonder if it makes more sense to put the placeholder above the hovered project if you're on the upper half of the entire entry, not just the header.

in edf9e5b. Try it out!

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Oct 26, 2016

Member

@forivall This looks awesome, great work! I'm going to look into the spec failures (which have nothing to do with your PR) and once that's resolved I'll merge this. 🎉

[Edit] I lied, gonna worry about the Windows stuff later. To the Merge Button!

Member

BinaryMuse commented Oct 26, 2016

@forivall This looks awesome, great work! I'm going to look into the spec failures (which have nothing to do with your PR) and once that's resolved I'll merge this. 🎉

[Edit] I lied, gonna worry about the Windows stuff later. To the Merge Button!

@BinaryMuse BinaryMuse merged commit 8ebef1e into atom:master Oct 26, 2016

1 of 2 checks passed

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

Ben3eeE added a commit to atom/tabs that referenced this pull request Oct 26, 2016

@JetUni

This comment has been minimized.

Show comment
Hide comment
@JetUni

JetUni Oct 28, 2016

@forivall THANK YOU SO MUCH!!!!! 🎉🎉

JetUni commented Oct 28, 2016

@forivall THANK YOU SO MUCH!!!!! 🎉🎉

@peterschmidler

This comment has been minimized.

Show comment
Hide comment
@peterschmidler

peterschmidler Oct 31, 2016

How can I use this feature?

How can I use this feature?

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Oct 31, 2016

Member

If you want it right now, you'll have to build Atom from source. If you can wait, this will be available in the next beta release.

Member

thomasjo commented Oct 31, 2016

If you want it right now, you'll have to build Atom from source. If you can wait, this will be available in the next beta release.

@forivall

This comment has been minimized.

Show comment
Hide comment
@forivall

forivall Oct 31, 2016

Contributor

@peterschmidler you can also clone this repository and npm install && apm rebuild && apm link .

Contributor

forivall commented Oct 31, 2016

@peterschmidler you can also clone this repository and npm install && apm rebuild && apm link .

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 31, 2016

Member

Or just apm install && apm link.

Member

50Wliu commented Oct 31, 2016

Or just apm install && apm link.

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Oct 31, 2016

Member

Keep in mind that installing the package from a local source via apm link will shadow the built-in package and keep you from using the latest version when Atom updates.

Member

BinaryMuse commented Oct 31, 2016

Keep in mind that installing the package from a local source via apm link will shadow the built-in package and keep you from using the latest version when Atom updates.

@KSXGitHub

This comment has been minimized.

Show comment
Hide comment
@KSXGitHub

KSXGitHub Nov 1, 2016

What about apm install tree-view?

What about apm install tree-view?

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Nov 1, 2016

Member

@KSXGitHub I guess to be more specific, any mechanism by which you place a package with the same name as a core, bundled package in your .atom/packages directory will shadow the bundled package (that's why doing it makes the feature work. :) But if you forget to remove it, Atom will continue to use the one you installed instead of the bundled one, even if the bundled one is newer.

Member

BinaryMuse commented Nov 1, 2016

@KSXGitHub I guess to be more specific, any mechanism by which you place a package with the same name as a core, bundled package in your .atom/packages directory will shadow the bundled package (that's why doing it makes the feature work. :) But if you forget to remove it, Atom will continue to use the one you installed instead of the bundled one, even if the bundled one is newer.

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Nov 1, 2016

Member

I highly advise people to not install core packages directly — it only leads to tears.

Core packages should only be symlinked in dev mode when you are actively working on the package.

Since this issue has gone completely off-topic, and needlessly notifying 21+ people is unnecessary, I'm locking this thread. If someone wants to discuss this further, do it on the Slack channel or on Discuss 🙇

Member

thomasjo commented Nov 1, 2016

I highly advise people to not install core packages directly — it only leads to tears.

Core packages should only be symlinked in dev mode when you are actively working on the package.

Since this issue has gone completely off-topic, and needlessly notifying 21+ people is unnecessary, I'm locking this thread. If someone wants to discuss this further, do it on the Slack channel or on Discuss 🙇

@atom atom locked and limited conversation to collaborators Nov 1, 2016

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